Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux