On Thu, 12 Jul 2018 09:17:45 +0200
David Hildenbrand <david@xxxxxxxxxx> wrote:
> On 06.07.2018 15:55, Janosch Frank wrote:
> > From: Dominik Dingel <dingel@xxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> > [replaced page_set_storage_key loop with __storage_key_init_range]
> > Reviewed-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > ---
> > 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;
> > }
> > }
> >
>
>
