[RFC,29/37] KVM: s390: protvirt: Sync pv state
diff mbox series

Message ID 20191024114059.102802-30-frankja@linux.ibm.com
State New
Headers show
Series
  • KVM: s390: Add support for protected VMs
Related show

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC
Indicate via register sync if the VM is in secure mode.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/uapi/asm/kvm.h | 5 ++++-
 arch/s390/kvm/kvm-s390.c         | 7 ++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Thomas Huth Nov. 15, 2019, 9:36 a.m. UTC | #1
On 24/10/2019 13.40, Janosch Frank wrote:
> Indicate via register sync if the VM is in secure mode.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/include/uapi/asm/kvm.h | 5 ++++-
>  arch/s390/kvm/kvm-s390.c         | 7 ++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 436ec7636927..b44c02426c2e 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
>  #define KVM_SYNC_GSCB   (1UL << 9)
>  #define KVM_SYNC_BPBC   (1UL << 10)
>  #define KVM_SYNC_ETOKEN (1UL << 11)
> +#define KVM_SYNC_PV	(1UL << 12)
>  
>  #define KVM_SYNC_S390_VALID_FIELDS \
>  	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>  	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
> -	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
> +	 KVM_SYNC_PV)
>  
>  /* length and alignment of the sdnx as a power of two */
>  #define SDNXC 8
> @@ -261,6 +263,7 @@ struct kvm_sync_regs {
>  	__u8  reserved[512];	/* for future vector expansion */
>  	__u32 fpc;		/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */
>  	__u8 bpbc : 1;		/* bp mode */
> +	__u8 pv : 1;		/* pv mode */
>  	__u8 reserved2 : 7;

Don't you want to decrease the reserved2 bits to 6 ? ...

>  	__u8 padding1[51];	/* riccb needs to be 64byte aligned */

... otherwise you might mess up the alignment here!

>  	__u8 riccb[64];		/* runtime instrumentation controls block */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index f623c64aeade..500972a1f742 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2856,6 +2856,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_GSCB;
>  	if (test_kvm_facility(vcpu->kvm, 156))
>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_ETOKEN;
> +	if (test_kvm_facility(vcpu->kvm, 161))
> +		vcpu->run->kvm_valid_regs |= KVM_SYNC_PV;
>  	/* fprs can be synchronized via vrs, even if the guest has no vx. With
>  	 * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format.
>  	 */
> @@ -4136,6 +4138,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  {
>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
> +	kvm_run->s.regs.pv = 0;
>  	if (MACHINE_HAS_GS) {
>  		__ctl_set_bit(2, 4);
>  		if (vcpu->arch.gs_enabled)
> @@ -4172,8 +4175,10 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	/* Restore will be done lazily at return */
>  	current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc;
>  	current->thread.fpu.regs = vcpu->arch.host_fpregs.regs;
> -	if (likely(!kvm_s390_pv_is_protected(vcpu->kvm)))
> +	if (likely(!kvm_s390_pv_handle_cpu(vcpu)))

Why change the if-statement now? Should this maybe rather be squashed
into the patch that introduced the if-statement?

>  		store_regs_fmt2(vcpu, kvm_run);
> +	else
> +		kvm_run->s.regs.pv = 1;
>  }
>  
>  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> 

 Thomas
Janosch Frank Nov. 15, 2019, 9:59 a.m. UTC | #2
On 11/15/19 10:36 AM, Thomas Huth wrote:
> On 24/10/2019 13.40, Janosch Frank wrote:
>> Indicate via register sync if the VM is in secure mode.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/include/uapi/asm/kvm.h | 5 ++++-
>>  arch/s390/kvm/kvm-s390.c         | 7 ++++++-
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 436ec7636927..b44c02426c2e 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>> +#define KVM_SYNC_PV	(1UL << 12)
>>  
>>  #define KVM_SYNC_S390_VALID_FIELDS \
>>  	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>  	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>> -	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
>> +	 KVM_SYNC_PV)
>>  
>>  /* length and alignment of the sdnx as a power of two */
>>  #define SDNXC 8
>> @@ -261,6 +263,7 @@ struct kvm_sync_regs {
>>  	__u8  reserved[512];	/* for future vector expansion */
>>  	__u32 fpc;		/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */
>>  	__u8 bpbc : 1;		/* bp mode */
>> +	__u8 pv : 1;		/* pv mode */
>>  	__u8 reserved2 : 7;
> 
> Don't you want to decrease the reserved2 bits to 6 ? ...

Ups

> 
>>  	__u8 padding1[51];	/* riccb needs to be 64byte aligned */
> 
> ... otherwise you might mess up the alignment here!
> 
>>  	__u8 riccb[64];		/* runtime instrumentation controls block */
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index f623c64aeade..500972a1f742 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2856,6 +2856,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_GSCB;
>>  	if (test_kvm_facility(vcpu->kvm, 156))
>>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_ETOKEN;
>> +	if (test_kvm_facility(vcpu->kvm, 161))
>> +		vcpu->run->kvm_valid_regs |= KVM_SYNC_PV;
>>  	/* fprs can be synchronized via vrs, even if the guest has no vx. With
>>  	 * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format.
>>  	 */
>> @@ -4136,6 +4138,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  {
>>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>> +	kvm_run->s.regs.pv = 0;
>>  	if (MACHINE_HAS_GS) {
>>  		__ctl_set_bit(2, 4);
>>  		if (vcpu->arch.gs_enabled)
>> @@ -4172,8 +4175,10 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  	/* Restore will be done lazily at return */
>>  	current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc;
>>  	current->thread.fpu.regs = vcpu->arch.host_fpregs.regs;
>> -	if (likely(!kvm_s390_pv_is_protected(vcpu->kvm)))
>> +	if (likely(!kvm_s390_pv_handle_cpu(vcpu)))
> 
> Why change the if-statement now? Should this maybe rather be squashed
> into the patch that introduced the if-statement?

That was part of a cleanup that should have been done in other patches.

> 
>>  		store_regs_fmt2(vcpu, kvm_run);
>> +	else
>> +		kvm_run->s.regs.pv = 1;
>>  }
>>  
>>  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>
> 
>  Thomas
>

Patch
diff mbox series

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 436ec7636927..b44c02426c2e 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -231,11 +231,13 @@  struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+#define KVM_SYNC_PV	(1UL << 12)
 
 #define KVM_SYNC_S390_VALID_FIELDS \
 	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
 	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
-	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
+	 KVM_SYNC_PV)
 
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
@@ -261,6 +263,7 @@  struct kvm_sync_regs {
 	__u8  reserved[512];	/* for future vector expansion */
 	__u32 fpc;		/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */
 	__u8 bpbc : 1;		/* bp mode */
+	__u8 pv : 1;		/* pv mode */
 	__u8 reserved2 : 7;
 	__u8 padding1[51];	/* riccb needs to be 64byte aligned */
 	__u8 riccb[64];		/* runtime instrumentation controls block */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f623c64aeade..500972a1f742 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2856,6 +2856,8 @@  int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 		vcpu->run->kvm_valid_regs |= KVM_SYNC_GSCB;
 	if (test_kvm_facility(vcpu->kvm, 156))
 		vcpu->run->kvm_valid_regs |= KVM_SYNC_ETOKEN;
+	if (test_kvm_facility(vcpu->kvm, 161))
+		vcpu->run->kvm_valid_regs |= KVM_SYNC_PV;
 	/* fprs can be synchronized via vrs, even if the guest has no vx. With
 	 * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format.
 	 */
@@ -4136,6 +4138,7 @@  static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
 	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
+	kvm_run->s.regs.pv = 0;
 	if (MACHINE_HAS_GS) {
 		__ctl_set_bit(2, 4);
 		if (vcpu->arch.gs_enabled)
@@ -4172,8 +4175,10 @@  static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	/* Restore will be done lazily at return */
 	current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc;
 	current->thread.fpu.regs = vcpu->arch.host_fpregs.regs;
-	if (likely(!kvm_s390_pv_is_protected(vcpu->kvm)))
+	if (likely(!kvm_s390_pv_handle_cpu(vcpu)))
 		store_regs_fmt2(vcpu, kvm_run);
+	else
+		kvm_run->s.regs.pv = 1;
 }
 
 int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)