Message ID | 20180627135510.117945-7-frankja@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 27.06.2018 15:55, Janosch Frank wrote: > From: Dominik Dingel <dingel@linux.vnet.ibm.com> > > When a guest starts using storage keys, we trap and set a default one > for its whole valid address space. With this patch we are now able to > do that for large pages. > > To speed up the storage key insertion, we use > __storage_key_init_range, which in-turn will use sske_frame to set > multiple storage keys with one instruction. As it has been previously > used for debuging we have to get rid of the default key check and make > it quiescing. > > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > [replaced page_set_storage_key loop with __storage_key_init_range] > Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > arch/s390/mm/gmap.c | 26 +++++++++++++++++++++++--- > arch/s390/mm/pageattr.c | 6 ++---- > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 3da4f85ec330..af0a87eeede0 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2761,17 +2761,37 @@ EXPORT_SYMBOL_GPL(s390_enable_sie); > * Enable storage key handling from now on and initialize the storage > * keys with the default key. > */ > -static int __s390_enable_skey(pte_t *pte, unsigned long addr, > - unsigned long next, struct mm_walk *walk) > +static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr, > + unsigned long next, struct mm_walk *walk) > { > /* Clear storage key */ > ptep_zap_key(walk->mm, addr, pte); > return 0; > } > > +static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr, > + unsigned long hmask, unsigned long next, > + struct mm_walk *walk) > +{ > + pmd_t *pmd = (pmd_t *)pte; > + unsigned long start, end; > + > + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID > + || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) In this file, we usually have the "||"/"&&" at the end of the previous line. Silly, question: why do we have to handle invalid entries here and why do we have to skip !write? Can both even happen? Should we simply not initialize storage keys here? > + return 0; > + > + start = pmd_val(*pmd) & HPAGE_MASK; > + end = start + HPAGE_SIZE - 1; > + __storage_key_init_range(start, end); > + return 0; > +} > + > int s390_enable_skey(void) > { > - struct mm_walk walk = { .pte_entry = __s390_enable_skey }; > + struct mm_walk walk = { > + .hugetlb_entry = __s390_enable_skey_hugetlb, > + .pte_entry = __s390_enable_skey_pte, > + }; > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > int rc = 0; > diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c > index c44171588d08..f8c6faab41f4 100644 > --- a/arch/s390/mm/pageattr.c > +++ b/arch/s390/mm/pageattr.c > @@ -14,7 +14,7 @@ > > static inline unsigned long sske_frame(unsigned long addr, unsigned char skey) > { > - asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0" > + asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0" > : [addr] "+a" (addr) : [skey] "d" (skey)); > return addr; > } > @@ -23,8 +23,6 @@ void __storage_key_init_range(unsigned long start, unsigned long end) > { > unsigned long boundary, size; > > - if (!PAGE_DEFAULT_KEY) > - return; > while (start < end) { > if (MACHINE_HAS_EDAT1) { > /* set storage keys for a 1MB frame */ > @@ -37,7 +35,7 @@ void __storage_key_init_range(unsigned long start, unsigned long end) > continue; > } > } > - page_set_storage_key(start, PAGE_DEFAULT_KEY, 0); > + page_set_storage_key(start, PAGE_DEFAULT_KEY, 1); Why do we have to change quiescing here? And why in sske_frame? (in other words: why did it work previously?) > start += PAGE_SIZE; > } > } >
On 28.06.2018 10:07, David Hildenbrand wrote: > On 27.06.2018 15:55, Janosch Frank wrote: >> From: Dominik Dingel <dingel@linux.vnet.ibm.com> >> >> When a guest starts using storage keys, we trap and set a default one >> for its whole valid address space. With this patch we are now able to >> do that for large pages. >> >> To speed up the storage key insertion, we use >> __storage_key_init_range, which in-turn will use sske_frame to set >> multiple storage keys with one instruction. As it has been previously >> used for debuging we have to get rid of the default key check and make >> it quiescing. >> >> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> >> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> [replaced page_set_storage_key loop with __storage_key_init_range] >> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com> >> --- >> arch/s390/mm/gmap.c | 26 +++++++++++++++++++++++--- >> arch/s390/mm/pageattr.c | 6 ++---- >> 2 files changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index 3da4f85ec330..af0a87eeede0 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -2761,17 +2761,37 @@ EXPORT_SYMBOL_GPL(s390_enable_sie); >> * Enable storage key handling from now on and initialize the storage >> * keys with the default key. >> */ >> -static int __s390_enable_skey(pte_t *pte, unsigned long addr, >> - unsigned long next, struct mm_walk *walk) >> +static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr, >> + unsigned long next, struct mm_walk *walk) >> { >> /* Clear storage key */ >> ptep_zap_key(walk->mm, addr, pte); >> return 0; >> } >> >> +static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr, >> + unsigned long hmask, unsigned long next, >> + struct mm_walk *walk) >> +{ >> + pmd_t *pmd = (pmd_t *)pte; >> + unsigned long start, end; >> + >> + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID >> + || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) > > In this file, we usually have the "||"/"&&" at the end of the previous line. > > Silly, question: why do we have to handle invalid entries here and why > do we have to skip !write? Can both even happen? Should we simply not > initialize storage keys here? walk->hugetlb_range does not check the I bit before calling the handler, hence we have to. The write check is indeed weird, especially as it is the software bit. > >> + return 0; >> + >> + start = pmd_val(*pmd) & HPAGE_MASK; >> + end = start + HPAGE_SIZE - 1; >> + __storage_key_init_range(start, end); >> + return 0; >> +} >> + >> int s390_enable_skey(void) >> { >> - struct mm_walk walk = { .pte_entry = __s390_enable_skey }; >> + struct mm_walk walk = { >> + .hugetlb_entry = __s390_enable_skey_hugetlb, >> + .pte_entry = __s390_enable_skey_pte, >> + }; >> struct mm_struct *mm = current->mm; >> struct vm_area_struct *vma; >> int rc = 0; >> diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c >> index c44171588d08..f8c6faab41f4 100644 >> --- a/arch/s390/mm/pageattr.c >> +++ b/arch/s390/mm/pageattr.c >> @@ -14,7 +14,7 @@ >> >> static inline unsigned long sske_frame(unsigned long addr, unsigned char skey) >> { >> - asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0" >> + asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0" >> : [addr] "+a" (addr) : [skey] "d" (skey)); >> return addr; >> } >> @@ -23,8 +23,6 @@ void __storage_key_init_range(unsigned long start, unsigned long end) >> { >> unsigned long boundary, size; >> >> - if (!PAGE_DEFAULT_KEY) >> - return; >> while (start < end) { >> if (MACHINE_HAS_EDAT1) { >> /* set storage keys for a 1MB frame */ >> @@ -37,7 +35,7 @@ void __storage_key_init_range(unsigned long start, unsigned long end) >> continue; >> } >> } >> - page_set_storage_key(start, PAGE_DEFAULT_KEY, 0); >> + page_set_storage_key(start, PAGE_DEFAULT_KEY, 1); > > Why do we have to change quiescing here? And why in sske_frame? (in > other words: why did it work previously?) Previously this was only debug code, let me cite Heiko: The whole code only exists for debugging purposes, where we might set PAGE_DEFAULT_KEY to a value that is not zero. The last time I did that however is a couple of years ago. And that was in order to prove that z/VM modified the storage keys used by Linux. > >> start += PAGE_SIZE; >> } >> } >> > >
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 3da4f85ec330..af0a87eeede0 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2761,17 +2761,37 @@ EXPORT_SYMBOL_GPL(s390_enable_sie); * Enable storage key handling from now on and initialize the storage * keys with the default key. */ -static int __s390_enable_skey(pte_t *pte, unsigned long addr, - unsigned long next, struct mm_walk *walk) +static int __s390_enable_skey_pte(pte_t *pte, unsigned long addr, + unsigned long next, struct mm_walk *walk) { /* Clear storage key */ ptep_zap_key(walk->mm, addr, pte); return 0; } +static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr, + unsigned long hmask, unsigned long next, + struct mm_walk *walk) +{ + pmd_t *pmd = (pmd_t *)pte; + unsigned long start, end; + + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID + || !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) + return 0; + + start = pmd_val(*pmd) & HPAGE_MASK; + end = start + HPAGE_SIZE - 1; + __storage_key_init_range(start, end); + return 0; +} + int s390_enable_skey(void) { - struct mm_walk walk = { .pte_entry = __s390_enable_skey }; + struct mm_walk walk = { + .hugetlb_entry = __s390_enable_skey_hugetlb, + .pte_entry = __s390_enable_skey_pte, + }; struct mm_struct *mm = current->mm; struct vm_area_struct *vma; int rc = 0; diff --git a/arch/s390/mm/pageattr.c b/arch/s390/mm/pageattr.c index c44171588d08..f8c6faab41f4 100644 --- a/arch/s390/mm/pageattr.c +++ b/arch/s390/mm/pageattr.c @@ -14,7 +14,7 @@ static inline unsigned long sske_frame(unsigned long addr, unsigned char skey) { - asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0" + asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0" : [addr] "+a" (addr) : [skey] "d" (skey)); return addr; } @@ -23,8 +23,6 @@ void __storage_key_init_range(unsigned long start, unsigned long end) { unsigned long boundary, size; - if (!PAGE_DEFAULT_KEY) - return; while (start < end) { if (MACHINE_HAS_EDAT1) { /* set storage keys for a 1MB frame */ @@ -37,7 +35,7 @@ void __storage_key_init_range(unsigned long start, unsigned long end) continue; } } - page_set_storage_key(start, PAGE_DEFAULT_KEY, 0); + page_set_storage_key(start, PAGE_DEFAULT_KEY, 1); start += PAGE_SIZE; } }