[RFC,33/37] KVM: s390: Introduce VCPU reset IOCTL
diff mbox series

Message ID 20191024114059.102802-34-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
With PV we need to do things for all reset types, not only initial...

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

Comments

Thomas Huth Nov. 15, 2019, 10:47 a.m. UTC | #1
On 24/10/2019 13.40, Janosch Frank wrote:
> With PV we need to do things for all reset types, not only initial...
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h |  6 +++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d3fd3ad1d09b..d8ee3a98e961 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,
> +				     unsigned long type)
> +{
> +	int rc;
> +	u32 ret;
> +
> +	switch (type) {
> +	case KVM_S390_VCPU_RESET_NORMAL:
> +		/*
> +		 * Only very little is reset, userspace handles the
> +		 * non-protected case.
> +		 */
> +		rc = 0;
> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
> +					   UVC_CMD_CPU_RESET, &ret);
> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x",
> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
> +		}
> +		break;
> +	case KVM_S390_VCPU_RESET_INITIAL:
> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
> +			uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
> +				      UVC_CMD_CPU_RESET_INITIAL,
> +				      &ret);
> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x",
> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
> +		}
> +		break;
> +	case KVM_S390_VCPU_RESET_CLEAR:
> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
> +					   UVC_CMD_CPU_RESET_CLEAR, &ret);
> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x",
> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
> +		}
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;

(nit: you could drop the "break;" here)

> +	}
> +	return rc;
> +}
> +
> +
>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  {
>  	vcpu_load(vcpu);
> @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		break;
>  	}
>  	case KVM_S390_INITIAL_RESET:
> +		r = -EINVAL;
> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
> +			break;

Wouldn't it be nicer to call

  kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)

in this case instead?

>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>  		break;
> +	case KVM_S390_VCPU_RESET:
> +		r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
> +		break;
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
>  		struct kvm_one_reg reg;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f75a051a7705..2846ed5e5dd9 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1496,6 +1496,12 @@ struct kvm_pv_cmd {
>  #define KVM_S390_PV_COMMAND		_IOW(KVMIO, 0xc3, struct kvm_pv_cmd)
>  #define KVM_S390_PV_COMMAND_VCPU	_IOW(KVMIO, 0xc4, struct kvm_pv_cmd)
>  
> +#define KVM_S390_VCPU_RESET_NORMAL	0
> +#define KVM_S390_VCPU_RESET_INITIAL	1
> +#define KVM_S390_VCPU_RESET_CLEAR	2
> +
> +#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xd0)

Why not 0xc5 ?

 Thomas
Janosch Frank Nov. 15, 2019, 1:06 p.m. UTC | #2
On 11/15/19 11:47 AM, Thomas Huth wrote:
> On 24/10/2019 13.40, Janosch Frank wrote:
>> With PV we need to do things for all reset types, not only initial...
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h |  6 +++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d3fd3ad1d09b..d8ee3a98e961 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>  	return 0;
>>  }
>>  
>> +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,
>> +				     unsigned long type)
>> +{
>> +	int rc;
>> +	u32 ret;
>> +
>> +	switch (type) {
>> +	case KVM_S390_VCPU_RESET_NORMAL:
>> +		/*
>> +		 * Only very little is reset, userspace handles the
>> +		 * non-protected case.
>> +		 */
>> +		rc = 0;
>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>> +					   UVC_CMD_CPU_RESET, &ret);
>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x",
>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>> +		}
>> +		break;
>> +	case KVM_S390_VCPU_RESET_INITIAL:
>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>> +			uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>> +				      UVC_CMD_CPU_RESET_INITIAL,
>> +				      &ret);
>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x",
>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>> +		}
>> +		break;
>> +	case KVM_S390_VCPU_RESET_CLEAR:
>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>> +					   UVC_CMD_CPU_RESET_CLEAR, &ret);
>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x",
>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>> +		}
>> +		break;
>> +	default:
>> +		rc = -EINVAL;
>> +		break;
> 
> (nit: you could drop the "break;" here)
> 
>> +	}
>> +	return rc;
>> +}
>> +
>> +
>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>  {
>>  	vcpu_load(vcpu);
>> @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  		break;
>>  	}
>>  	case KVM_S390_INITIAL_RESET:
>> +		r = -EINVAL;
>> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
>> +			break;
> 
> Wouldn't it be nicer to call
> 
>   kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)
> 
> in this case instead?

How about:
        case KVM_S390_INITIAL_RESET:


                arg = KVM_S390_VCPU_RESET_INITIAL;


        case KVM_S390_VCPU_RESET:


                r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);


                break;



> 
>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>  		break;
>> +	case KVM_S390_VCPU_RESET:
>> +		r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
>> +		break;
>>  	case KVM_SET_ONE_REG:
>>  	case KVM_GET_ONE_REG: {
>>  		struct kvm_one_reg reg;
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index f75a051a7705..2846ed5e5dd9 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1496,6 +1496,12 @@ struct kvm_pv_cmd {
>>  #define KVM_S390_PV_COMMAND		_IOW(KVMIO, 0xc3, struct kvm_pv_cmd)
>>  #define KVM_S390_PV_COMMAND_VCPU	_IOW(KVMIO, 0xc4, struct kvm_pv_cmd)
>>  
>> +#define KVM_S390_VCPU_RESET_NORMAL	0
>> +#define KVM_S390_VCPU_RESET_INITIAL	1
>> +#define KVM_S390_VCPU_RESET_CLEAR	2
>> +
>> +#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xd0)
> 
> Why not 0xc5 ?

Fixed

> 
>  Thomas
>
Thomas Huth Nov. 15, 2019, 1:18 p.m. UTC | #3
On 15/11/2019 14.06, Janosch Frank wrote:
> On 11/15/19 11:47 AM, Thomas Huth wrote:
>> On 24/10/2019 13.40, Janosch Frank wrote:
>>> With PV we need to do things for all reset types, not only initial...
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++
>>>  include/uapi/linux/kvm.h |  6 +++++
>>>  2 files changed, 59 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index d3fd3ad1d09b..d8ee3a98e961 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>>  	return 0;
>>>  }
>>>  
>>> +static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,
>>> +				     unsigned long type)
>>> +{
>>> +	int rc;
>>> +	u32 ret;
>>> +
>>> +	switch (type) {
>>> +	case KVM_S390_VCPU_RESET_NORMAL:
>>> +		/*
>>> +		 * Only very little is reset, userspace handles the
>>> +		 * non-protected case.
>>> +		 */
>>> +		rc = 0;
>>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>>> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>>> +					   UVC_CMD_CPU_RESET, &ret);
>>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x",
>>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>>> +		}
>>> +		break;
>>> +	case KVM_S390_VCPU_RESET_INITIAL:
>>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>>> +			uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>>> +				      UVC_CMD_CPU_RESET_INITIAL,
>>> +				      &ret);
>>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x",
>>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>>> +		}
>>> +		break;
>>> +	case KVM_S390_VCPU_RESET_CLEAR:
>>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>> +		if (kvm_s390_pv_handle_cpu(vcpu)) {
>>> +			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
>>> +					   UVC_CMD_CPU_RESET_CLEAR, &ret);
>>> +			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x",
>>> +				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
>>> +		}
>>> +		break;
>>> +	default:
>>> +		rc = -EINVAL;
>>> +		break;
>>
>> (nit: you could drop the "break;" here)
>>
>>> +	}
>>> +	return rc;
>>> +}
>>> +
>>> +
>>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>  {
>>>  	vcpu_load(vcpu);
>>> @@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>>  		break;
>>>  	}
>>>  	case KVM_S390_INITIAL_RESET:
>>> +		r = -EINVAL;
>>> +		if (kvm_s390_pv_is_protected(vcpu->kvm))
>>> +			break;
>>
>> Wouldn't it be nicer to call
>>
>>   kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)
>>
>> in this case instead?
> 
> How about:
>         case KVM_S390_INITIAL_RESET:
> 
> 
>                 arg = KVM_S390_VCPU_RESET_INITIAL;
> 

Add a "/* fallthrough */" comment here...

>         case KVM_S390_VCPU_RESET:
> 
> 
>                 r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
> 
> 
>                 break;

... then this looks good, yes!

 Thomas

Patch
diff mbox series

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d3fd3ad1d09b..d8ee3a98e961 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3472,6 +3472,53 @@  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,
+				     unsigned long type)
+{
+	int rc;
+	u32 ret;
+
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		/*
+		 * Only very little is reset, userspace handles the
+		 * non-protected case.
+		 */
+		rc = 0;
+		if (kvm_s390_pv_handle_cpu(vcpu)) {
+			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
+					   UVC_CMD_CPU_RESET, &ret);
+			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x",
+				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
+		}
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		if (kvm_s390_pv_handle_cpu(vcpu)) {
+			uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
+				      UVC_CMD_CPU_RESET_INITIAL,
+				      &ret);
+			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x",
+				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
+		}
+		break;
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		if (kvm_s390_pv_handle_cpu(vcpu)) {
+			rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
+					   UVC_CMD_CPU_RESET_CLEAR, &ret);
+			VCPU_EVENT(vcpu, 3, "PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x",
+				   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
+		}
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+	return rc;
+}
+
+
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	vcpu_load(vcpu);
@@ -4633,8 +4680,14 @@  long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 	case KVM_S390_INITIAL_RESET:
+		r = -EINVAL;
+		if (kvm_s390_pv_is_protected(vcpu->kvm))
+			break;
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_VCPU_RESET:
+		r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
+		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
 		struct kvm_one_reg reg;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f75a051a7705..2846ed5e5dd9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1496,6 +1496,12 @@  struct kvm_pv_cmd {
 #define KVM_S390_PV_COMMAND		_IOW(KVMIO, 0xc3, struct kvm_pv_cmd)
 #define KVM_S390_PV_COMMAND_VCPU	_IOW(KVMIO, 0xc4, struct kvm_pv_cmd)
 
+#define KVM_S390_VCPU_RESET_NORMAL	0
+#define KVM_S390_VCPU_RESET_INITIAL	1
+#define KVM_S390_VCPU_RESET_CLEAR	2
+
+#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xd0)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */