| Message ID | 20191129142122.21528-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add new reset vcpu API | expand |
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?
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
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 :) )
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 ?
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 ?
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 ...
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...
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?
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 */
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(-)