| Message ID | 20180706135529.88966-9-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06.07.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 | 32 +++++++++++++++++++++++++++++--- > arch/s390/mm/pageattr.c | 6 ++---- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index c898e3a4628b..90f3c9dae983 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2545,17 +2545,43 @@ 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; > + > + /* > + * The write check makes sure we do not set a key on shared > + * memory. This is needed as the walker does not differentiate > + * between actual guest memory and the process executable or > + * shared libraries. > + */ > + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID || > + !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) > + return 0; With the given comment, this is interesting. Actually, I am not sure if this interface (s390_enable_skey()) is what we actually want. Think about the following: 1. User space setups a guest. 2. Guest executes a skey operation. Skeys initialized for the whole MM of user space. 3. User space adds more memory to the guest (hotplug via some mechanism) - new kvm memory slot. For 3. I don't think that skeys are initialized for the new memory slot. Am I missing something? If so, what we actually need might be the following: 1. int s390_init_skey(uint64_t vmstart, uint64_t vmend); 2. kvm_s390_enable_skey() -> Loop over all KVM memory slots, calling s390_enable_skey() 3. In kvm_arch_commit_memory_region() / gmap_map_segment() -> call s390_init_skey for all *new* memory We can of course use some flag to do initialization lazily just as now (mm->context.uses_skeys). This avoids initializing storage keys for !guest related stuff (e.g. your write check above) and we catch new VMAs. AFAICS we have the same problem already with current s390_enable_skey() if I am not missing something important here. So this should be fixed (if applicable) independently and not here. Any experts? > + > + 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; > } > } >
On Thu, 12 Jul 2018 09:17:45 +0200 David Hildenbrand <david@redhat.com> wrote: > On 06.07.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 | 32 +++++++++++++++++++++++++++++--- > > arch/s390/mm/pageattr.c | 6 ++---- > > 2 files changed, 31 insertions(+), 7 deletions(-) > > > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > > index c898e3a4628b..90f3c9dae983 100644 > > --- a/arch/s390/mm/gmap.c > > +++ b/arch/s390/mm/gmap.c > > @@ -2545,17 +2545,43 @@ 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; > > + > > + /* > > + * The write check makes sure we do not set a key on shared > > + * memory. This is needed as the walker does not > > differentiate > > + * between actual guest memory and the process executable > > or > > + * shared libraries. > > + */ > > + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID || > > + !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) > > + return 0; > > With the given comment, this is interesting. Actually, I am not sure > if this interface (s390_enable_skey()) is what we actually want. Think > about the following: > > 1. User space setups a guest. > 2. Guest executes a skey operation. Skeys initialized for the whole MM > of user space. > 3. User space adds more memory to the guest (hotplug via some > mechanism) > - new kvm memory slot. > > For 3. I don't think that skeys are initialized for the new memory > slot. Am I missing something? We set the pgste key on xchg with a proper pte, but only when mm_use_skey(), so all new mappings have 0 keys after s390_enable_skey(). I asked Martin about pmds and he told me there are other mechanisms around for them, but I didn't find them yet. > > If so, what we actually need might be the following: > > 1. int s390_init_skey(uint64_t vmstart, uint64_t vmend); > > 2. kvm_s390_enable_skey() > -> Loop over all KVM memory slots, calling s390_enable_skey() > > 3. In kvm_arch_commit_memory_region() / gmap_map_segment() > -> call s390_init_skey for all *new* memory > > We can of course use some flag to do initialization lazily just as now > (mm->context.uses_skeys). > > This avoids initializing storage keys for !guest related stuff (e.g. > your write check above) and we catch new VMAs. > > > AFAICS we have the same problem already with current > s390_enable_skey() if I am not missing something important here. So > this should be fixed (if applicable) independently and not here. > > Any experts? > > > + > > + 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; > > } > > } > > > >
On 12.07.2018 09:50, frankja wrote: > On Thu, 12 Jul 2018 09:17:45 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 06.07.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 | 32 +++++++++++++++++++++++++++++--- >>> arch/s390/mm/pageattr.c | 6 ++---- >>> 2 files changed, 31 insertions(+), 7 deletions(-) >>> >>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >>> index c898e3a4628b..90f3c9dae983 100644 >>> --- a/arch/s390/mm/gmap.c >>> +++ b/arch/s390/mm/gmap.c >>> @@ -2545,17 +2545,43 @@ 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; >>> + >>> + /* >>> + * The write check makes sure we do not set a key on shared >>> + * memory. This is needed as the walker does not >>> differentiate >>> + * between actual guest memory and the process executable >>> or >>> + * shared libraries. >>> + */ >>> + if (pmd_val(*pmd) & _SEGMENT_ENTRY_INVALID || >>> + !(pmd_val(*pmd) & _SEGMENT_ENTRY_WRITE)) >>> + return 0; >> >> With the given comment, this is interesting. Actually, I am not sure >> if this interface (s390_enable_skey()) is what we actually want. Think >> about the following: >> >> 1. User space setups a guest. >> 2. Guest executes a skey operation. Skeys initialized for the whole MM >> of user space. >> 3. User space adds more memory to the guest (hotplug via some >> mechanism) >> - new kvm memory slot. >> >> For 3. I don't think that skeys are initialized for the new memory >> slot. Am I missing something? > > We set the pgste key on xchg with a proper pte, but only when > mm_use_skey(), so all new mappings have 0 keys after s390_enable_skey(). > > I asked Martin about pmds and he told me there are other mechanisms > around for them, but I didn't find them yet. Ah right, I was missing the pgste_set_key() function. I am also still looking for the PMD counterpart.
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index c898e3a4628b..90f3c9dae983 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2545,17 +2545,43 @@ 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; + + /* + * The write check makes sure we do not set a key on shared + * memory. This is needed as the walker does not differentiate + * between actual guest memory and the process executable or + * shared libraries. + */ + 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; } }