diff mbox

[v2] KVM: s390: Beautify skey enable check

Message ID 20180720141657.30152-1-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Janosch Frank July 20, 2018, 2:16 p.m. UTC 
Let's introduce an explicit check if skeys have already been enabled
for the vcpu, so we don't have to check the mm context if we don't have
the storage key facility.

This let's us check for enablement without having to take the mm
semaphore and thus speedup skey emulation.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/priv.c             | 12 ++++--------
 2 files changed, 5 insertions(+), 8 deletions(-)

Comments

David Hildenbrand July 20, 2018, 2:29 p.m. UTC | #1 
On 20.07.2018 16:16, Janosch Frank wrote:
> cpu_arch {
>  	seqcount_t cputm_seqcount;
>  	__u64 cputm_start;
>  	bool gs_enabled;
> +	bool skey_enabled;
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 83c678266588..54493c587a9e 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -205,13 +205,10 @@ static int handle_store_cpu_address(struct kvm_vcpu *vcpu)
>  int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
>  {
>  	int rc;
> -	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
>  
>  	trace_kvm_s390_skey_related_inst(vcpu);
>  	/* Already enabled? */
> -	if (vcpu->kvm->arch.use_skf &&
> -	    !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) &&
> -	    !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
> +	if (vcpu->arch.skey_enabled)
>  		return 0;
>  
>  	rc = s390_enable_skey();
> @@ -221,10 +218,9 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
>  
>  	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
> -		

Reviewed-by: David Hildenbrand <david@redhat.com>
Christian Borntraeger July 23, 2018, 12:42 p.m. UTC | #2 
Makes a lot of sense and it also improves the skey performane for the hlp patch series.
Does it make sense to let this be part of the next version of the host large pages
series or shall I take this patch directly?


On 07/20/2018 04:16 PM, Janosch Frank wrote:
> Let's introduce an explicit check if skeys have already been enabled
> for the vcpu, so we don't have to check the mm context if we don't have
> the storage key facility.
> 
> This let's us check for enablement without having to take the mm
> semaphore and thus speedup skey emulation.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Acked-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/priv.c             | 12 ++++--------
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index a2188e309bd6..2916f0a5585c 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -655,6 +655,7 @@ struct kvm_vcpu_arch {
>  	seqcount_t cputm_seqcount;
>  	__u64 cputm_start;
>  	bool gs_enabled;
> +	bool skey_enabled;
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 83c678266588..54493c587a9e 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -205,13 +205,10 @@ static int handle_store_cpu_address(struct kvm_vcpu *vcpu)
>  int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
>  {
>  	int rc;
> -	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
>  
>  	trace_kvm_s390_skey_related_inst(vcpu);
>  	/* Already enabled? */
> -	if (vcpu->kvm->arch.use_skf &&
> -	    !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) &&
> -	    !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
> +	if (vcpu->arch.skey_enabled)
>  		return 0;
>  
>  	rc = s390_enable_skey();
> @@ -221,10 +218,9 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
>  
>  	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);
> +	if (vcpu->kvm->arch.use_skf)
> +		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
> +	vcpu->arch.skey_enabled = true;
>  	return 0;
>  }
>  
>
Janosch Frank July 23, 2018, 1:06 p.m. UTC | #3 
On 23.07.2018 14:42, Christian Borntraeger wrote:
> Makes a lot of sense and it also improves the skey performane for the hlp patch series.
> Does it make sense to let this be part of the next version of the host large pages
> series or shall I take this patch directly?

I'd prefer to have it separately.

>>  	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'm not allowed to remove both lines here.
If we have kss but !skf, we'd never get any intercepts as we either set
ICTLS or KSS in the vcpu setup... I.e. hpage with KSS would be broken.


>> +	if (vcpu->kvm->arch.use_skf)
>> +		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
>> +	vcpu->arch.skey_enabled = true;
>>  	return 0;
>>  }
>>  
>>
David Hildenbrand July 23, 2018, 5:10 p.m. UTC | #4 
On 23.07.2018 15:06, Janosch Frank wrote:
> On 23.07.2018 14:42, Christian Borntraeger wrote:
>> Makes a lot of sense and it also improves the skey performane for the hlp patch series.
>> Does it make sense to let this be part of the next version of the host large pages
>> series or shall I take this patch directly?
> 
> I'd prefer to have it separately.
> 
>>>  	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'm not allowed to remove both lines here.
> If we have kss but !skf, we'd never get any intercepts as we either set
> ICTLS or KSS in the vcpu setup... I.e. hpage with KSS would be broken.
> 
> 

Right, I keep forgetting this special case. I wish I had access to this
confidential documentation.
Christian Borntraeger July 24, 2018, 7:19 a.m. UTC | #5 
On 07/23/2018 03:06 PM, Janosch Frank wrote:
> On 23.07.2018 14:42, Christian Borntraeger wrote:
>> Makes a lot of sense and it also improves the skey performane for the hlp patch series.
>> Does it make sense to let this be part of the next version of the host large pages
>> series or shall I take this patch directly?
> 
> I'd prefer to have it separately.
> 
>>>  	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'm not allowed to remove both lines here.
> If we have kss but !skf, we'd never get any intercepts as we either set
> ICTLS or KSS in the vcpu setup... I.e. hpage with KSS would be broken.

So you are going to send a v3?

> 
> 
>>> +	if (vcpu->kvm->arch.use_skf)
>>> +		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
>>> +	vcpu->arch.skey_enabled = true;
>>>  	return 0;
>>>  }
>>>  
>>>
> 
>
Janosch Frank July 24, 2018, 7:31 a.m. UTC | #6 
On 24.07.2018 09:19, Christian Borntraeger wrote:
> 
> 
> On 07/23/2018 03:06 PM, Janosch Frank wrote:
>> On 23.07.2018 14:42, Christian Borntraeger wrote:
>>> Makes a lot of sense and it also improves the skey performane for the hlp patch series.
>>> Does it make sense to let this be part of the next version of the host large pages
>>> series or shall I take this patch directly?
>>
>> I'd prefer to have it separately.
>>
>>>>  	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'm not allowed to remove both lines here.
>> If we have kss but !skf, we'd never get any intercepts as we either set
>> ICTLS or KSS in the vcpu setup... I.e. hpage with KSS would be broken.
> 
> So you are going to send a v3?

I'd just apply it with the change, if nobody has anything against that.

> 
>>
>>
>>>> +	if (vcpu->kvm->arch.use_skf)
>>>> +		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
>>>> +	vcpu->arch.skey_enabled = true;
>>>>  	return 0;
>>>>  }
>>>>  
>>>>
>>
>>
Christian Borntraeger July 24, 2018, 7:33 a.m. UTC | #7 
On 07/24/2018 09:31 AM, Janosch Frank wrote:
> On 24.07.2018 09:19, Christian Borntraeger wrote:
>>
>>
>> On 07/23/2018 03:06 PM, Janosch Frank wrote:
>>> On 23.07.2018 14:42, Christian Borntraeger wrote:
>>>> Makes a lot of sense and it also improves the skey performane for the hlp patch series.
>>>> Does it make sense to let this be part of the next version of the host large pages
>>>> series or shall I take this patch directly?
>>>
>>> I'd prefer to have it separately.
>>>
>>>>>  	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'm not allowed to remove both lines here.
>>> If we have kss but !skf, we'd never get any intercepts as we either set
>>> ICTLS or KSS in the vcpu setup... I.e. hpage with KSS would be broken.
>>
>> So you are going to send a v3?
> 
> I'd just apply it with the change, if nobody has anything against that.
Fine for me, go ahead.
> 
>>
>>>
>>>
>>>>> +	if (vcpu->kvm->arch.use_skf)
>>>>> +		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
>>>>> +	vcpu->arch.skey_enabled = true;
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>>
>>>
>>>
> 
>
diff mbox

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index a2188e309bd6..2916f0a5585c 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -655,6 +655,7 @@  struct kvm_vcpu_arch {
 	seqcount_t cputm_seqcount;
 	__u64 cputm_start;
 	bool gs_enabled;
+	bool skey_enabled;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 83c678266588..54493c587a9e 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -205,13 +205,10 @@  static int handle_store_cpu_address(struct kvm_vcpu *vcpu)
 int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
 {
 	int rc;
-	struct kvm_s390_sie_block *sie_block = vcpu->arch.sie_block;
 
 	trace_kvm_s390_skey_related_inst(vcpu);
 	/* Already enabled? */
-	if (vcpu->kvm->arch.use_skf &&
-	    !(sie_block->ictl & (ICTL_ISKE | ICTL_SSKE | ICTL_RRBE)) &&
-	    !kvm_s390_test_cpuflags(vcpu, CPUSTAT_KSS))
+	if (vcpu->arch.skey_enabled)
 		return 0;
 
 	rc = s390_enable_skey();
@@ -221,10 +218,9 @@  int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
 
 	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);
+	if (vcpu->kvm->arch.use_skf)
+		vcpu->arch.sie_block->ictl &= ~(ICTL_ISKE | ICTL_SSKE | ICTL_RRBE);
+	vcpu->arch.skey_enabled = true;
 	return 0;
 }