* Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control [not found] <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com> @ 2018-02-27 12:47 ` Janosch Frank 0 siblings, 0 replies; 5+ messages in thread From: Janosch Frank @ 2018-02-27 12:47 UTC (permalink / raw) To: linux-s390, kvm [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: multipart/mixed; boundary="--9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m", Size: 3526 bytes --] This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m Content-Type: multipart/mixed; boundary="YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj"; protected-headers="v1" From: Janosch Frank <frankja@linux.vnet.ibm.com> To: David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org Cc: schwidefsky@de.ibm.com, borntraeger@de.ibm.com, alifm@linux.vnet.ibm.com, imbrenda@linux.vnet.ibm.com, linux-s390@vger.kernel.org Message-ID: <52697ed8-f1d0-b7e1-3e56-00d89c28f633@linux.vnet.ibm.com> Subject: Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control References: <1518779775-256056-1-git-send-email-frankja@linux.vnet.ibm.com> <1518779775-256056-3-git-send-email-frankja@linux.vnet.ibm.com> <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com> In-Reply-To: <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com> --YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 27.02.2018 10:40, David Hildenbrand wrote: >=20 >> spin_lock_init(&kvm->arch.start_stop_lock); >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index 76a2380..d9bd147 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *= vcpu) >> struct kvm_s390_sie_block *sie_block =3D vcpu->arch.sie_block; >> =20 >> trace_kvm_s390_skey_related_inst(vcpu); >> - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> + /* Already enabled? */ >> + if (vcpu->kvm->arch.use_skf && >> + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> return rc; >=20 > Wonder if it is nicer to simply remember for each CPU if this function > has already been called. This way we can avoid calling > s390_enable_skey() in all configurations. >=20 With the benefit being, that on a per cpu storage we wouldn't have to lock the call indication, as we are the thread of this vcpu and only we can alter it? Which would speed up hpage and VSIE cases a tad, as the down_write is quite heavy. We could also do a down_read, store enablement status, up_read, if !enabled -> 390_enable_skey(). I'm also wondering, if we have a clean way after migrating a skey using guest to disable interception right away when we get the skeys from QEMU. I don't think we do that yet. --YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj-- --9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJalVN4AAoJEBcO/8Q8ZEV5HkQQALmhUssaNJ/mwJilQA8ArIRg 0xiYhOmTzYQcVPYQa2sRZfgbBnuhM//5JPvkXM4aMrucywqgEj1O1arGhZiH3dH7 8EGN7+evOyVLB125CjG5N9tmSkROe3ilRbBa3aTPeK9ZwGp/EkzcbF834rDV8dMl dLjH7kgD9NgQIZ2SUP45TEPOzhk3VXahlTsJ44SsXBb1G5bNjW+fN7gWEmni4Kas dwWcnnbSY4sFuGZFzcc/ls0h3oPo2eGln/ssvct8qjEeL31rljzoXuYYeboLT2hr MTowGcaRQvjCZk9t8XtoKoRPmL2os6HCvNbAucjzDqLJetSoBR2PlCsv7gnEVJFC X9FcCbYj3G18mxpGgy40tVZbdoY0mGc9NipKdkDvGm8DZPzQnBL3VQOtyo7WI1AI 90VG/H86zg3VstgbyhZiN/RMzP/e1j+Cv9UlHOxugSPaqgID49gmKhCy+HB0b7Oi VU5ImaiSXvH7OFcZ5JKytbpJrFRX90+E3qIS9GPDSQzj4WAstZDDn7pp2G9xy/MF u7UuxwfMa0iWAJWnHR8lOu/+B8c2tFAeZMivfsombPbundZzEmDTq0kwVt7jsn4S kUWH2eYrl/tAJcOmfiiSUcmWC0jsLcohusS0TWgbvzDILFT0k2aGDTO2Gfv5nX49 hdtsHtgvNq6GEi3RDKQD =RlCV -----END PGP SIGNATURE----- --9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m-- ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 0/2] KVM: s390: Refactor host interpretation facilities controls @ 2018-02-16 11:16 Janosch Frank 2018-02-16 11:16 ` [PATCH 2/2] KVM: s390: Add storage key facility interpretation control Janosch Frank 0 siblings, 1 reply; 5+ messages in thread From: Janosch Frank @ 2018-02-16 11:16 UTC (permalink / raw) To: kvm; +Cc: schwidefsky, borntraeger, david, alifm, imbrenda, linux-s390 The host interpretation controls and usage indication variables need some renaming, so one can actually distinguish them by name. For the sake of completeness and later expected use, we introduce controls for pfmfi and the storage key facility (skf). Janosch Frank (2): KVM: s390: Refactor host cmma and pfmfi interpretation controls KVM: s390: Add storage key facility interpretation control arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/include/asm/mmu.h | 6 +++--- arch/s390/include/asm/mmu_context.h | 4 ++-- arch/s390/include/asm/pgtable.h | 4 ++-- arch/s390/kvm/kvm-s390.c | 26 ++++++++++++++------------ arch/s390/kvm/priv.c | 26 +++++++++++++++----------- arch/s390/mm/gmap.c | 6 +++--- arch/s390/mm/pgtable.c | 4 ++-- 8 files changed, 43 insertions(+), 35 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] KVM: s390: Add storage key facility interpretation control 2018-02-16 11:16 [PATCH 0/2] KVM: s390: Refactor host interpretation facilities controls Janosch Frank @ 2018-02-16 11:16 ` Janosch Frank 2018-02-16 14:30 ` David Hildenbrand 0 siblings, 1 reply; 5+ messages in thread From: Janosch Frank @ 2018-02-16 11:16 UTC (permalink / raw) To: kvm; +Cc: schwidefsky, borntraeger, david, alifm, imbrenda, linux-s390 Up to now we always expected to have the storage key facility available for our (non-VSIE) KVM guests. For huge page support, we need to be able to disable it, so let's introduce that now. Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com> --- arch/s390/include/asm/kvm_host.h | 1 + arch/s390/include/asm/mmu.h | 2 +- arch/s390/include/asm/mmu_context.h | 2 +- arch/s390/include/asm/pgtable.h | 4 ++-- arch/s390/kvm/kvm-s390.c | 3 ++- arch/s390/kvm/priv.c | 22 +++++++++++++--------- arch/s390/mm/gmap.c | 6 +++--- arch/s390/mm/pgtable.c | 4 ++-- 8 files changed, 25 insertions(+), 19 deletions(-) diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 27918b1..f161ad0 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -793,6 +793,7 @@ struct kvm_arch{ int use_irqchip; int use_cmma; int use_pfmfi; + int use_skf; int user_cpu_state_ctrl; int user_sigp; int user_stsi; diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h index c639c95..f5ff9db 100644 --- a/arch/s390/include/asm/mmu.h +++ b/arch/s390/include/asm/mmu.h @@ -21,7 +21,7 @@ typedef struct { /* The mmu context uses extended page tables. */ unsigned int has_pgste:1; /* The mmu context uses storage keys. */ - unsigned int use_skey:1; + unsigned int uses_skeys:1; /* The mmu context uses CMM. */ unsigned int uses_cmm:1; } mm_context_t; diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h index d3ebfa8..bc9a2a9 100644 --- a/arch/s390/include/asm/mmu_context.h +++ b/arch/s390/include/asm/mmu_context.h @@ -30,7 +30,7 @@ static inline int init_new_context(struct task_struct *tsk, test_thread_flag(TIF_PGSTE) || (current->mm && current->mm->context.alloc_pgste); mm->context.has_pgste = 0; - mm->context.use_skey = 0; + mm->context.uses_skeys = 0; mm->context.uses_cmm = 0; #endif switch (mm->context.asce_limit) { diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 9223b4d..4f26425 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -509,10 +509,10 @@ static inline int mm_alloc_pgste(struct mm_struct *mm) * faults should no longer be backed by zero pages */ #define mm_forbids_zeropage mm_has_pgste -static inline int mm_use_skey(struct mm_struct *mm) +static inline int mm_uses_skeys(struct mm_struct *mm) { #ifdef CONFIG_PGSTE - if (mm->context.use_skey) + if (mm->context.uses_skeys) return 1; #endif return 0; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 8fb6549..90deb7b 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1432,7 +1432,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) return -EINVAL; /* Is this guest using storage keys? */ - if (!mm_use_skey(current->mm)) + if (!mm_uses_skeys(current->mm)) return KVM_S390_GET_SKEYS_NONE; /* Enforce sane limit on memory allocation */ @@ -2010,6 +2010,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) kvm->arch.css_support = 0; kvm->arch.use_irqchip = 0; kvm->arch.use_pfmfi = sclp.has_pfmfi; + kvm->arch.use_skf = sclp.has_skey; kvm->arch.epoch = 0; spin_lock_init(&kvm->arch.start_stop_lock); diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 76a2380..d9bd147 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block; trace_kvm_s390_skey_related_inst(vcpu); - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && + /* Already enabled? */ + if (vcpu->kvm->arch.use_skf && + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) return rc; rc = s390_enable_skey(); VCPU_EVENT(vcpu, 3, "enabling storage keys for guest: %d", rc); - if (!rc) { - if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) - kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); - else - sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | - ICTL_RRBE); - } + if (rc) + return rc; + + if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) + kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); + if (!vcpu->kvm->arch.use_skf) + sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; + else + sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); return rc; } @@ -231,7 +235,7 @@ static int try_handle_skey(struct kvm_vcpu *vcpu) rc = kvm_s390_skey_check_enable(vcpu); if (rc) return rc; - if (sclp.has_skey) { + if (vcpu->kvm->arch.use_skf) { /* with storage-key facility, SIE interprets it for us */ kvm_s390_retry_instr(vcpu); VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation"); diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 2cafcba..dbdcd25 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -3094,14 +3094,14 @@ int s390_enable_skey(void) int rc = 0; down_write(&mm->mmap_sem); - if (mm_use_skey(mm)) + if (mm_uses_skeys(mm)) goto out_up; - mm->context.use_skey = 1; + mm->context.uses_skeys = 1; for (vma = mm->mmap; vma; vma = vma->vm_next) { if (ksm_madvise(vma, vma->vm_start, vma->vm_end, MADV_UNMERGEABLE, &vma->vm_flags)) { - mm->context.use_skey = 0; + mm->context.uses_skeys = 0; rc = -ENOMEM; goto out_up; } diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c index 871fc65..158c880 100644 --- a/arch/s390/mm/pgtable.c +++ b/arch/s390/mm/pgtable.c @@ -158,7 +158,7 @@ static inline pgste_t pgste_update_all(pte_t pte, pgste_t pgste, #ifdef CONFIG_PGSTE unsigned long address, bits, skey; - if (!mm_use_skey(mm) || pte_val(pte) & _PAGE_INVALID) + if (!mm_uses_skeys(mm) || pte_val(pte) & _PAGE_INVALID) return pgste; address = pte_val(pte) & PAGE_MASK; skey = (unsigned long) page_get_storage_key(address); @@ -180,7 +180,7 @@ static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry, unsigned long address; unsigned long nkey; - if (!mm_use_skey(mm) || pte_val(entry) & _PAGE_INVALID) + if (!mm_uses_skeys(mm) || pte_val(entry) & _PAGE_INVALID) return; VM_BUG_ON(!(pte_val(*ptep) & _PAGE_INVALID)); address = pte_val(entry) & PAGE_MASK; -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control 2018-02-16 11:16 ` [PATCH 2/2] KVM: s390: Add storage key facility interpretation control Janosch Frank @ 2018-02-16 14:30 ` David Hildenbrand 2018-02-16 14:35 ` Janosch Frank 0 siblings, 1 reply; 5+ messages in thread From: David Hildenbrand @ 2018-02-16 14:30 UTC (permalink / raw) To: Janosch Frank, kvm; +Cc: schwidefsky, borntraeger, alifm, imbrenda, linux-s390 On 16.02.2018 12:16, Janosch Frank wrote: > Up to now we always expected to have the storage key facility > available for our (non-VSIE) KVM guests. For huge page support, we > need to be able to disable it, so let's introduce that now. > > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/include/asm/mmu.h | 2 +- > arch/s390/include/asm/mmu_context.h | 2 +- > arch/s390/include/asm/pgtable.h | 4 ++-- > arch/s390/kvm/kvm-s390.c | 3 ++- > arch/s390/kvm/priv.c | 22 +++++++++++++--------- > arch/s390/mm/gmap.c | 6 +++--- > arch/s390/mm/pgtable.c | 4 ++-- > 8 files changed, 25 insertions(+), 19 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 27918b1..f161ad0 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -793,6 +793,7 @@ struct kvm_arch{ > int use_irqchip; > int use_cmma; > int use_pfmfi; > + int use_skf; > int user_cpu_state_ctrl; > int user_sigp; > int user_stsi; > diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h > index c639c95..f5ff9db 100644 > --- a/arch/s390/include/asm/mmu.h > +++ b/arch/s390/include/asm/mmu.h > @@ -21,7 +21,7 @@ typedef struct { > /* The mmu context uses extended page tables. */ > unsigned int has_pgste:1; > /* The mmu context uses storage keys. */ > - unsigned int use_skey:1; > + unsigned int uses_skeys:1; > /* The mmu context uses CMM. */ > unsigned int uses_cmm:1; > } mm_context_t; > diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h > index d3ebfa8..bc9a2a9 100644 > --- a/arch/s390/include/asm/mmu_context.h > +++ b/arch/s390/include/asm/mmu_context.h > @@ -30,7 +30,7 @@ static inline int init_new_context(struct task_struct *tsk, > test_thread_flag(TIF_PGSTE) || > (current->mm && current->mm->context.alloc_pgste); > mm->context.has_pgste = 0; > - mm->context.use_skey = 0; > + mm->context.uses_skeys = 0; > mm->context.uses_cmm = 0; > #endif > switch (mm->context.asce_limit) { > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > index 9223b4d..4f26425 100644 > --- a/arch/s390/include/asm/pgtable.h > +++ b/arch/s390/include/asm/pgtable.h > @@ -509,10 +509,10 @@ static inline int mm_alloc_pgste(struct mm_struct *mm) > * faults should no longer be backed by zero pages > */ > #define mm_forbids_zeropage mm_has_pgste > -static inline int mm_use_skey(struct mm_struct *mm) > +static inline int mm_uses_skeys(struct mm_struct *mm) > { > #ifdef CONFIG_PGSTE > - if (mm->context.use_skey) > + if (mm->context.uses_skeys) > return 1; > #endif > return 0; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 8fb6549..90deb7b 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1432,7 +1432,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) > return -EINVAL; > > /* Is this guest using storage keys? */ > - if (!mm_use_skey(current->mm)) > + if (!mm_uses_skeys(current->mm)) > return KVM_S390_GET_SKEYS_NONE; > > /* Enforce sane limit on memory allocation */ > @@ -2010,6 +2010,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.css_support = 0; > kvm->arch.use_irqchip = 0; > kvm->arch.use_pfmfi = sclp.has_pfmfi; > + kvm->arch.use_skf = sclp.has_skey; > kvm->arch.epoch = 0; > > spin_lock_init(&kvm->arch.start_stop_lock); > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 76a2380..d9bd147 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) > struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block; > > trace_kvm_s390_skey_related_inst(vcpu); > - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && > + /* Already enabled? */ > + if (vcpu->kvm->arch.use_skf && > + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && > !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) > return rc; While at it, can you directly "return 0;" here and remove the initialization of rc to 0? Makes the code easier to read > > rc = s390_enable_skey(); > VCPU_EVENT(vcpu, 3, "enabling storage keys for guest: %d", rc); > - if (!rc) { > - if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) > - kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); > - else > - sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | > - ICTL_RRBE); > - } > + if (rc) > + return rc; > + > + if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) > + kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); > + if (!vcpu->kvm->arch.use_skf) > + sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; > + else > + sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); I wonder why vcpu->arch.sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; Is set conditionally (sclp.has_kss) in kvm_arch_vcpu_setup(). Can't we simply always set these bits there and only clear them here conditionally? > return rc; > } > > @@ -231,7 +235,7 @@ static int try_handle_skey(struct kvm_vcpu *vcpu) > rc = kvm_s390_skey_check_enable(vcpu); > if (rc) > return rc; > - if (sclp.has_skey) { > + if (vcpu->kvm->arch.use_skf) { > /* with storage-key facility, SIE interprets it for us */ > kvm_s390_retry_instr(vcpu); > VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation"); > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 2cafcba..dbdcd25 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -3094,14 +3094,14 @@ int s390_enable_skey(void) > int rc = 0; > > down_write(&mm->mmap_sem); > - if (mm_use_skey(mm)) > + if (mm_uses_skeys(mm)) > goto out_up; > > - mm->context.use_skey = 1; > + mm->context.uses_skeys = 1; > for (vma = mm->mmap; vma; vma = vma->vm_next) { > if (ksm_madvise(vma, vma->vm_start, vma->vm_end, > MADV_UNMERGEABLE, &vma->vm_flags)) { > - mm->context.use_skey = 0; > + mm->context.uses_skeys = 0; > rc = -ENOMEM; > goto out_up; > } > diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c > index 871fc65..158c880 100644 > --- a/arch/s390/mm/pgtable.c > +++ b/arch/s390/mm/pgtable.c > @@ -158,7 +158,7 @@ static inline pgste_t pgste_update_all(pte_t pte, pgste_t pgste, > #ifdef CONFIG_PGSTE > unsigned long address, bits, skey; > > - if (!mm_use_skey(mm) || pte_val(pte) & _PAGE_INVALID) > + if (!mm_uses_skeys(mm) || pte_val(pte) & _PAGE_INVALID) > return pgste; > address = pte_val(pte) & PAGE_MASK; > skey = (unsigned long) page_get_storage_key(address); > @@ -180,7 +180,7 @@ static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry, > unsigned long address; > unsigned long nkey; > > - if (!mm_use_skey(mm) || pte_val(entry) & _PAGE_INVALID) > + if (!mm_uses_skeys(mm) || pte_val(entry) & _PAGE_INVALID) > return; > VM_BUG_ON(!(pte_val(*ptep) & _PAGE_INVALID)); > address = pte_val(entry) & PAGE_MASK; > -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control 2018-02-16 14:30 ` David Hildenbrand @ 2018-02-16 14:35 ` Janosch Frank 2018-02-16 14:46 ` David Hildenbrand 0 siblings, 1 reply; 5+ messages in thread From: Janosch Frank @ 2018-02-16 14:35 UTC (permalink / raw) To: David Hildenbrand, kvm Cc: schwidefsky, borntraeger, alifm, imbrenda, linux-s390 [-- Attachment #1.1: Type: text/plain, Size: 7735 bytes --] On 16.02.2018 15:30, David Hildenbrand wrote: > On 16.02.2018 12:16, Janosch Frank wrote: >> Up to now we always expected to have the storage key facility >> available for our (non-VSIE) KVM guests. For huge page support, we >> need to be able to disable it, so let's introduce that now. >> >> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/include/asm/mmu.h | 2 +- >> arch/s390/include/asm/mmu_context.h | 2 +- >> arch/s390/include/asm/pgtable.h | 4 ++-- >> arch/s390/kvm/kvm-s390.c | 3 ++- >> arch/s390/kvm/priv.c | 22 +++++++++++++--------- >> arch/s390/mm/gmap.c | 6 +++--- >> arch/s390/mm/pgtable.c | 4 ++-- >> 8 files changed, 25 insertions(+), 19 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 27918b1..f161ad0 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -793,6 +793,7 @@ struct kvm_arch{ >> int use_irqchip; >> int use_cmma; >> int use_pfmfi; >> + int use_skf; >> int user_cpu_state_ctrl; >> int user_sigp; >> int user_stsi; >> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h >> index c639c95..f5ff9db 100644 >> --- a/arch/s390/include/asm/mmu.h >> +++ b/arch/s390/include/asm/mmu.h >> @@ -21,7 +21,7 @@ typedef struct { >> /* The mmu context uses extended page tables. */ >> unsigned int has_pgste:1; >> /* The mmu context uses storage keys. */ >> - unsigned int use_skey:1; >> + unsigned int uses_skeys:1; >> /* The mmu context uses CMM. */ >> unsigned int uses_cmm:1; >> } mm_context_t; >> diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h >> index d3ebfa8..bc9a2a9 100644 >> --- a/arch/s390/include/asm/mmu_context.h >> +++ b/arch/s390/include/asm/mmu_context.h >> @@ -30,7 +30,7 @@ static inline int init_new_context(struct task_struct *tsk, >> test_thread_flag(TIF_PGSTE) || >> (current->mm && current->mm->context.alloc_pgste); >> mm->context.has_pgste = 0; >> - mm->context.use_skey = 0; >> + mm->context.uses_skeys = 0; >> mm->context.uses_cmm = 0; >> #endif >> switch (mm->context.asce_limit) { >> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h >> index 9223b4d..4f26425 100644 >> --- a/arch/s390/include/asm/pgtable.h >> +++ b/arch/s390/include/asm/pgtable.h >> @@ -509,10 +509,10 @@ static inline int mm_alloc_pgste(struct mm_struct *mm) >> * faults should no longer be backed by zero pages >> */ >> #define mm_forbids_zeropage mm_has_pgste >> -static inline int mm_use_skey(struct mm_struct *mm) >> +static inline int mm_uses_skeys(struct mm_struct *mm) >> { >> #ifdef CONFIG_PGSTE >> - if (mm->context.use_skey) >> + if (mm->context.uses_skeys) >> return 1; >> #endif >> return 0; >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 8fb6549..90deb7b 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -1432,7 +1432,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) >> return -EINVAL; >> >> /* Is this guest using storage keys? */ >> - if (!mm_use_skey(current->mm)) >> + if (!mm_uses_skeys(current->mm)) >> return KVM_S390_GET_SKEYS_NONE; >> >> /* Enforce sane limit on memory allocation */ >> @@ -2010,6 +2010,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> kvm->arch.css_support = 0; >> kvm->arch.use_irqchip = 0; >> kvm->arch.use_pfmfi = sclp.has_pfmfi; >> + kvm->arch.use_skf = sclp.has_skey; >> kvm->arch.epoch = 0; >> >> spin_lock_init(&kvm->arch.start_stop_lock); >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index 76a2380..d9bd147 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >> struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block; >> >> trace_kvm_s390_skey_related_inst(vcpu); >> - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> + /* Already enabled? */ >> + if (vcpu->kvm->arch.use_skf && >> + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >> !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> return rc; > > While at it, can you directly "return 0;" here and remove the > initialization of rc to 0? Makes the code easier to read Sure > >> >> rc = s390_enable_skey(); >> VCPU_EVENT(vcpu, 3, "enabling storage keys for guest: %d", rc); >> - if (!rc) { >> - if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> - kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); >> - else >> - sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | >> - ICTL_RRBE); >> - } >> + if (rc) >> + return rc; >> + >> + if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >> + kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); >> + if (!vcpu->kvm->arch.use_skf) >> + sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; >> + else >> + sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); > > > I wonder why > > vcpu->arch.sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; > > Is set conditionally (sclp.has_kss) in kvm_arch_vcpu_setup(). > > Can't we simply always set these bits there and only clear them here > conditionally? Intercept priority... skey intercepts are more important than kss. > > >> return rc; >> } >> >> @@ -231,7 +235,7 @@ static int try_handle_skey(struct kvm_vcpu *vcpu) >> rc = kvm_s390_skey_check_enable(vcpu); >> if (rc) >> return rc; >> - if (sclp.has_skey) { >> + if (vcpu->kvm->arch.use_skf) { >> /* with storage-key facility, SIE interprets it for us */ >> kvm_s390_retry_instr(vcpu); >> VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation"); >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index 2cafcba..dbdcd25 100644 >> --- a/arch/s390/mm/gmap.c >> +++ b/arch/s390/mm/gmap.c >> @@ -3094,14 +3094,14 @@ int s390_enable_skey(void) >> int rc = 0; >> >> down_write(&mm->mmap_sem); >> - if (mm_use_skey(mm)) >> + if (mm_uses_skeys(mm)) >> goto out_up; >> >> - mm->context.use_skey = 1; >> + mm->context.uses_skeys = 1; >> for (vma = mm->mmap; vma; vma = vma->vm_next) { >> if (ksm_madvise(vma, vma->vm_start, vma->vm_end, >> MADV_UNMERGEABLE, &vma->vm_flags)) { >> - mm->context.use_skey = 0; >> + mm->context.uses_skeys = 0; >> rc = -ENOMEM; >> goto out_up; >> } >> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c >> index 871fc65..158c880 100644 >> --- a/arch/s390/mm/pgtable.c >> +++ b/arch/s390/mm/pgtable.c >> @@ -158,7 +158,7 @@ static inline pgste_t pgste_update_all(pte_t pte, pgste_t pgste, >> #ifdef CONFIG_PGSTE >> unsigned long address, bits, skey; >> >> - if (!mm_use_skey(mm) || pte_val(pte) & _PAGE_INVALID) >> + if (!mm_uses_skeys(mm) || pte_val(pte) & _PAGE_INVALID) >> return pgste; >> address = pte_val(pte) & PAGE_MASK; >> skey = (unsigned long) page_get_storage_key(address); >> @@ -180,7 +180,7 @@ static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry, >> unsigned long address; >> unsigned long nkey; >> >> - if (!mm_use_skey(mm) || pte_val(entry) & _PAGE_INVALID) >> + if (!mm_uses_skeys(mm) || pte_val(entry) & _PAGE_INVALID) >> return; >> VM_BUG_ON(!(pte_val(*ptep) & _PAGE_INVALID)); >> address = pte_val(entry) & PAGE_MASK; >> > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control 2018-02-16 14:35 ` Janosch Frank @ 2018-02-16 14:46 ` David Hildenbrand 0 siblings, 0 replies; 5+ messages in thread From: David Hildenbrand @ 2018-02-16 14:46 UTC (permalink / raw) To: Janosch Frank, kvm; +Cc: schwidefsky, borntraeger, alifm, imbrenda, linux-s390 On 16.02.2018 15:35, Janosch Frank wrote: > On 16.02.2018 15:30, David Hildenbrand wrote: >> On 16.02.2018 12:16, Janosch Frank wrote: >>> Up to now we always expected to have the storage key facility >>> available for our (non-VSIE) KVM guests. For huge page support, we >>> need to be able to disable it, so let's introduce that now. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >>> Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com> >>> --- >>> arch/s390/include/asm/kvm_host.h | 1 + >>> arch/s390/include/asm/mmu.h | 2 +- >>> arch/s390/include/asm/mmu_context.h | 2 +- >>> arch/s390/include/asm/pgtable.h | 4 ++-- >>> arch/s390/kvm/kvm-s390.c | 3 ++- >>> arch/s390/kvm/priv.c | 22 +++++++++++++--------- >>> arch/s390/mm/gmap.c | 6 +++--- >>> arch/s390/mm/pgtable.c | 4 ++-- >>> 8 files changed, 25 insertions(+), 19 deletions(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 27918b1..f161ad0 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -793,6 +793,7 @@ struct kvm_arch{ >>> int use_irqchip; >>> int use_cmma; >>> int use_pfmfi; >>> + int use_skf; >>> int user_cpu_state_ctrl; >>> int user_sigp; >>> int user_stsi; >>> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h >>> index c639c95..f5ff9db 100644 >>> --- a/arch/s390/include/asm/mmu.h >>> +++ b/arch/s390/include/asm/mmu.h >>> @@ -21,7 +21,7 @@ typedef struct { >>> /* The mmu context uses extended page tables. */ >>> unsigned int has_pgste:1; >>> /* The mmu context uses storage keys. */ >>> - unsigned int use_skey:1; >>> + unsigned int uses_skeys:1; >>> /* The mmu context uses CMM. */ >>> unsigned int uses_cmm:1; >>> } mm_context_t; >>> diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h >>> index d3ebfa8..bc9a2a9 100644 >>> --- a/arch/s390/include/asm/mmu_context.h >>> +++ b/arch/s390/include/asm/mmu_context.h >>> @@ -30,7 +30,7 @@ static inline int init_new_context(struct task_struct *tsk, >>> test_thread_flag(TIF_PGSTE) || >>> (current->mm && current->mm->context.alloc_pgste); >>> mm->context.has_pgste = 0; >>> - mm->context.use_skey = 0; >>> + mm->context.uses_skeys = 0; >>> mm->context.uses_cmm = 0; >>> #endif >>> switch (mm->context.asce_limit) { >>> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h >>> index 9223b4d..4f26425 100644 >>> --- a/arch/s390/include/asm/pgtable.h >>> +++ b/arch/s390/include/asm/pgtable.h >>> @@ -509,10 +509,10 @@ static inline int mm_alloc_pgste(struct mm_struct *mm) >>> * faults should no longer be backed by zero pages >>> */ >>> #define mm_forbids_zeropage mm_has_pgste >>> -static inline int mm_use_skey(struct mm_struct *mm) >>> +static inline int mm_uses_skeys(struct mm_struct *mm) >>> { >>> #ifdef CONFIG_PGSTE >>> - if (mm->context.use_skey) >>> + if (mm->context.uses_skeys) >>> return 1; >>> #endif >>> return 0; >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 8fb6549..90deb7b 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -1432,7 +1432,7 @@ static long kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) >>> return -EINVAL; >>> >>> /* Is this guest using storage keys? */ >>> - if (!mm_use_skey(current->mm)) >>> + if (!mm_uses_skeys(current->mm)) >>> return KVM_S390_GET_SKEYS_NONE; >>> >>> /* Enforce sane limit on memory allocation */ >>> @@ -2010,6 +2010,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >>> kvm->arch.css_support = 0; >>> kvm->arch.use_irqchip = 0; >>> kvm->arch.use_pfmfi = sclp.has_pfmfi; >>> + kvm->arch.use_skf = sclp.has_skey; >>> kvm->arch.epoch = 0; >>> >>> spin_lock_init(&kvm->arch.start_stop_lock); >>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>> index 76a2380..d9bd147 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -208,19 +208,23 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu) >>> struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block; >>> >>> trace_kvm_s390_skey_related_inst(vcpu); >>> - if (!(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >>> + /* Already enabled? */ >>> + if (vcpu->kvm->arch.use_skf && >>> + !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) && >>> !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >>> return rc; >> >> While at it, can you directly "return 0;" here and remove the >> initialization of rc to 0? Makes the code easier to read > > Sure > >> >>> >>> rc = s390_enable_skey(); >>> VCPU_EVENT(vcpu, 3, "enabling storage keys for guest: %d", rc); >>> - if (!rc) { >>> - if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >>> - kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); >>> - else >>> - sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | >>> - ICTL_RRBE); >>> - } >>> + if (rc) >>> + return rc; >>> + >>> + if (kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS)) >>> + kvm_s390_clear_cpuflags(vcpu, CPUSTAT_KSS); >>> + if (!vcpu->kvm->arch.use_skf) >>> + sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; >>> + else >>> + sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE); >> >> >> I wonder why >> >> vcpu->arch.sie_block->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE; >> >> Is set conditionally (sclp.has_kss) in kvm_arch_vcpu_setup(). >> >> Can't we simply always set these bits there and only clear them here >> conditionally? > > Intercept priority... skey intercepts are more important than kss. > ... but does that make any difference here? -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-27 12:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com>
2018-02-27 12:47 ` [PATCH 2/2] KVM: s390: Add storage key facility interpretation control Janosch Frank
2018-02-16 11:16 [PATCH 0/2] KVM: s390: Refactor host interpretation facilities controls Janosch Frank
2018-02-16 11:16 ` [PATCH 2/2] KVM: s390: Add storage key facility interpretation control Janosch Frank
2018-02-16 14:30 ` David Hildenbrand
2018-02-16 14:35 ` Janosch Frank
2018-02-16 14:46 ` David Hildenbrand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).