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).
