diff mbox series

[RFCv2,26/37] KVM: s390: protvirt: disallow one_reg

Message ID 20200203131957.383915-27-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Add support for protected VMs | expand

Commit Message

Christian Borntraeger Feb. 3, 2020, 1:19 p.m. UTC 
From: Janosch Frank <frankja@linux.ibm.com>

A lot of the registers are controlled by the Ultravisor and never
visible to KVM. Some fields in the sie control block are overlayed,
like gbea. As no userspace uses the ONE_REG interface on s390 it is safe
to disable this for protected guests.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Huth Feb. 5, 2020, 12:16 p.m. UTC | #1 
On 03/02/2020 14.19, Christian Borntraeger wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> A lot of the registers are controlled by the Ultravisor and never
> visible to KVM. Some fields in the sie control block are overlayed,
> like gbea. As no userspace uses the ONE_REG interface on s390 it is safe
> to disable this for protected guests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6e74c7afae3a..b9692d722c1e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4641,6 +4641,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
>  		struct kvm_one_reg reg;
> +		r = -EINVAL;
> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
> +			break;
>  		r = -EFAULT;
>  		if (copy_from_user(&reg, argp, sizeof(reg)))
>  			break;

Reviewed-by: Thomas Huth <thuth@redhat.com>

PS:
Not sure, but maybe it would be also be good to add a sentence to
Documentation/virt/kvm/api.txt ?
Cornelia Huck Feb. 5, 2020, 2:42 p.m. UTC | #2 
On Mon,  3 Feb 2020 08:19:46 -0500
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Janosch Frank <frankja@linux.ibm.com>
> 
> A lot of the registers are controlled by the Ultravisor and never
> visible to KVM. Some fields in the sie control block are overlayed,
> like gbea. As no userspace uses the ONE_REG interface on s390 it is safe
> to disable this for protected guests.

Hm, QEMU seems to fall back to ONE_REG if sync regs are not available,
doesn't it? So maybe it would be better to word this as

"As no known userspace uses the ONE_REG interface on s390 if sync regs
are available, no functionality is lost if it is disabled for protected
guests."

?

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6e74c7afae3a..b9692d722c1e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4641,6 +4641,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
>  		struct kvm_one_reg reg;
> +		r = -EINVAL;
> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
> +			break;
>  		r = -EFAULT;
>  		if (copy_from_user(&reg, argp, sizeof(reg)))
>  			break;

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Christian Borntraeger Feb. 5, 2020, 7:25 p.m. UTC | #3 
On 05.02.20 13:16, Thomas Huth wrote:
> On 03/02/2020 14.19, Christian Borntraeger wrote:
>> From: Janosch Frank <frankja@linux.ibm.com>
>>
>> A lot of the registers are controlled by the Ultravisor and never
>> visible to KVM. Some fields in the sie control block are overlayed,
>> like gbea. As no userspace uses the ONE_REG interface on s390 it is safe
>> to disable this for protected guests.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 6e74c7afae3a..b9692d722c1e 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -4641,6 +4641,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  	case KVM_SET_ONE_REG:
>>  	case KVM_GET_ONE_REG: {
>>  		struct kvm_one_reg reg;
>> +		r = -EINVAL;
>> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
>> +			break;
>>  		r = -EFAULT;
>>  		if (copy_from_user(&reg, argp, sizeof(reg)))
>>  			break;
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> PS:
> Not sure, but maybe it would be also be good to add a sentence to
> Documentation/virt/kvm/api.txt ?


Ack:

diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt
index 4874d42286ca..8239b3665736 100644
--- a/Documentation/virt/kvm/api.txt
+++ b/Documentation/virt/kvm/api.txt
@@ -1918,7 +1918,8 @@ Parameters: struct kvm_one_reg (in)
 Returns: 0 on success, negative value on failure
 Errors:
   ENOENT:   no such register
-  EINVAL:   invalid register ID, or no such register
+  EINVAL:   invalid register ID, or no such register, ONE_REG forbidden
+            for protected guests (s390).
   EPERM:    (arm64) register access not allowed before vcpu finalization
 (These error codes are indicative only: do not rely on a specific error
 code being returned in a specific situation.)
@@ -2311,7 +2312,8 @@ Parameters: struct kvm_one_reg (in and out)
 Returns: 0 on success, negative value on failure
 Errors include:
   ENOENT:   no such register
-  EINVAL:   invalid register ID, or no such register
+  EINVAL:   invalid register ID, or no such register,ONE_REG forbidden
+            for protected guests (s390)
   EPERM:    (arm64) register access not allowed before vcpu finalization
 (These error codes are indicative only: do not rely on a specific error
 code being returned in a specific situation.)
diff mbox series

Patch

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6e74c7afae3a..b9692d722c1e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4641,6 +4641,9 @@  long kvm_arch_vcpu_ioctl(struct file *filp,
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
 		struct kvm_one_reg reg;
+		r = -EINVAL;
+		if (kvm_s390_pv_is_protected(vcpu->kvm))
+			break;
 		r = -EFAULT;
 		if (copy_from_user(&reg, argp, sizeof(reg)))
 			break;