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

[Janosch Frank <frankja@linux.vnet.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7735 bytes --]

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@linux.vnet.ibm.com>
>> Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com>
>> ---
>>  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.

> 
> 
>>  	return rc;
>>  }
>>  
>> @@ -231,7 +235,7 @@ static int try_handle_skey(struct kvm_vcpu *vcpu)
>>  	rc = kvm_s390_skey_check_enable(vcpu);
>>  	if (rc)
>>  		return rc;
>> -	if (sclp.has_skey) {
>> +	if (vcpu->kvm->arch.use_skf) {
>>  		/* with storage-key facility, SIE interprets it for us */
>>  		kvm_s390_retry_instr(vcpu);
>>  		VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation");
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 2cafcba..dbdcd25 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -3094,14 +3094,14 @@ int s390_enable_skey(void)
>>  	int rc = 0;
>>  
>>  	down_write(&mm->mmap_sem);
>> -	if (mm_use_skey(mm))
>> +	if (mm_uses_skeys(mm))
>>  		goto out_up;
>>  
>> -	mm->context.use_skey = 1;
>> +	mm->context.uses_skeys = 1;
>>  	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>  		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>  				MADV_UNMERGEABLE, &vma->vm_flags)) {
>> -			mm->context.use_skey = 0;
>> +			mm->context.uses_skeys = 0;
>>  			rc = -ENOMEM;
>>  			goto out_up;
>>  		}
>> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
>> index 871fc65..158c880 100644
>> --- a/arch/s390/mm/pgtable.c
>> +++ b/arch/s390/mm/pgtable.c
>> @@ -158,7 +158,7 @@ static inline pgste_t pgste_update_all(pte_t pte, pgste_t pgste,
>>  #ifdef CONFIG_PGSTE
>>  	unsigned long address, bits, skey;
>>  
>> -	if (!mm_use_skey(mm) || pte_val(pte) & _PAGE_INVALID)
>> +	if (!mm_uses_skeys(mm) || pte_val(pte) & _PAGE_INVALID)
>>  		return pgste;
>>  	address = pte_val(pte) & PAGE_MASK;
>>  	skey = (unsigned long) page_get_storage_key(address);
>> @@ -180,7 +180,7 @@ static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry,
>>  	unsigned long address;
>>  	unsigned long nkey;
>>  
>> -	if (!mm_use_skey(mm) || pte_val(entry) & _PAGE_INVALID)
>> +	if (!mm_uses_skeys(mm) || pte_val(entry) & _PAGE_INVALID)
>>  		return;
>>  	VM_BUG_ON(!(pte_val(*ptep) & _PAGE_INVALID));
>>  	address = pte_val(entry) & PAGE_MASK;
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]