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@xxxxxxxxxxxxxxxxxx> >>> Reviewed-by: Farhan Ali <alifm@xxxxxxxxxxxxxxxxxx> >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html