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: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



[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