diff mbox series

[v3] KVM: s390: Add new reset vcpu API

Message ID 20191205120956.50930-1-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series [v3] KVM: s390: Add new reset vcpu API | expand

Commit Message

Janosch Frank Dec. 5, 2019, 12:09 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.

Also we add a ioctl for the clear reset to have all resets exposed to
userspace. Currently the clear reset falls back to the initial reset,
but we plan to have clear reset specific code in the future.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 Documentation/virt/kvm/api.txt | 45 ++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c       | 14 +++++++++++
 include/uapi/linux/kvm.h       |  5 ++++
 3 files changed, 64 insertions(+)

Comments

Cornelia Huck Dec. 5, 2019, 12:19 p.m. UTC | #1
On Thu,  5 Dec 2019 07:09:56 -0500
Janosch Frank <frankja@linux.ibm.com> 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.

I'm still confused what this commit is supposed to be a reminder of :)

> 
> Also we add a ioctl for the clear reset to have all resets exposed to
> userspace. Currently the clear reset falls back to the initial reset,
> but we plan to have clear reset specific code in the future.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  Documentation/virt/kvm/api.txt | 45 ++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c       | 14 +++++++++++
>  include/uapi/linux/kvm.h       |  5 ++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt
> index 4833904d32a5..3b62489308e0 100644
> --- a/Documentation/virt/kvm/api.txt
> +++ b/Documentation/virt/kvm/api.txt
> @@ -4126,6 +4126,44 @@ Valid values for 'action':
>  #define KVM_PMU_EVENT_ALLOW 0
>  #define KVM_PMU_EVENT_DENY 1
>  
> +4.121 KVM_S390_NORMAL_RESET
> +
> +Capability: KVM_CAP_S390_VCPU_RESETS
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: none
> +Returns: 0
> +
> +This ioctl resets VCPU registers and control structures that QEMU

s/QEMU/userspace/ (also below)

> +can't access via the kvm_run structure. The normal reset clears local
> +interrupts, the riccb and PSW bit 24.

What about:

"It is intended to be called when a normal reset is performed on the
vcpu and clears..."

> +
> +4.122 KVM_S390_INITIAL_RESET

Oh, we had never documented it before?

> +
> +Capability: none
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: none
> +Returns: 0
> +
> +This ioctl resets VCPU registers and control structures that QEMU
> +can't access via the kvm_run structure. The initial reset is a
> +superset of the normal reset and additionally clears the psw, prefix,
> +timing related registers, as well as setting the control registers to
> +their initial value.

"It is intended to be called when an initial reset (which is a superset
of the normal reset) is performed on the vcpu and additionally
clears..."

?

> +
> +4.123 KVM_S390_CLEAR_RESET
> +
> +Capability: KVM_CAP_S390_VCPU_RESETS
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: none
> +Returns: 0
> +
> +This ioctl resets VCPU registers and control structures that QEMU
> +can't access via the kvm_run structure. The clear reset is a superset
> +of the initial reset and additionally clears general, access, floating
> +and vector registers.

You can probably guess what I was going to write :)

>  
>  5. The kvm_run structure
>  ------------------------
> @@ -5322,3 +5360,10 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
>  flush hypercalls by Hyper-V) so userspace should disable KVM identification
>  in CPUID and only exposes Hyper-V identification. In this case, guest
>  thinks it's running on Hyper-V and only use Hyper-V hypercalls.
> +
> +8.22 KVM_CAP_S390_VCPU_RESETS
> +
> +Architectures: s390
> +
> +This capability indicates that the KVM_S390_NORMAL_RESET and
> +KVM_S390_CLEAR_RESET ioctls are available.

(...)

The code looks good.
Christian Borntraeger Jan. 8, 2020, 2:28 p.m. UTC | #2
On 05.12.19 13:09, Janosch Frank wrote:
[...]
> +4.123 KVM_S390_CLEAR_RESET
> +
> +Capability: KVM_CAP_S390_VCPU_RESETS
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: none
> +Returns: 0
> +
> +This ioctl resets VCPU registers and control structures that QEMU
> +can't access via the kvm_run structure. The clear reset is a superset
> +of the initial reset and additionally clears general, access, floating
> +and vector registers.

As Thomas outlined, make it more obvious that userspace does the remaining
parts. I do not think that we want the kernel to do the things (unless it
helps you in some way for the ultravisor guests)

[...]
>  
> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> +{
> +	kvm_clear_async_pf_completion_queue(vcpu);
> +	kvm_s390_clear_local_irqs(vcpu);
> +	return 0;

Shouldnt we also do 
        if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
                kvm_s390_vcpu_stop(vcpu);

here?


> +}
> +
>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>  {
>  	kvm_s390_vcpu_initial_reset(vcpu);
> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>  		break;
>  	}
> +
> +	case KVM_S390_CLEAR_RESET:
> +		/* fallthrough */
>  	case KVM_S390_INITIAL_RESET:
>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>  		break;

Then we could also do a fallthrough here and do:

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d9e6bf3..c715ae3 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
        vcpu->arch.sie_block->pp = 0;
        vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
        vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
-       kvm_clear_async_pf_completion_queue(vcpu);
-       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
-               kvm_s390_vcpu_stop(vcpu);
-       kvm_s390_clear_local_irqs(vcpu);
 }
 
 void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)




> +	case KVM_S390_NORMAL_RESET:
> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> +		break;
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
Janosch Frank Jan. 8, 2020, 2:35 p.m. UTC | #3
On 1/8/20 3:28 PM, Christian Borntraeger wrote:
> 
> 
> On 05.12.19 13:09, Janosch Frank wrote:
> [...]
>> +4.123 KVM_S390_CLEAR_RESET
>> +
>> +Capability: KVM_CAP_S390_VCPU_RESETS
>> +Architectures: s390
>> +Type: vcpu ioctl
>> +Parameters: none
>> +Returns: 0
>> +
>> +This ioctl resets VCPU registers and control structures that QEMU
>> +can't access via the kvm_run structure. The clear reset is a superset
>> +of the initial reset and additionally clears general, access, floating
>> +and vector registers.
> 
> As Thomas outlined, make it more obvious that userspace does the remaining
> parts. I do not think that we want the kernel to do the things (unless it
> helps you in some way for the ultravisor guests)

Ok, will do

> 
> [...]
>>  
>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>> +{
>> +	kvm_clear_async_pf_completion_queue(vcpu);
>> +	kvm_s390_clear_local_irqs(vcpu);
>> +	return 0;
> 
> Shouldnt we also do 
>         if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>                 kvm_s390_vcpu_stop(vcpu);
> 
> here?

Isn't userspace cpu state ctrl basically a prereq anyway for getting
those new ioctls?

> 
> 
>> +}
>> +
>>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>  {
>>  	kvm_s390_vcpu_initial_reset(vcpu);
>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>  		break;
>>  	}
>> +
>> +	case KVM_S390_CLEAR_RESET:
>> +		/* fallthrough */
>>  	case KVM_S390_INITIAL_RESET:
>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>  		break;
> 
> Then we could also do a fallthrough here and do:
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3..c715ae3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>         vcpu->arch.sie_block->pp = 0;
>         vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>         vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
> -       kvm_clear_async_pf_completion_queue(vcpu);
> -       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
> -               kvm_s390_vcpu_stop(vcpu);
> -       kvm_s390_clear_local_irqs(vcpu);
>  }
>  
>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
> 
> 
> 
> 
>> +	case KVM_S390_NORMAL_RESET:
>> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>> +		break;
>>  	case KVM_SET_ONE_REG:
>>  	case KVM_GET_ONE_REG: {
Christian Borntraeger Jan. 8, 2020, 2:35 p.m. UTC | #4
On 08.01.20 15:28, Christian Borntraeger wrote:
> 
> 
> On 05.12.19 13:09, Janosch Frank wrote:
> [...]
>> +4.123 KVM_S390_CLEAR_RESET
>> +
>> +Capability: KVM_CAP_S390_VCPU_RESETS
>> +Architectures: s390
>> +Type: vcpu ioctl
>> +Parameters: none
>> +Returns: 0
>> +
>> +This ioctl resets VCPU registers and control structures that QEMU
>> +can't access via the kvm_run structure. The clear reset is a superset
>> +of the initial reset and additionally clears general, access, floating
>> +and vector registers.
> 
> As Thomas outlined, make it more obvious that userspace does the remaining
> parts. I do not think that we want the kernel to do the things (unless it
> helps you in some way for the ultravisor guests)

On the other hand. todays initial cpu reset DOES everything. So I guess
the other ones should do the same. That actually makes the semantics clearer -
when you call it it will have done whatever reset you have asked for.


> 
> [...]
>>  
>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>> +{
>> +	kvm_clear_async_pf_completion_queue(vcpu);
>> +	kvm_s390_clear_local_irqs(vcpu);
>> +	return 0;
> 
> Shouldnt we also do 
>         if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>                 kvm_s390_vcpu_stop(vcpu);
> 
> here?
> 
> 
>> +}
>> +
>>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>  {
>>  	kvm_s390_vcpu_initial_reset(vcpu);
>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>  		break;
>>  	}
>> +
>> +	case KVM_S390_CLEAR_RESET:
>> +		/* fallthrough */
>>  	case KVM_S390_INITIAL_RESET:
>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>  		break;
> 
> Then we could also do a fallthrough here and do:
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3..c715ae3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>         vcpu->arch.sie_block->pp = 0;
>         vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>         vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
> -       kvm_clear_async_pf_completion_queue(vcpu);
> -       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
> -               kvm_s390_vcpu_stop(vcpu);
> -       kvm_s390_clear_local_irqs(vcpu);
>  }
>  
>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
> 
> 
> 
> 
>> +	case KVM_S390_NORMAL_RESET:
>> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>> +		break;
>>  	case KVM_SET_ONE_REG:
>>  	case KVM_GET_ONE_REG: {
Christian Borntraeger Jan. 8, 2020, 2:38 p.m. UTC | #5
On 08.01.20 15:35, Janosch Frank wrote:
> On 1/8/20 3:28 PM, Christian Borntraeger wrote:
>>
>>
>> On 05.12.19 13:09, Janosch Frank wrote:
>> [...]
>>> +4.123 KVM_S390_CLEAR_RESET
>>> +
>>> +Capability: KVM_CAP_S390_VCPU_RESETS
>>> +Architectures: s390
>>> +Type: vcpu ioctl
>>> +Parameters: none
>>> +Returns: 0
>>> +
>>> +This ioctl resets VCPU registers and control structures that QEMU
>>> +can't access via the kvm_run structure. The clear reset is a superset
>>> +of the initial reset and additionally clears general, access, floating
>>> +and vector registers.
>>
>> As Thomas outlined, make it more obvious that userspace does the remaining
>> parts. I do not think that we want the kernel to do the things (unless it
>> helps you in some way for the ultravisor guests)
> 
> Ok, will do

I changed my mind (see my other mail) but I would like Thomas, Conny or David
to ack/nack.

> 
>>
>> [...]
>>>  
>>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>> +{
>>> +	kvm_clear_async_pf_completion_queue(vcpu);
>>> +	kvm_s390_clear_local_irqs(vcpu);
>>> +	return 0;
>>
>> Shouldnt we also do 
>>         if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>>                 kvm_s390_vcpu_stop(vcpu);
>>
>> here?
> 
> Isn't userspace cpu state ctrl basically a prereq anyway for getting
> those new ioctls?

I do not see a reason why we should mandate userspace cpu state for the normal
reset. Using this if also enables the fallthrough below - which I like.

> 
>>
>>
>>> +}
>>> +
>>>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>>  {
>>>  	kvm_s390_vcpu_initial_reset(vcpu);
>>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>>  		break;
>>>  	}
>>> +
>>> +	case KVM_S390_CLEAR_RESET:
>>> +		/* fallthrough */
>>>  	case KVM_S390_INITIAL_RESET:
>>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>>  		break;
>>
>> Then we could also do a fallthrough here and do:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d9e6bf3..c715ae3 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>>         vcpu->arch.sie_block->pp = 0;
>>         vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>>         vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>> -       kvm_clear_async_pf_completion_queue(vcpu);
>> -       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>> -               kvm_s390_vcpu_stop(vcpu);
>> -       kvm_s390_clear_local_irqs(vcpu);
>>  }
>>  
>>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>
>>
>>
>>
>>> +	case KVM_S390_NORMAL_RESET:
>>> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>>> +		break;
>>>  	case KVM_SET_ONE_REG:
>>>  	case KVM_GET_ONE_REG: {
> 
>
Janosch Frank Jan. 8, 2020, 2:42 p.m. UTC | #6
On 1/8/20 3:35 PM, Christian Borntraeger wrote:
> 
> 
> On 08.01.20 15:28, Christian Borntraeger wrote:
>>
>>
>> On 05.12.19 13:09, Janosch Frank wrote:
>> [...]
>>> +4.123 KVM_S390_CLEAR_RESET
>>> +
>>> +Capability: KVM_CAP_S390_VCPU_RESETS
>>> +Architectures: s390
>>> +Type: vcpu ioctl
>>> +Parameters: none
>>> +Returns: 0
>>> +
>>> +This ioctl resets VCPU registers and control structures that QEMU
>>> +can't access via the kvm_run structure. The clear reset is a superset
>>> +of the initial reset and additionally clears general, access, floating
>>> +and vector registers.
>>
>> As Thomas outlined, make it more obvious that userspace does the remaining
>> parts. I do not think that we want the kernel to do the things (unless it
>> helps you in some way for the ultravisor guests)
> 
> On the other hand. todays initial cpu reset DOES everything. So I guess
> the other ones should do the same. That actually makes the semantics clearer -
> when you call it it will have done whatever reset you have asked for.

Ok, will do, thanks for the clearup

> 
> 
>>
>> [...]
>>>  
>>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>> +{
>>> +	kvm_clear_async_pf_completion_queue(vcpu);
>>> +	kvm_s390_clear_local_irqs(vcpu);
>>> +	return 0;
>>
>> Shouldnt we also do 
>>         if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>>                 kvm_s390_vcpu_stop(vcpu);
>>
>> here?
>>
>>
>>> +}
>>> +
>>>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>>  {
>>>  	kvm_s390_vcpu_initial_reset(vcpu);
>>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>>  		break;
>>>  	}
>>> +
>>> +	case KVM_S390_CLEAR_RESET:
>>> +		/* fallthrough */
>>>  	case KVM_S390_INITIAL_RESET:
>>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>>  		break;
>>
>> Then we could also do a fallthrough here and do:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d9e6bf3..c715ae3 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>>         vcpu->arch.sie_block->pp = 0;
>>         vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>>         vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>> -       kvm_clear_async_pf_completion_queue(vcpu);
>> -       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>> -               kvm_s390_vcpu_stop(vcpu);
>> -       kvm_s390_clear_local_irqs(vcpu);
>>  }
>>  
>>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>
>>
>>
>>
>>> +	case KVM_S390_NORMAL_RESET:
>>> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>>> +		break;
>>>  	case KVM_SET_ONE_REG:
>>>  	case KVM_GET_ONE_REG: {
>
Thomas Huth Jan. 8, 2020, 2:44 p.m. UTC | #7
On 08/01/2020 15.38, Christian Borntraeger wrote:
> 
> 
> On 08.01.20 15:35, Janosch Frank wrote:
>> On 1/8/20 3:28 PM, Christian Borntraeger wrote:
>>>
>>>
>>> On 05.12.19 13:09, Janosch Frank wrote:
>>> [...]
>>>> +4.123 KVM_S390_CLEAR_RESET
>>>> +
>>>> +Capability: KVM_CAP_S390_VCPU_RESETS
>>>> +Architectures: s390
>>>> +Type: vcpu ioctl
>>>> +Parameters: none
>>>> +Returns: 0
>>>> +
>>>> +This ioctl resets VCPU registers and control structures that QEMU
>>>> +can't access via the kvm_run structure. The clear reset is a superset
>>>> +of the initial reset and additionally clears general, access, floating
>>>> +and vector registers.
>>>
>>> As Thomas outlined, make it more obvious that userspace does the remaining
>>> parts. I do not think that we want the kernel to do the things (unless it
>>> helps you in some way for the ultravisor guests)
>>
>> Ok, will do
> 
> I changed my mind (see my other mail) but I would like Thomas, Conny or David
> to ack/nack.

I don't mind too much as long as it is properly documented, but I also
slightly prefer to be consistent here, i.e. let the kernel clear the
rest here, too, just like we do it already with the initial reset.

 Thomas
Cornelia Huck Jan. 8, 2020, 3:36 p.m. UTC | #8
On Wed, 8 Jan 2020 15:44:57 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 08/01/2020 15.38, Christian Borntraeger wrote:
> > 
> > 
> > On 08.01.20 15:35, Janosch Frank wrote:  
> >> On 1/8/20 3:28 PM, Christian Borntraeger wrote:  
> >>>
> >>>
> >>> On 05.12.19 13:09, Janosch Frank wrote:
> >>> [...]  
> >>>> +4.123 KVM_S390_CLEAR_RESET
> >>>> +
> >>>> +Capability: KVM_CAP_S390_VCPU_RESETS
> >>>> +Architectures: s390
> >>>> +Type: vcpu ioctl
> >>>> +Parameters: none
> >>>> +Returns: 0
> >>>> +
> >>>> +This ioctl resets VCPU registers and control structures that QEMU
> >>>> +can't access via the kvm_run structure. The clear reset is a superset
> >>>> +of the initial reset and additionally clears general, access, floating
> >>>> +and vector registers.  
> >>>
> >>> As Thomas outlined, make it more obvious that userspace does the remaining
> >>> parts. I do not think that we want the kernel to do the things (unless it
> >>> helps you in some way for the ultravisor guests)  
> >>
> >> Ok, will do  
> > 
> > I changed my mind (see my other mail) but I would like Thomas, Conny or David
> > to ack/nack.  
> 
> I don't mind too much as long as it is properly documented, but I also
> slightly prefer to be consistent here, i.e. let the kernel clear the
> rest here, too, just like we do it already with the initial reset.

I definitely agree with the 'properly documented' part :) It's probably
enough to just state that the kernel resets the stuff, no need to go
into details (and we also don't need to update this for later versions.)

Q: Are we sure that we will always be able to reset everything from the
kernel?
diff mbox series

Patch

diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt
index 4833904d32a5..3b62489308e0 100644
--- a/Documentation/virt/kvm/api.txt
+++ b/Documentation/virt/kvm/api.txt
@@ -4126,6 +4126,44 @@  Valid values for 'action':
 #define KVM_PMU_EVENT_ALLOW 0
 #define KVM_PMU_EVENT_DENY 1
 
+4.121 KVM_S390_NORMAL_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The normal reset clears local
+interrupts, the riccb and PSW bit 24.
+
+4.122 KVM_S390_INITIAL_RESET
+
+Capability: none
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The initial reset is a
+superset of the normal reset and additionally clears the psw, prefix,
+timing related registers, as well as setting the control registers to
+their initial value.
+
+4.123 KVM_S390_CLEAR_RESET
+
+Capability: KVM_CAP_S390_VCPU_RESETS
+Architectures: s390
+Type: vcpu ioctl
+Parameters: none
+Returns: 0
+
+This ioctl resets VCPU registers and control structures that QEMU
+can't access via the kvm_run structure. The clear reset is a superset
+of the initial reset and additionally clears general, access, floating
+and vector registers.
 
 5. The kvm_run structure
 ------------------------
@@ -5322,3 +5360,10 @@  handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_S390_VCPU_RESETS
+
+Architectures: s390
+
+This capability indicates that the KVM_S390_NORMAL_RESET and
+KVM_S390_CLEAR_RESET ioctls are available.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d9e6bf3d54f0..7f3ede0b2715 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:
@@ -3287,6 +3288,13 @@  static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
 	return r;
 }
 
+static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
+{
+	kvm_clear_async_pf_completion_queue(vcpu);
+	kvm_s390_clear_local_irqs(vcpu);
+	return 0;
+}
+
 static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_vcpu_initial_reset(vcpu);
@@ -4363,9 +4371,15 @@  long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
 		break;
 	}
+
+	case KVM_S390_CLEAR_RESET:
+		/* fallthrough */
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_NORMAL_RESET:
+		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
+		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 52641d8ca9e8..edbb2da43f02 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 176
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1462,10 @@  struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+/* Available with  KVM_CAP_S390_VCPU_RESETS */
+#define KVM_S390_NORMAL_RESET	_IO(KVMIO,   0xc3)
+#define KVM_S390_CLEAR_RESET	_IO(KVMIO,   0xc4)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */