On 19.02.2018 12:29, David Hildenbrand wrote:
> 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>
>>>>>> ---
[...]
>>>>>> /* 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.
I also would expect that KSS has some benefits for the SIE and the CPU
itself, but I don't know that for sure.
For now I'd like to keep it like I have it in this patch.
It seems like the first patch works for everyone, do you want a v2 for
this one?
Attachment:
signature.asc
Description: OpenPGP digital signature
