On 19.02.2018 12:16, Christian Borntraeger wrote: > > > On 02/16/2018 03:46 PM, David Hildenbrand wrote: >> 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? > > The priority makes a difference for vsie. I think it makes sense to > keep this is sync (e.g. if everybody has kss, really use kss and not > the ictl variants). > Yes, it matters for vSIE (ICPT_KSS over ICPT_INST). Simply always setting the icpt controls if !SKF makes it less error prone. Not sure if it makes sense to keep this in sync here - the priority handling really is just a way to get vSIE priorities right. For !vSIE code we don't care at all. -- 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