KVM: s390: Add new reset vcpu API
diff mbox series

Message ID 20191129142122.21528-1-frankja@linux.ibm.com
State New
Headers show
Series
  • KVM: s390: Add new reset vcpu API
Related show

Commit Message

Janosch Frank Nov. 29, 2019, 2:21 p.m. UTC
The architecture states that we need to reset local IRQs for all CPU
resets. Because the old reset interface did not support the normal CPU
reset we never did that.

Now that we have a new interface, let's properly clear out local IRQs
and let this commit be a reminder.

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

Comments

David Hildenbrand Nov. 29, 2019, 2:31 p.m. UTC | #1
On 29.11.19 15:21, Janosch Frank wrote:
> The architecture states that we need to reset local IRQs for all CPU
> resets. Because the old reset interface did not support the normal CPU
> reset we never did that.
> 
> Now that we have a new interface, let's properly clear out local IRQs
> and let this commit be a reminder.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
>  include/uapi/linux/kvm.h |  7 +++++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3d54f0..2f74ff46b176 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_CMMA_MIGRATION:
>  	case KVM_CAP_S390_AIS:
>  	case KVM_CAP_S390_AIS_MIGRATION:
> +	case KVM_CAP_S390_VCPU_RESETS:
>  		r = 1;
>  		break;
>  	case KVM_CAP_S390_HPAGE_1M:
> @@ -3293,6 +3294,25 @@ 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 = -EINVAL;
> +
> +	switch (type) {
> +	case KVM_S390_VCPU_RESET_NORMAL:
> +		rc = 0;
> +		kvm_clear_async_pf_completion_queue(vcpu);
> +		kvm_s390_clear_local_irqs(vcpu);
> +		break;
> +	case KVM_S390_VCPU_RESET_INITIAL:
> +		/* fallthrough */
> +	case KVM_S390_VCPU_RESET_CLEAR:
> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);

As we now have two interfaces to achieve the same thing (initial reset),
I do wonder if we should simply introduce

KVM_S390_NORMAL_RESET
KVM_S390_CLEAR_RESET

instead ...

Then you can do KVM_S390_NORMAL_RESET for the bugfix and
KVM_S390_CLEAR_RESET later for PV.

Does anything speak against that?
Janosch Frank Nov. 29, 2019, 2:33 p.m. UTC | #2
On 11/29/19 3:31 PM, David Hildenbrand wrote:
> On 29.11.19 15:21, Janosch Frank wrote:
>> The architecture states that we need to reset local IRQs for all CPU
>> resets. Because the old reset interface did not support the normal CPU
>> reset we never did that.
>>
>> Now that we have a new interface, let's properly clear out local IRQs
>> and let this commit be a reminder.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
>>  include/uapi/linux/kvm.h |  7 +++++++
>>  2 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d9e6bf3d54f0..2f74ff46b176 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_S390_CMMA_MIGRATION:
>>  	case KVM_CAP_S390_AIS:
>>  	case KVM_CAP_S390_AIS_MIGRATION:
>> +	case KVM_CAP_S390_VCPU_RESETS:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_S390_HPAGE_1M:
>> @@ -3293,6 +3294,25 @@ 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 = -EINVAL;
>> +
>> +	switch (type) {
>> +	case KVM_S390_VCPU_RESET_NORMAL:
>> +		rc = 0;
>> +		kvm_clear_async_pf_completion_queue(vcpu);
>> +		kvm_s390_clear_local_irqs(vcpu);
>> +		break;
>> +	case KVM_S390_VCPU_RESET_INITIAL:
>> +		/* fallthrough */
>> +	case KVM_S390_VCPU_RESET_CLEAR:
>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> 
> As we now have two interfaces to achieve the same thing (initial reset),
> I do wonder if we should simply introduce
> 
> KVM_S390_NORMAL_RESET
> KVM_S390_CLEAR_RESET
> 
> instead ...
> 
> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
> KVM_S390_CLEAR_RESET later for PV.
> 
> Does anything speak against that?

Apart from loosing one more ioctl number probably not
David Hildenbrand Nov. 29, 2019, 2:33 p.m. UTC | #3
On 29.11.19 15:33, Janosch Frank wrote:
> On 11/29/19 3:31 PM, David Hildenbrand wrote:
>> On 29.11.19 15:21, Janosch Frank wrote:
>>> The architecture states that we need to reset local IRQs for all CPU
>>> resets. Because the old reset interface did not support the normal CPU
>>> reset we never did that.
>>>
>>> Now that we have a new interface, let's properly clear out local IRQs
>>> and let this commit be a reminder.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
>>>  include/uapi/linux/kvm.h |  7 +++++++
>>>  2 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index d9e6bf3d54f0..2f74ff46b176 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>  	case KVM_CAP_S390_CMMA_MIGRATION:
>>>  	case KVM_CAP_S390_AIS:
>>>  	case KVM_CAP_S390_AIS_MIGRATION:
>>> +	case KVM_CAP_S390_VCPU_RESETS:
>>>  		r = 1;
>>>  		break;
>>>  	case KVM_CAP_S390_HPAGE_1M:
>>> @@ -3293,6 +3294,25 @@ 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 = -EINVAL;
>>> +
>>> +	switch (type) {
>>> +	case KVM_S390_VCPU_RESET_NORMAL:
>>> +		rc = 0;
>>> +		kvm_clear_async_pf_completion_queue(vcpu);
>>> +		kvm_s390_clear_local_irqs(vcpu);
>>> +		break;
>>> +	case KVM_S390_VCPU_RESET_INITIAL:
>>> +		/* fallthrough */
>>> +	case KVM_S390_VCPU_RESET_CLEAR:
>>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>
>> As we now have two interfaces to achieve the same thing (initial reset),
>> I do wonder if we should simply introduce
>>
>> KVM_S390_NORMAL_RESET
>> KVM_S390_CLEAR_RESET
>>
>> instead ...
>>
>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
>> KVM_S390_CLEAR_RESET later for PV.
>>
>> Does anything speak against that?
> 
> Apart from loosing one more ioctl number probably not

Do we care? (I think not, but maybe I am missing something :) )
Janosch Frank Nov. 29, 2019, 2:38 p.m. UTC | #4
On 11/29/19 3:33 PM, David Hildenbrand wrote:
> On 29.11.19 15:33, Janosch Frank wrote:
>> On 11/29/19 3:31 PM, David Hildenbrand wrote:
>>> On 29.11.19 15:21, Janosch Frank wrote:
>>>> The architecture states that we need to reset local IRQs for all CPU
>>>> resets. Because the old reset interface did not support the normal CPU
>>>> reset we never did that.
>>>>
>>>> Now that we have a new interface, let's properly clear out local IRQs
>>>> and let this commit be a reminder.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  arch/s390/kvm/kvm-s390.c | 25 ++++++++++++++++++++++++-
>>>>  include/uapi/linux/kvm.h |  7 +++++++
>>>>  2 files changed, 31 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index d9e6bf3d54f0..2f74ff46b176 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -529,6 +529,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>  	case KVM_CAP_S390_CMMA_MIGRATION:
>>>>  	case KVM_CAP_S390_AIS:
>>>>  	case KVM_CAP_S390_AIS_MIGRATION:
>>>> +	case KVM_CAP_S390_VCPU_RESETS:
>>>>  		r = 1;
>>>>  		break;
>>>>  	case KVM_CAP_S390_HPAGE_1M:
>>>> @@ -3293,6 +3294,25 @@ 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 = -EINVAL;
>>>> +
>>>> +	switch (type) {
>>>> +	case KVM_S390_VCPU_RESET_NORMAL:
>>>> +		rc = 0;
>>>> +		kvm_clear_async_pf_completion_queue(vcpu);
>>>> +		kvm_s390_clear_local_irqs(vcpu);
>>>> +		break;
>>>> +	case KVM_S390_VCPU_RESET_INITIAL:
>>>> +		/* fallthrough */
>>>> +	case KVM_S390_VCPU_RESET_CLEAR:
>>>> +		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>>
>>> As we now have two interfaces to achieve the same thing (initial reset),
>>> I do wonder if we should simply introduce
>>>
>>> KVM_S390_NORMAL_RESET
>>> KVM_S390_CLEAR_RESET
>>>
>>> instead ...
>>>
>>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
>>> KVM_S390_CLEAR_RESET later for PV.
>>>
>>> Does anything speak against that?
>>
>> Apart from loosing one more ioctl number probably not
> 
> Do we care? (I think not, but maybe I am missing something :) )
> 

I don't, maybe somebody else does
Btw. I'm struggling to find a good name for the capability:
KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?
Christian Borntraeger Nov. 29, 2019, 2:39 p.m. UTC | #5
On 29.11.19 15:38, Janosch Frank wrote:
[...]
>>>> As we now have two interfaces to achieve the same thing (initial reset),
>>>> I do wonder if we should simply introduce
>>>>
>>>> KVM_S390_NORMAL_RESET
>>>> KVM_S390_CLEAR_RESET
>>>>
>>>> instead ...
>>>>
>>>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
>>>> KVM_S390_CLEAR_RESET later for PV.
>>>>
>>>> Does anything speak against that?
>>>
>>> Apart from loosing one more ioctl number probably not
>>
>> Do we care? (I think not, but maybe I am missing something :) )
>>
> 
> I don't, maybe somebody else does
> Btw. I'm struggling to find a good name for the capability:
> KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?

KVM_CAP_S390_VCPU_RESETS ?
David Hildenbrand Nov. 29, 2019, 2:48 p.m. UTC | #6
On 29.11.19 15:39, Christian Borntraeger wrote:
> 
> 
> On 29.11.19 15:38, Janosch Frank wrote:
> [...]
>>>>> As we now have two interfaces to achieve the same thing (initial reset),
>>>>> I do wonder if we should simply introduce
>>>>>
>>>>> KVM_S390_NORMAL_RESET
>>>>> KVM_S390_CLEAR_RESET
>>>>>
>>>>> instead ...
>>>>>
>>>>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
>>>>> KVM_S390_CLEAR_RESET later for PV.
>>>>>
>>>>> Does anything speak against that?
>>>>
>>>> Apart from loosing one more ioctl number probably not
>>>
>>> Do we care? (I think not, but maybe I am missing something :) )
>>>
>>
>> I don't, maybe somebody else does
>> Btw. I'm struggling to find a good name for the capability:
>> KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?
> 
> KVM_CAP_S390_VCPU_RESETS ?

Either that or two separate ones if you're not going to introduce them
at the same time ...
Janosch Frank Nov. 29, 2019, 2:57 p.m. UTC | #7
On 11/29/19 3:48 PM, David Hildenbrand wrote:
> On 29.11.19 15:39, Christian Borntraeger wrote:
>>
>>
>> On 29.11.19 15:38, Janosch Frank wrote:
>> [...]
>>>>>> As we now have two interfaces to achieve the same thing (initial reset),
>>>>>> I do wonder if we should simply introduce
>>>>>>
>>>>>> KVM_S390_NORMAL_RESET
>>>>>> KVM_S390_CLEAR_RESET
>>>>>>
>>>>>> instead ...
>>>>>>
>>>>>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
>>>>>> KVM_S390_CLEAR_RESET later for PV.
>>>>>>
>>>>>> Does anything speak against that?
>>>>>
>>>>> Apart from loosing one more ioctl number probably not
>>>>
>>>> Do we care? (I think not, but maybe I am missing something :) )
>>>>
>>>
>>> I don't, maybe somebody else does
>>> Btw. I'm struggling to find a good name for the capability:
>>> KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?
>>
>> KVM_CAP_S390_VCPU_RESETS ?
> 
> Either that or two separate ones if you're not going to introduce them
> at the same time ...
> 

This is starting to get messy...
Cornelia Huck Dec. 2, 2019, 8:54 a.m. UTC | #8
On Fri, 29 Nov 2019 15:57:25 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/29/19 3:48 PM, David Hildenbrand wrote:
> > On 29.11.19 15:39, Christian Borntraeger wrote:  
> >>
> >>
> >> On 29.11.19 15:38, Janosch Frank wrote:
> >> [...]  
> >>>>>> As we now have two interfaces to achieve the same thing (initial reset),
> >>>>>> I do wonder if we should simply introduce
> >>>>>>
> >>>>>> KVM_S390_NORMAL_RESET
> >>>>>> KVM_S390_CLEAR_RESET
> >>>>>>
> >>>>>> instead ...
> >>>>>>
> >>>>>> Then you can do KVM_S390_NORMAL_RESET for the bugfix and
> >>>>>> KVM_S390_CLEAR_RESET later for PV.
> >>>>>>
> >>>>>> Does anything speak against that?  
> >>>>>
> >>>>> Apart from loosing one more ioctl number probably not  
> >>>>
> >>>> Do we care? (I think not, but maybe I am missing something :) )
> >>>>  
> >>>
> >>> I don't, maybe somebody else does
> >>> Btw. I'm struggling to find a good name for the capability:
> >>> KVM_CAP_S390_VCPU_ADDITIONAL_RESETS ?  
> >>
> >> KVM_CAP_S390_VCPU_RESETS ?  
> > 
> > Either that or two separate ones if you're not going to introduce them
> > at the same time ...
> >   
> 
> This is starting to get messy...

In order to reduce the mess, simply introduce them at the same time? I
might be missing something, but is there anything speaking against it,
as you can simply invoke the initial reset handler for clear reset for
now?

Also:
KVM_CAP_S390_ENHANCED_VCPU_RESETS, maybe?

Patch
diff mbox series

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d9e6bf3d54f0..2f74ff46b176 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -529,6 +529,7 @@  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_CMMA_MIGRATION:
 	case KVM_CAP_S390_AIS:
 	case KVM_CAP_S390_AIS_MIGRATION:
+	case KVM_CAP_S390_VCPU_RESETS:
 		r = 1;
 		break;
 	case KVM_CAP_S390_HPAGE_1M:
@@ -3293,6 +3294,25 @@  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 = -EINVAL;
+
+	switch (type) {
+	case KVM_S390_VCPU_RESET_NORMAL:
+		rc = 0;
+		kvm_clear_async_pf_completion_queue(vcpu);
+		kvm_s390_clear_local_irqs(vcpu);
+		break;
+	case KVM_S390_VCPU_RESET_INITIAL:
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET_CLEAR:
+		rc = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		break;
+	}
+	return rc;
+}
+
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
 	vcpu_load(vcpu);
@@ -4364,7 +4384,10 @@  long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 	case KVM_S390_INITIAL_RESET:
-		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
+		arg = KVM_S390_VCPU_RESET_INITIAL;
+		/* fallthrough */
+	case KVM_S390_VCPU_RESET:
+		r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
 		break;
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 52641d8ca9e8..6da16b1f2c86 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1000,6 +1000,7 @@  struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PMU_EVENT_FILTER 173
 #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
 #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
+#define KVM_CAP_S390_VCPU_RESETS 180
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1462,12 @@  struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#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,   0xc3)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */