| Message ID | 20200121163338.21704-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] target/s390x/kvm: Enable adapter interruption suppression again | expand |
On 21.01.20 17:33, Thomas Huth wrote: > The AIS feature has been disabled late in the v2.10 development cycle since > there were some issues with migration (see commit 3f2d07b3b01ea61126b - > "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted > to enable it again for newer machine types, but apparently we forgot to do > this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now. > > While at it, also add a more verbose comment why we need the *_allowed() > wrappers in s390-virtio-ccw.c. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > Reviewed-by: David Hildenbrand <david@redhat.com> > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v4: Use kvm_kernel_irqchip_allowed() for avoiding problems when running > with -machine s390-ccw-virtio,kernel_irqchip=off > > hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++--- > include/hw/s390x/s390-virtio-ccw.h | 3 +++ > target/s390x/kvm.c | 9 ++++++--- > 3 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e0e28139a2..76254e8447 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) > s390mc->cpu_model_allowed = true; > s390mc->css_migration_enabled = true; > s390mc->hpage_1m_allowed = true; > + s390mc->kvm_ais_allowed = true; > mc->init = ccw_init; > mc->reset = s390_machine_reset; > mc->hot_add_cpu = s390_hot_add_cpu; > @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, > > static S390CcwMachineClass *current_mc; > > +/* > + * Get the class of the s390-ccw-virtio machine that is currently in use. > + * Note: libvirt is using the "none" machine to probe for the features of the > + * host CPU, so in case this is called with the "none" machine, the function > + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the > + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers > + * below return the correct default value for the "none" machine. > + */ > static S390CcwMachineClass *get_machine_class(void) > { > if (unlikely(!current_mc)) { > @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void) > > bool ri_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->ri_allowed; > } > > bool cpu_model_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->cpu_model_allowed; > } > > bool hpage_1m_allowed(void) > { > - /* for "none" machine this results in true */ > return get_machine_class()->hpage_1m_allowed; > } > > +bool kvm_ais_allowed(void) > +{ > + return get_machine_class()->kvm_ais_allowed; > +} > + > static char *machine_get_loadparm(Object *obj, Error **errp) > { > S390CcwMachineState *ms = S390_CCW_MACHINE(obj); > @@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine) > > static void ccw_machine_4_2_class_options(MachineClass *mc) > { > + S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc); > + > ccw_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > + s390mc->kvm_ais_allowed = false; > } > DEFINE_CCW_MACHINE(4_2, "4.2", false); > > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h > index 8aa27199c9..e3ba3b88b1 100644 > --- a/include/hw/s390x/s390-virtio-ccw.h > +++ b/include/hw/s390x/s390-virtio-ccw.h > @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass { > bool cpu_model_allowed; > bool css_migration_enabled; > bool hpage_1m_allowed; > + bool kvm_ais_allowed; > } S390CcwMachineClass; > > /* runtime-instrumentation allowed by the machine */ > @@ -48,6 +49,8 @@ bool ri_allowed(void); > bool cpu_model_allowed(void); > /* 1M huge page mappings allowed by the machine */ > bool hpage_1m_allowed(void); > +/* adapter-interrupt suppression allowed by the machine? */ > +bool kvm_ais_allowed(void); > > /** > * Returns true if (vmstate based) migration of the channel subsystem > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 15260aeb9a..1602a2c33d 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > /* > * The migration interface for ais was introduced with kernel 4.13 > * but the capability itself had been active since 4.12. As migration > - * support is considered necessary let's disable ais in the 2.10 > - * machine. > + * support is considered necessary, we only try to enable this for > + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. > */ > - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ > + if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() && > + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { > + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); > + } I just remembered that the availability of CPU features should in general not depend on the selected machine. We only have compat handling for pre-cpu-model-support machines (e.g., vx). The issue is, that the probed host cpu model is otherwise not guaranteed to run om a selected machine and you get misleading errors. Can someone remind me why we need kvm_ais_allowed() at all and cannot simply rely on cpu model checks to properly handle this (like all other features)?
On 21/01/2020 17.48, David Hildenbrand wrote: > On 21.01.20 17:33, Thomas Huth wrote: >> The AIS feature has been disabled late in the v2.10 development cycle since >> there were some issues with migration (see commit 3f2d07b3b01ea61126b - >> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted >> to enable it again for newer machine types, but apparently we forgot to do >> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now. >> >> While at it, also add a more verbose comment why we need the *_allowed() >> wrappers in s390-virtio-ccw.c. >> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >> Reviewed-by: David Hildenbrand <david@redhat.com> >> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> v4: Use kvm_kernel_irqchip_allowed() for avoiding problems when running >> with -machine s390-ccw-virtio,kernel_irqchip=off >> >> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++--- >> include/hw/s390x/s390-virtio-ccw.h | 3 +++ >> target/s390x/kvm.c | 9 ++++++--- >> 3 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index e0e28139a2..76254e8447 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) >> s390mc->cpu_model_allowed = true; >> s390mc->css_migration_enabled = true; >> s390mc->hpage_1m_allowed = true; >> + s390mc->kvm_ais_allowed = true; >> mc->init = ccw_init; >> mc->reset = s390_machine_reset; >> mc->hot_add_cpu = s390_hot_add_cpu; >> @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, >> >> static S390CcwMachineClass *current_mc; >> >> +/* >> + * Get the class of the s390-ccw-virtio machine that is currently in use. >> + * Note: libvirt is using the "none" machine to probe for the features of the >> + * host CPU, so in case this is called with the "none" machine, the function >> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the >> + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers >> + * below return the correct default value for the "none" machine. >> + */ >> static S390CcwMachineClass *get_machine_class(void) >> { >> if (unlikely(!current_mc)) { >> @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void) >> >> bool ri_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->ri_allowed; >> } >> >> bool cpu_model_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->cpu_model_allowed; >> } >> >> bool hpage_1m_allowed(void) >> { >> - /* for "none" machine this results in true */ >> return get_machine_class()->hpage_1m_allowed; >> } >> >> +bool kvm_ais_allowed(void) >> +{ >> + return get_machine_class()->kvm_ais_allowed; >> +} >> + >> static char *machine_get_loadparm(Object *obj, Error **errp) >> { >> S390CcwMachineState *ms = S390_CCW_MACHINE(obj); >> @@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine) >> >> static void ccw_machine_4_2_class_options(MachineClass *mc) >> { >> + S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc); >> + >> ccw_machine_5_0_class_options(mc); >> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >> + s390mc->kvm_ais_allowed = false; >> } >> DEFINE_CCW_MACHINE(4_2, "4.2", false); >> >> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >> index 8aa27199c9..e3ba3b88b1 100644 >> --- a/include/hw/s390x/s390-virtio-ccw.h >> +++ b/include/hw/s390x/s390-virtio-ccw.h >> @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass { >> bool cpu_model_allowed; >> bool css_migration_enabled; >> bool hpage_1m_allowed; >> + bool kvm_ais_allowed; >> } S390CcwMachineClass; >> >> /* runtime-instrumentation allowed by the machine */ >> @@ -48,6 +49,8 @@ bool ri_allowed(void); >> bool cpu_model_allowed(void); >> /* 1M huge page mappings allowed by the machine */ >> bool hpage_1m_allowed(void); >> +/* adapter-interrupt suppression allowed by the machine? */ >> +bool kvm_ais_allowed(void); >> >> /** >> * Returns true if (vmstate based) migration of the channel subsystem >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 15260aeb9a..1602a2c33d 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >> /* >> * The migration interface for ais was introduced with kernel 4.13 >> * but the capability itself had been active since 4.12. As migration >> - * support is considered necessary let's disable ais in the 2.10 >> - * machine. >> + * support is considered necessary, we only try to enable this for >> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. >> */ >> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >> + if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() && >> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >> + } > > I just remembered that the availability of CPU features should in > general not depend on the selected machine. We only have compat handling > for pre-cpu-model-support machines (e.g., vx). > > The issue is, that the probed host cpu model is otherwise not guaranteed > to run om a selected machine and you get misleading errors. > > Can someone remind me why we need kvm_ais_allowed() at all and cannot > simply rely on cpu model checks to properly handle this (like all other > features)? Sorry, I don't quite get what you mean here. Which other features are you talking about? KVM_CAP_S390_RI and KVM_CAP_S390_GS are enabled in a very similar way... Thomas
On 1/21/20 11:33 AM, Thomas Huth wrote: > The AIS feature has been disabled late in the v2.10 development cycle since > there were some issues with migration (see commit 3f2d07b3b01ea61126b - > "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted > to enable it again for newer machine types, but apparently we forgot to do > this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now. > > While at it, also add a more verbose comment why we need the *_allowed() > wrappers in s390-virtio-ccw.c. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > Reviewed-by: David Hildenbrand <david@redhat.com> > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v4: Use kvm_kernel_irqchip_allowed() for avoiding problems when running > with -machine s390-ccw-virtio,kernel_irqchip=off > > hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++--- > include/hw/s390x/s390-virtio-ccw.h | 3 +++ > target/s390x/kvm.c | 9 ++++++--- > 3 files changed, 26 insertions(+), 6 deletions(-) > For the sake of completeness, re-ran a quick test on v4. Looks good.
On 21.01.20 17:56, Thomas Huth wrote: > On 21/01/2020 17.48, David Hildenbrand wrote: >> On 21.01.20 17:33, Thomas Huth wrote: >>> The AIS feature has been disabled late in the v2.10 development cycle since >>> there were some issues with migration (see commit 3f2d07b3b01ea61126b - >>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted >>> to enable it again for newer machine types, but apparently we forgot to do >>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now. >>> >>> While at it, also add a more verbose comment why we need the *_allowed() >>> wrappers in s390-virtio-ccw.c. >>> >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>> Reviewed-by: David Hildenbrand <david@redhat.com> >>> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> v4: Use kvm_kernel_irqchip_allowed() for avoiding problems when running >>> with -machine s390-ccw-virtio,kernel_irqchip=off >>> >>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++--- >>> include/hw/s390x/s390-virtio-ccw.h | 3 +++ >>> target/s390x/kvm.c | 9 ++++++--- >>> 3 files changed, 26 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index e0e28139a2..76254e8447 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) >>> s390mc->cpu_model_allowed = true; >>> s390mc->css_migration_enabled = true; >>> s390mc->hpage_1m_allowed = true; >>> + s390mc->kvm_ais_allowed = true; >>> mc->init = ccw_init; >>> mc->reset = s390_machine_reset; >>> mc->hot_add_cpu = s390_hot_add_cpu; >>> @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, >>> >>> static S390CcwMachineClass *current_mc; >>> >>> +/* >>> + * Get the class of the s390-ccw-virtio machine that is currently in use. >>> + * Note: libvirt is using the "none" machine to probe for the features of the >>> + * host CPU, so in case this is called with the "none" machine, the function >>> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the >>> + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers >>> + * below return the correct default value for the "none" machine. >>> + */ >>> static S390CcwMachineClass *get_machine_class(void) >>> { >>> if (unlikely(!current_mc)) { >>> @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void) >>> >>> bool ri_allowed(void) >>> { >>> - /* for "none" machine this results in true */ >>> return get_machine_class()->ri_allowed; >>> } >>> >>> bool cpu_model_allowed(void) >>> { >>> - /* for "none" machine this results in true */ >>> return get_machine_class()->cpu_model_allowed; >>> } >>> >>> bool hpage_1m_allowed(void) >>> { >>> - /* for "none" machine this results in true */ >>> return get_machine_class()->hpage_1m_allowed; >>> } >>> >>> +bool kvm_ais_allowed(void) >>> +{ >>> + return get_machine_class()->kvm_ais_allowed; >>> +} >>> + >>> static char *machine_get_loadparm(Object *obj, Error **errp) >>> { >>> S390CcwMachineState *ms = S390_CCW_MACHINE(obj); >>> @@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine) >>> >>> static void ccw_machine_4_2_class_options(MachineClass *mc) >>> { >>> + S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc); >>> + >>> ccw_machine_5_0_class_options(mc); >>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >>> + s390mc->kvm_ais_allowed = false; >>> } >>> DEFINE_CCW_MACHINE(4_2, "4.2", false); >>> >>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >>> index 8aa27199c9..e3ba3b88b1 100644 >>> --- a/include/hw/s390x/s390-virtio-ccw.h >>> +++ b/include/hw/s390x/s390-virtio-ccw.h >>> @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass { >>> bool cpu_model_allowed; >>> bool css_migration_enabled; >>> bool hpage_1m_allowed; >>> + bool kvm_ais_allowed; >>> } S390CcwMachineClass; >>> >>> /* runtime-instrumentation allowed by the machine */ >>> @@ -48,6 +49,8 @@ bool ri_allowed(void); >>> bool cpu_model_allowed(void); >>> /* 1M huge page mappings allowed by the machine */ >>> bool hpage_1m_allowed(void); >>> +/* adapter-interrupt suppression allowed by the machine? */ >>> +bool kvm_ais_allowed(void); >>> >>> /** >>> * Returns true if (vmstate based) migration of the channel subsystem >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 15260aeb9a..1602a2c33d 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >>> /* >>> * The migration interface for ais was introduced with kernel 4.13 >>> * but the capability itself had been active since 4.12. As migration >>> - * support is considered necessary let's disable ais in the 2.10 >>> - * machine. >>> + * support is considered necessary, we only try to enable this for >>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. >>> */ >>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>> + if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() && >>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >>> + } >> >> I just remembered that the availability of CPU features should in >> general not depend on the selected machine. We only have compat handling >> for pre-cpu-model-support machines (e.g., vx). >> >> The issue is, that the probed host cpu model is otherwise not guaranteed >> to run om a selected machine and you get misleading errors. >> >> Can someone remind me why we need kvm_ais_allowed() at all and cannot >> simply rely on cpu model checks to properly handle this (like all other >> features)? > > Sorry, I don't quite get what you mean here. Which other features are > you talking about? KVM_CAP_S390_RI and KVM_CAP_S390_GS are enabled in a > very similar way... Similar but different. If you look closely, that was all being done before we had machines with CPU model support 2.8 introduced CPU models 2.7 introduced RI So ri_allowed() is in place because we have to handle compatibility thingies with machines before we had cpu_model_allowed(). Basically, cpu model support invalidated the need for all new flags. You can see how it is to be done for KVM_CAP_S390_GS So this here should be if (cpu_model_allowed() && kvm_kernel_irqchip_allowed() ... We do have to make sure that all migration-related things are also glued to the actual availability of the feature. Usually, there should be a s390_has_feat(ADAPTER_INT_SUPPRESSION) somewhere that guards this (e.g., see target/s390x/machine.c:vregs_needed()). Sorry, I have to revoke my rb, was not paying enough attention :(
On 21.01.20 18:06, David Hildenbrand wrote: > On 21.01.20 17:56, Thomas Huth wrote: >> On 21/01/2020 17.48, David Hildenbrand wrote: >>> On 21.01.20 17:33, Thomas Huth wrote: >>>> The AIS feature has been disabled late in the v2.10 development cycle since >>>> there were some issues with migration (see commit 3f2d07b3b01ea61126b - >>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted >>>> to enable it again for newer machine types, but apparently we forgot to do >>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now. >>>> >>>> While at it, also add a more verbose comment why we need the *_allowed() >>>> wrappers in s390-virtio-ccw.c. >>>> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>>> Reviewed-by: David Hildenbrand <david@redhat.com> >>>> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> v4: Use kvm_kernel_irqchip_allowed() for avoiding problems when running >>>> with -machine s390-ccw-virtio,kernel_irqchip=off >>>> >>>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++--- >>>> include/hw/s390x/s390-virtio-ccw.h | 3 +++ >>>> target/s390x/kvm.c | 9 ++++++--- >>>> 3 files changed, 26 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index e0e28139a2..76254e8447 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) >>>> s390mc->cpu_model_allowed = true; >>>> s390mc->css_migration_enabled = true; >>>> s390mc->hpage_1m_allowed = true; >>>> + s390mc->kvm_ais_allowed = true; >>>> mc->init = ccw_init; >>>> mc->reset = s390_machine_reset; >>>> mc->hot_add_cpu = s390_hot_add_cpu; >>>> @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, >>>> >>>> static S390CcwMachineClass *current_mc; >>>> >>>> +/* >>>> + * Get the class of the s390-ccw-virtio machine that is currently in use. >>>> + * Note: libvirt is using the "none" machine to probe for the features of the >>>> + * host CPU, so in case this is called with the "none" machine, the function >>>> + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the >>>> + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers >>>> + * below return the correct default value for the "none" machine. >>>> + */ >>>> static S390CcwMachineClass *get_machine_class(void) >>>> { >>>> if (unlikely(!current_mc)) { >>>> @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void) >>>> >>>> bool ri_allowed(void) >>>> { >>>> - /* for "none" machine this results in true */ >>>> return get_machine_class()->ri_allowed; >>>> } >>>> >>>> bool cpu_model_allowed(void) >>>> { >>>> - /* for "none" machine this results in true */ >>>> return get_machine_class()->cpu_model_allowed; >>>> } >>>> >>>> bool hpage_1m_allowed(void) >>>> { >>>> - /* for "none" machine this results in true */ >>>> return get_machine_class()->hpage_1m_allowed; >>>> } >>>> >>>> +bool kvm_ais_allowed(void) >>>> +{ >>>> + return get_machine_class()->kvm_ais_allowed; >>>> +} >>>> + >>>> static char *machine_get_loadparm(Object *obj, Error **errp) >>>> { >>>> S390CcwMachineState *ms = S390_CCW_MACHINE(obj); >>>> @@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine) >>>> >>>> static void ccw_machine_4_2_class_options(MachineClass *mc) >>>> { >>>> + S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc); >>>> + >>>> ccw_machine_5_0_class_options(mc); >>>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >>>> + s390mc->kvm_ais_allowed = false; >>>> } >>>> DEFINE_CCW_MACHINE(4_2, "4.2", false); >>>> >>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >>>> index 8aa27199c9..e3ba3b88b1 100644 >>>> --- a/include/hw/s390x/s390-virtio-ccw.h >>>> +++ b/include/hw/s390x/s390-virtio-ccw.h >>>> @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass { >>>> bool cpu_model_allowed; >>>> bool css_migration_enabled; >>>> bool hpage_1m_allowed; >>>> + bool kvm_ais_allowed; >>>> } S390CcwMachineClass; >>>> >>>> /* runtime-instrumentation allowed by the machine */ >>>> @@ -48,6 +49,8 @@ bool ri_allowed(void); >>>> bool cpu_model_allowed(void); >>>> /* 1M huge page mappings allowed by the machine */ >>>> bool hpage_1m_allowed(void); >>>> +/* adapter-interrupt suppression allowed by the machine? */ >>>> +bool kvm_ais_allowed(void); >>>> >>>> /** >>>> * Returns true if (vmstate based) migration of the channel subsystem >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>> index 15260aeb9a..1602a2c33d 100644 >>>> --- a/target/s390x/kvm.c >>>> +++ b/target/s390x/kvm.c >>>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >>>> /* >>>> * The migration interface for ais was introduced with kernel 4.13 >>>> * but the capability itself had been active since 4.12. As migration >>>> - * support is considered necessary let's disable ais in the 2.10 >>>> - * machine. >>>> + * support is considered necessary, we only try to enable this for >>>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. >>>> */ >>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>>> + if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() && >>>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>>> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >>>> + } >>> >>> I just remembered that the availability of CPU features should in >>> general not depend on the selected machine. We only have compat handling >>> for pre-cpu-model-support machines (e.g., vx). >>> >>> The issue is, that the probed host cpu model is otherwise not guaranteed >>> to run om a selected machine and you get misleading errors. >>> >>> Can someone remind me why we need kvm_ais_allowed() at all and cannot >>> simply rely on cpu model checks to properly handle this (like all other >>> features)? >> >> Sorry, I don't quite get what you mean here. Which other features are >> you talking about? KVM_CAP_S390_RI and KVM_CAP_S390_GS are enabled in a >> very similar way... > Similar but different. > > If you look closely, that was all being done before we had machines with > CPU model support > > 2.8 introduced CPU models > 2.7 introduced RI > > So ri_allowed() is in place because we have to handle compatibility > thingies with machines before we had cpu_model_allowed(). > > Basically, cpu model support invalidated the need for all new flags. > You can see how it is to be done for KVM_CAP_S390_GS > > So this here should be > > if (cpu_model_allowed() && kvm_kernel_irqchip_allowed() ... > > We do have to make sure that all migration-related things are also glued > to the actual availability of the feature. Usually, there should be a > s390_has_feat(ADAPTER_INT_SUPPRESSION) somewhere that guards this (e.g., > see target/s390x/machine.c:vregs_needed()). FWIW, migration should be fine already fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION); and bool ais_needed(void *opaque) { S390FLICState *s = opaque; return s->ais_supported; }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index e0e28139a2..76254e8447 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data) s390mc->cpu_model_allowed = true; s390mc->css_migration_enabled = true; s390mc->hpage_1m_allowed = true; + s390mc->kvm_ais_allowed = true; mc->init = ccw_init; mc->reset = s390_machine_reset; mc->hot_add_cpu = s390_hot_add_cpu; @@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value, static S390CcwMachineClass *current_mc; +/* + * Get the class of the s390-ccw-virtio machine that is currently in use. + * Note: libvirt is using the "none" machine to probe for the features of the + * host CPU, so in case this is called with the "none" machine, the function + * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the + * various "*_allowed" variables are enabled, so that the *_allowed() wrappers + * below return the correct default value for the "none" machine. + */ static S390CcwMachineClass *get_machine_class(void) { if (unlikely(!current_mc)) { @@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void) bool ri_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->ri_allowed; } bool cpu_model_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->cpu_model_allowed; } bool hpage_1m_allowed(void) { - /* for "none" machine this results in true */ return get_machine_class()->hpage_1m_allowed; } +bool kvm_ais_allowed(void) +{ + return get_machine_class()->kvm_ais_allowed; +} + static char *machine_get_loadparm(Object *obj, Error **errp) { S390CcwMachineState *ms = S390_CCW_MACHINE(obj); @@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine) static void ccw_machine_4_2_class_options(MachineClass *mc) { + S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc); + ccw_machine_5_0_class_options(mc); compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); + s390mc->kvm_ais_allowed = false; } DEFINE_CCW_MACHINE(4_2, "4.2", false); diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h index 8aa27199c9..e3ba3b88b1 100644 --- a/include/hw/s390x/s390-virtio-ccw.h +++ b/include/hw/s390x/s390-virtio-ccw.h @@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass { bool cpu_model_allowed; bool css_migration_enabled; bool hpage_1m_allowed; + bool kvm_ais_allowed; } S390CcwMachineClass; /* runtime-instrumentation allowed by the machine */ @@ -48,6 +49,8 @@ bool ri_allowed(void); bool cpu_model_allowed(void); /* 1M huge page mappings allowed by the machine */ bool hpage_1m_allowed(void); +/* adapter-interrupt suppression allowed by the machine? */ +bool kvm_ais_allowed(void); /** * Returns true if (vmstate based) migration of the channel subsystem diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 15260aeb9a..1602a2c33d 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s) /* * The migration interface for ais was introduced with kernel 4.13 * but the capability itself had been active since 4.12. As migration - * support is considered necessary let's disable ais in the 2.10 - * machine. + * support is considered necessary, we only try to enable this for + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. */ - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ + if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() && + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); + } kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES); return 0;