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

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

[Janosch Frank]

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;
 
 	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);
 	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;
-- 
2.7.4

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

[David Hildenbrand]

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

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


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


-- 

Thanks,

David / dhildenb

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

[Janosch Frank]

[-- 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 --]

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

[David Hildenbrand]

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

... but does that make any difference here?


-- 

Thanks,

David / dhildenb

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

[Janosch Frank]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="--9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m", Size: 3526 bytes --]

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m
Content-Type: multipart/mixed; boundary="YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj";
 protected-headers="v1"
From: Janosch Frank <frankja@linux.vnet.ibm.com>
To: David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org
Cc: schwidefsky@de.ibm.com, borntraeger@de.ibm.com, alifm@linux.vnet.ibm.com,
 imbrenda@linux.vnet.ibm.com, linux-s390@vger.kernel.org
Message-ID: <52697ed8-f1d0-b7e1-3e56-00d89c28f633@linux.vnet.ibm.com>
Subject: Re: [PATCH 2/2] KVM: s390: Add storage key facility interpretation
 control
References: <1518779775-256056-1-git-send-email-frankja@linux.vnet.ibm.com>
 <1518779775-256056-3-git-send-email-frankja@linux.vnet.ibm.com>
 <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com>
In-Reply-To: <36e734a6-77ce-2e91-f15b-00772aaa04cc@redhat.com>

--YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

On 27.02.2018 10:40, David Hildenbrand wrote:
>=20
>>  	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 =3D vcpu->arch.sie_block;
>> =20
>>  	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;
>=20
> Wonder if it is nicer to simply remember for each CPU if this function
> has already been called. This way we can avoid calling
> s390_enable_skey() in all configurations.
>=20

With the benefit being, that on a per cpu storage we wouldn't have to
lock the call indication, as we are the thread of this vcpu and only we
can alter it? Which would speed up hpage and VSIE cases a tad, as the
down_write is quite heavy. We could also do a down_read, store
enablement status, up_read, if !enabled -> 390_enable_skey().

I'm also wondering, if we have a clean way after migrating a skey using
guest to disable interception right away when we get the skeys from
QEMU. I don't think we do that yet.


--YcUYSQ8r0698rGTYZOOtwsyBKf6Ov4Kkj--

--9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJalVN4AAoJEBcO/8Q8ZEV5HkQQALmhUssaNJ/mwJilQA8ArIRg
0xiYhOmTzYQcVPYQa2sRZfgbBnuhM//5JPvkXM4aMrucywqgEj1O1arGhZiH3dH7
8EGN7+evOyVLB125CjG5N9tmSkROe3ilRbBa3aTPeK9ZwGp/EkzcbF834rDV8dMl
dLjH7kgD9NgQIZ2SUP45TEPOzhk3VXahlTsJ44SsXBb1G5bNjW+fN7gWEmni4Kas
dwWcnnbSY4sFuGZFzcc/ls0h3oPo2eGln/ssvct8qjEeL31rljzoXuYYeboLT2hr
MTowGcaRQvjCZk9t8XtoKoRPmL2os6HCvNbAucjzDqLJetSoBR2PlCsv7gnEVJFC
X9FcCbYj3G18mxpGgy40tVZbdoY0mGc9NipKdkDvGm8DZPzQnBL3VQOtyo7WI1AI
90VG/H86zg3VstgbyhZiN/RMzP/e1j+Cv9UlHOxugSPaqgID49gmKhCy+HB0b7Oi
VU5ImaiSXvH7OFcZ5JKytbpJrFRX90+E3qIS9GPDSQzj4WAstZDDn7pp2G9xy/MF
u7UuxwfMa0iWAJWnHR8lOu/+B8c2tFAeZMivfsombPbundZzEmDTq0kwVt7jsn4S
kUWH2eYrl/tAJcOmfiiSUcmWC0jsLcohusS0TWgbvzDILFT0k2aGDTO2Gfv5nX49
hdtsHtgvNq6GEi3RDKQD
=RlCV
-----END PGP SIGNATURE-----

--9WGouaep2QQHsnPkuBQDAmTxCm8JKiD0m--