| Message ID | 20180706135529.88966-5-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06.07.2018 15:55, Janosch Frank wrote: > Let's allow pmds to be linked into gmap for the upcoming s390 KVM huge > page support. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/pgtable.h | 6 ++++-- > arch/s390/mm/gmap.c | 13 +++++++++---- > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > index 5ab636089c60..fe36b3bb2afd 100644 > --- a/arch/s390/include/asm/pgtable.h > +++ b/arch/s390/include/asm/pgtable.h > @@ -268,8 +268,10 @@ static inline int is_module_addr(void *addr) > #define _REGION_ENTRY_BITS_LARGE 0xffffffff8000fe2fUL > > /* Bits in the segment table entry */ > -#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL > -#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL > +#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL > +#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL > +#define _SEGMENT_ENTRY_HARDWARE_BITS 0xfffffffffffffe30UL > +#define _SEGMENT_ENTRY_HARDWARE_BITS_LARGE 0xfffffffffff00730UL > #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL /* large page address */ > #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL/* page table origin */ > #define _SEGMENT_ENTRY_PROTECT 0x200 /* segment protection bit */ > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 177419f01449..e31c365d4a2f 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -599,10 +599,15 @@ int __gmap_link(struct gmap *gmap, unsigned long gaddr, unsigned long vmaddr) > if (*table == _SEGMENT_ENTRY_EMPTY) { > rc = radix_tree_insert(&gmap->host_to_guest, > vmaddr >> PMD_SHIFT, table); > - if (!rc) > - *table = pmd_val(*pmd); So until now we copied all bits (SW and HW), but now we want to only copy the HW bits. You should add that to the patch description, because it is also a change for 4k backed guests. Something like "The GMAP tables should never copy SW defined bits from process page tables that might be interpreted differently in GMAP tables. So let's drop all SW defined bits." > - } else > - rc = 0; > + if (!rc) { > + if (pmd_large(*pmd)) { > + *table = pmd_val(*pmd) & > + _SEGMENT_ENTRY_HARDWARE_BITS_LARGE; > + } else > + *table = pmd_val(*pmd) & > + _SEGMENT_ENTRY_HARDWARE_BITS; > + } > + } > spin_unlock(&gmap->guest_table_lock); > spin_unlock(ptl); > radix_tree_preload_end(); > I think this should be fine and we never actually accessed these SW bits via the GMAP (would be wrong). Reviewed-by: David Hildenbrand <david@redhat.com>
On 06.07.2018 16:24, David Hildenbrand wrote: > On 06.07.2018 15:55, Janosch Frank wrote: >> Let's allow pmds to be linked into gmap for the upcoming s390 KVM huge >> page support. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/pgtable.h | 6 ++++-- >> arch/s390/mm/gmap.c | 13 +++++++++---- >> 2 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h >> index 5ab636089c60..fe36b3bb2afd 100644 >> --- a/arch/s390/include/asm/pgtable.h >> +++ b/arch/s390/include/asm/pgtable.h >> @@ -268,8 +268,10 @@ static inline int is_module_addr(void *addr) >> #define _REGION_ENTRY_BITS_LARGE 0xffffffff8000fe2fUL >> >> /* Bits in the segment table entry */ >> -#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL >> -#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL >> +#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL >> +#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL >> +#define _SEGMENT_ENTRY_HARDWARE_BITS 0xfffffffffffffe30UL >> +#define _SEGMENT_ENTRY_HARDWARE_BITS_LARGE 0xfffffffffff00730UL >> #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL /* large page address */ >> #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL/* page table origin */ >> #define _SEGMENT_ENTRY_PROTECT 0x200 /* segment protection bit */ >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index 177419f01449..e31c365d4a2f 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -599,10 +599,15 @@ int __gmap_link(struct gmap *gmap, unsigned long gaddr, unsigned long vmaddr) >> if (*table == _SEGMENT_ENTRY_EMPTY) { >> rc = radix_tree_insert(&gmap->host_to_guest, >> vmaddr >> PMD_SHIFT, table); >> - if (!rc) >> - *table = pmd_val(*pmd); > > So until now we copied all bits (SW and HW), but now we want to only > copy the HW bits. You should add that to the patch description, because > it is also a change for 4k backed guests. > > Something like "The GMAP tables should never copy SW defined bits from > process page tables that might be interpreted differently in GMAP > tables. So let's drop all SW defined bits." Added: Before this patch we copied the full userspace pmd entry. This is not correct, as it contains SW defined bits that might be interpreted differently in the GMAP context. Now we only copy over all hardware relevant information leaving out the software bits. > >> - } else >> - rc = 0; >> + if (!rc) { >> + if (pmd_large(*pmd)) { >> + *table = pmd_val(*pmd) & >> + _SEGMENT_ENTRY_HARDWARE_BITS_LARGE; >> + } else >> + *table = pmd_val(*pmd) & >> + _SEGMENT_ENTRY_HARDWARE_BITS; >> + } >> + } >> spin_unlock(&gmap->guest_table_lock); >> spin_unlock(ptl); >> radix_tree_preload_end(); >> > > I think this should be fine and we never actually accessed these SW bits > via the GMAP (would be wrong). > > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 5ab636089c60..fe36b3bb2afd 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -268,8 +268,10 @@ static inline int is_module_addr(void *addr) #define _REGION_ENTRY_BITS_LARGE 0xffffffff8000fe2fUL /* Bits in the segment table entry */ -#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL -#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL +#define _SEGMENT_ENTRY_BITS 0xfffffffffffffe33UL +#define _SEGMENT_ENTRY_BITS_LARGE 0xfffffffffff0ff33UL +#define _SEGMENT_ENTRY_HARDWARE_BITS 0xfffffffffffffe30UL +#define _SEGMENT_ENTRY_HARDWARE_BITS_LARGE 0xfffffffffff00730UL #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL /* large page address */ #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL/* page table origin */ #define _SEGMENT_ENTRY_PROTECT 0x200 /* segment protection bit */ diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 177419f01449..e31c365d4a2f 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -599,10 +599,15 @@ int __gmap_link(struct gmap *gmap, unsigned long gaddr, unsigned long vmaddr) if (*table == _SEGMENT_ENTRY_EMPTY) { rc = radix_tree_insert(&gmap->host_to_guest, vmaddr >> PMD_SHIFT, table); - if (!rc) - *table = pmd_val(*pmd); - } else - rc = 0; + if (!rc) { + if (pmd_large(*pmd)) { + *table = pmd_val(*pmd) & + _SEGMENT_ENTRY_HARDWARE_BITS_LARGE; + } else + *table = pmd_val(*pmd) & + _SEGMENT_ENTRY_HARDWARE_BITS; + } + } spin_unlock(&gmap->guest_table_lock); spin_unlock(ptl); radix_tree_preload_end();
Let's allow pmds to be linked into gmap for the upcoming s390 KVM huge page support. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/include/asm/pgtable.h | 6 ++++-- arch/s390/mm/gmap.c | 13 +++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-)