| Message ID | 20200116122026.5804-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/s390x/kvm: Enable adapter interruption suppression again | expand |
On 16.01.20 13:20, 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. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++++ > include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > target/s390x/kvm.c | 11 ++++++++--- > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e7eadd14e8..6f43136396 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -456,6 +456,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; > @@ -662,6 +663,9 @@ 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); > + > + s390mc->kvm_ais_allowed = false; > ccw_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > } > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h > index 8aa27199c9..f142d379c6 100644 > --- a/include/hw/s390x/s390-virtio-ccw.h > +++ b/include/hw/s390x/s390-virtio-ccw.h > @@ -21,6 +21,9 @@ > #define S390_MACHINE_CLASS(klass) \ > OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) > > +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) > + > typedef struct S390CcwMachineState { > /*< private >*/ > MachineState parent_obj; > @@ -40,6 +43,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 */ > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 15260aeb9a..4c1c8c0208 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) > > int kvm_arch_init(MachineState *ms, KVMState *s) > { > + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); > + > object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, > false, NULL); > > @@ -365,10 +367,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 enable this for newer > + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. > */ > - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ > + if (smc->kvm_ais_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; > We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed(). Care to create a similar wrapper? apart from that Reviewed-by: David Hildenbrand <david@redhat.com>
On 16/01/2020 13.23, David Hildenbrand wrote: > On 16.01.20 13:20, 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. >> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 4 ++++ >> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >> target/s390x/kvm.c | 11 ++++++++--- >> 3 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index e7eadd14e8..6f43136396 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -456,6 +456,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; >> @@ -662,6 +663,9 @@ 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); >> + >> + s390mc->kvm_ais_allowed = false; >> ccw_machine_5_0_class_options(mc); >> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >> } >> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >> index 8aa27199c9..f142d379c6 100644 >> --- a/include/hw/s390x/s390-virtio-ccw.h >> +++ b/include/hw/s390x/s390-virtio-ccw.h >> @@ -21,6 +21,9 @@ >> #define S390_MACHINE_CLASS(klass) \ >> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) >> >> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) >> + >> typedef struct S390CcwMachineState { >> /*< private >*/ >> MachineState parent_obj; >> @@ -40,6 +43,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 */ >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 15260aeb9a..4c1c8c0208 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >> >> int kvm_arch_init(MachineState *ms, KVMState *s) >> { >> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >> + >> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, >> false, NULL); >> >> @@ -365,10 +367,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 enable this for newer >> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. >> */ >> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >> + if (smc->kvm_ais_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; >> > > We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed(). > > Care to create a similar wrapper? Honestly, why do we need these wrappers at all? They look cumbersome to me. I'd rather remove them in case they are not urgently needed (so far I don't see the point... could someone enlighten me why we have them?). Thomas
On 16.01.20 13:26, Thomas Huth wrote: > On 16/01/2020 13.23, David Hildenbrand wrote: >> On 16.01.20 13:20, 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. >>> >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>> target/s390x/kvm.c | 11 ++++++++--- >>> 3 files changed, 16 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>> index e7eadd14e8..6f43136396 100644 >>> --- a/hw/s390x/s390-virtio-ccw.c >>> +++ b/hw/s390x/s390-virtio-ccw.c >>> @@ -456,6 +456,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; >>> @@ -662,6 +663,9 @@ 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); >>> + >>> + s390mc->kvm_ais_allowed = false; >>> ccw_machine_5_0_class_options(mc); >>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >>> } >>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >>> index 8aa27199c9..f142d379c6 100644 >>> --- a/include/hw/s390x/s390-virtio-ccw.h >>> +++ b/include/hw/s390x/s390-virtio-ccw.h >>> @@ -21,6 +21,9 @@ >>> #define S390_MACHINE_CLASS(klass) \ >>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) >>> >>> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ >>> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) >>> + >>> typedef struct S390CcwMachineState { >>> /*< private >*/ >>> MachineState parent_obj; >>> @@ -40,6 +43,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 */ >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 15260aeb9a..4c1c8c0208 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >>> >>> int kvm_arch_init(MachineState *ms, KVMState *s) >>> { >>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >>> + >>> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, >>> false, NULL); >>> >>> @@ -365,10 +367,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 enable this for newer >>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. >>> */ >>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>> + if (smc->kvm_ais_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; >>> >> >> We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed(). >> >> Care to create a similar wrapper? > > Honestly, why do we need these wrappers at all? They look cumbersome to > me. I'd rather remove them in case they are not urgently needed (so far > I don't see the point... could someone enlighten me why we have them?). I assume to minimize the number of places you have to lookup the machine/machine class.
On 16/01/2020 13.28, David Hildenbrand wrote: > On 16.01.20 13:26, Thomas Huth wrote: >> On 16/01/2020 13.23, David Hildenbrand wrote: >>> On 16.01.20 13:20, 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. >>>> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>>> target/s390x/kvm.c | 11 ++++++++--- >>>> 3 files changed, 16 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >>>> index e7eadd14e8..6f43136396 100644 >>>> --- a/hw/s390x/s390-virtio-ccw.c >>>> +++ b/hw/s390x/s390-virtio-ccw.c >>>> @@ -456,6 +456,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; >>>> @@ -662,6 +663,9 @@ 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); >>>> + >>>> + s390mc->kvm_ais_allowed = false; >>>> ccw_machine_5_0_class_options(mc); >>>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); >>>> } >>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h >>>> index 8aa27199c9..f142d379c6 100644 >>>> --- a/include/hw/s390x/s390-virtio-ccw.h >>>> +++ b/include/hw/s390x/s390-virtio-ccw.h >>>> @@ -21,6 +21,9 @@ >>>> #define S390_MACHINE_CLASS(klass) \ >>>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) >>>> >>>> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ >>>> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) >>>> + >>>> typedef struct S390CcwMachineState { >>>> /*< private >*/ >>>> MachineState parent_obj; >>>> @@ -40,6 +43,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 */ >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>> index 15260aeb9a..4c1c8c0208 100644 >>>> --- a/target/s390x/kvm.c >>>> +++ b/target/s390x/kvm.c >>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >>>> >>>> int kvm_arch_init(MachineState *ms, KVMState *s) >>>> { >>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >>>> + >>>> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, >>>> false, NULL); >>>> >>>> @@ -365,10 +367,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 enable this for newer >>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. >>>> */ >>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>>> + if (smc->kvm_ais_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; >>>> >>> >>> We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed(). >>> >>> Care to create a similar wrapper? >> >> Honestly, why do we need these wrappers at all? They look cumbersome to >> me. I'd rather remove them in case they are not urgently needed (so far >> I don't see the point... could someone enlighten me why we have them?). > > I assume to minimize the number of places you have to lookup the > machine/machine class. I don't think that any of these functions is performance critical, since they are only used during the initialization phase... But looking more closely, cpu_model_allowed() and hpage_1m_allowed() are used in functions where the current machine state / class is not directly available, so the wrappers indeed make sense there. We could remove the ri_allowed() wrapper, though, since this is also only used in kvm_arch_init() where the machine state is easily available. Thomas
On Thu, 16 Jan 2020 13:20:26 +0100 Thomas Huth <thuth@redhat.com> 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. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++++ > include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > target/s390x/kvm.c | 11 ++++++++--- > 3 files changed, 16 insertions(+), 3 deletions(-) > > @@ -365,10 +367,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 enable this for newer s/necessary we only enable this/necessary, we only try to enable this/ > + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. maybe s/and if/if/ > */ > - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ > + if (smc->kvm_ais_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; Looks good. Remind me again: ais only made a difference for pci devices, right? Is it enough to give this a quick whirl with virtio-pci devices?
On 16/01/2020 13.50, Cornelia Huck wrote: > On Thu, 16 Jan 2020 13:20:26 +0100 > Thomas Huth <thuth@redhat.com> 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. >> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 4 ++++ >> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >> target/s390x/kvm.c | 11 ++++++++--- >> 3 files changed, 16 insertions(+), 3 deletions(-) >> > >> @@ -365,10 +367,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 enable this for newer > > s/necessary we only enable this/necessary, we only try to enable this/ > >> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. > > maybe s/and if/if/ Sure ... could you fix it up when picking up the patch (in case I don't have to respin), or do you want me to send a v2? >> */ >> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >> + if (smc->kvm_ais_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; > > Looks good. > > Remind me again: ais only made a difference for pci devices, right? Is > it enough to give this a quick whirl with virtio-pci devices? I don't remember the details, Christian, could you please answer this question? Thomas
On 16.01.20 13:55, Thomas Huth wrote: > On 16/01/2020 13.50, Cornelia Huck wrote: >> On Thu, 16 Jan 2020 13:20:26 +0100 >> Thomas Huth <thuth@redhat.com> 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. >>> >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>> target/s390x/kvm.c | 11 ++++++++--- >>> 3 files changed, 16 insertions(+), 3 deletions(-) >>> >> >>> @@ -365,10 +367,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 enable this for newer >> >> s/necessary we only enable this/necessary, we only try to enable this/ >> >>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. >> >> maybe s/and if/if/ > > Sure ... could you fix it up when picking up the patch (in case I don't > have to respin), or do you want me to send a v2? > >>> */ >>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>> + if (smc->kvm_ais_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; >> >> Looks good. >> >> Remind me again: ais only made a difference for pci devices, right? Is >> it enough to give this a quick whirl with virtio-pci devices? > > I don't remember the details, Christian, could you please answer this > question? Yes, IIRC AIS was there for PCI, but not for Crypto or virtio. The patch looks sane, but it would be good if someone could try the AIS stuff. Matt, can you have a look?
On Thu, 16 Jan 2020 14:22:15 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 16.01.20 13:55, Thomas Huth wrote: > > On 16/01/2020 13.50, Cornelia Huck wrote: > >> On Thu, 16 Jan 2020 13:20:26 +0100 > >> Thomas Huth <thuth@redhat.com> 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. > >>> > >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > >>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>> --- > >>> hw/s390x/s390-virtio-ccw.c | 4 ++++ > >>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > >>> target/s390x/kvm.c | 11 ++++++++--- > >>> 3 files changed, 16 insertions(+), 3 deletions(-) > >>> > >> > >>> @@ -365,10 +367,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 enable this for newer > >> > >> s/necessary we only enable this/necessary, we only try to enable this/ > >> > >>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. > >> > >> maybe s/and if/if/ > > > > Sure ... could you fix it up when picking up the patch (in case I don't > > have to respin), or do you want me to send a v2? Sure, can do that.
On 1/16/20 8:22 AM, Christian Borntraeger wrote: > > > On 16.01.20 13:55, Thomas Huth wrote: >> On 16/01/2020 13.50, Cornelia Huck wrote: >>> On Thu, 16 Jan 2020 13:20:26 +0100 >>> Thomas Huth <thuth@redhat.com> 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. >>>> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>>> target/s390x/kvm.c | 11 ++++++++--- >>>> 3 files changed, 16 insertions(+), 3 deletions(-) >>>> >>> >>>> @@ -365,10 +367,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 enable this for newer >>> >>> s/necessary we only enable this/necessary, we only try to enable this/ >>> >>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. >>> >>> maybe s/and if/if/ >> >> Sure ... could you fix it up when picking up the patch (in case I don't >> have to respin), or do you want me to send a v2? >> >>>> */ >>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ >>>> + if (smc->kvm_ais_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; >>> >>> Looks good. >>> >>> Remind me again: ais only made a difference for pci devices, right? Is >>> it enough to give this a quick whirl with virtio-pci devices? >> >> I don't remember the details, Christian, could you please answer this >> question? > > Yes, IIRC AIS was there for PCI, but not for Crypto or virtio. This matches my understanding as well. > The patch looks sane, but it would be good if someone could try > the AIS stuff. > > Matt, can you have a look? Sure. But my PCI environment is currently down for a maintenance window, will try again later today.
On 1/16/20 7:20 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. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 4 ++++ > include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > target/s390x/kvm.c | 11 ++++++++--- > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index e7eadd14e8..6f43136396 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -456,6 +456,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; > @@ -662,6 +663,9 @@ 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); > + > + s390mc->kvm_ais_allowed = false; > ccw_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > } > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h > index 8aa27199c9..f142d379c6 100644 > --- a/include/hw/s390x/s390-virtio-ccw.h > +++ b/include/hw/s390x/s390-virtio-ccw.h > @@ -21,6 +21,9 @@ > #define S390_MACHINE_CLASS(klass) \ > OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) > > +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) > + > typedef struct S390CcwMachineState { > /*< private >*/ > MachineState parent_obj; > @@ -40,6 +43,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 */ > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 15260aeb9a..4c1c8c0208 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) > > int kvm_arch_init(MachineState *ms, KVMState *s) > { > + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); > + I still can't run a proper test due to unavailable hw but in the meantime I tried to virsh define a libvirt guest pointed at qemu (master + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or s390-ccw-virtio-4.2) I get: virsh define guest.xml error: Failed to define domain from /path/to/guest.xml error: invalid argument: could not find capabilities for arch=s390x domaintype=kvm Similarly: virsh domcapabilities error: failed to get emulator capabilities error: invalid argument: unable to find any emulator to serve 's390x' architecture Rolling back to qemu master, the define and domcapabilities work (with no ais of course). So: there is some incompatibility between the way libvirt invokes qemu to detect capabilities and this code. The above line seems to be the root problem - if I take your patch and remove 'smc' then libvirt works as expected and I can see ais in the domcapabilities. Looking at those wrappers David mentioned... I suspect you need this for the 'none' machine case. I tried a quick hack with the following: bool ais_allowed(void) { /* for "none" machine this results in true */ return get_machine_class()->kvm_ais_allowed; } and if (ais_allowed() && kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); } This works and doesn't break libvirt compatibility detection. > object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, > false, NULL); > > @@ -365,10 +367,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 enable this for newer > + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. > */ > - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ > + if (smc->kvm_ais_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; >
On Thu, 16 Jan 2020 15:19:13 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 1/16/20 7:20 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. > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > hw/s390x/s390-virtio-ccw.c | 4 ++++ > > include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > > target/s390x/kvm.c | 11 ++++++++--- > > 3 files changed, 16 insertions(+), 3 deletions(-) (...) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > index 15260aeb9a..4c1c8c0208 100644 > > --- a/target/s390x/kvm.c > > +++ b/target/s390x/kvm.c > > @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) > > > > int kvm_arch_init(MachineState *ms, KVMState *s) > > { > > + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); > > + > > I still can't run a proper test due to unavailable hw but in the > meantime I tried to virsh define a libvirt guest pointed at qemu (master > + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or > s390-ccw-virtio-4.2) I get: > > virsh define guest.xml > error: Failed to define domain from /path/to/guest.xml > error: invalid argument: could not find capabilities for arch=s390x > domaintype=kvm > > Similarly: > > virsh domcapabilities > error: failed to get emulator capabilities > error: invalid argument: unable to find any emulator to serve 's390x' > architecture > > Rolling back to qemu master, the define and domcapabilities work (with > no ais of course). > > So: there is some incompatibility between the way libvirt invokes qemu > to detect capabilities and this code. The above line seems to be the > root problem - if I take your patch and remove 'smc' then libvirt works > as expected and I can see ais in the domcapabilities. > > Looking at those wrappers David mentioned... I suspect you need this > for the 'none' machine case. I tried a quick hack with the following: > > bool ais_allowed(void) > { > /* for "none" machine this results in true */ > return get_machine_class()->kvm_ais_allowed; > } > > and > > if (ais_allowed() && > kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { > kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); > } > > This works and doesn't break libvirt compatibility detection. Oh, "none" machine fun again... I think you're on the right track, and we really need a wrapper.
On 16/01/2020 21.26, Cornelia Huck wrote: > On Thu, 16 Jan 2020 15:19:13 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 1/16/20 7:20 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. >>> >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>> target/s390x/kvm.c | 11 ++++++++--- >>> 3 files changed, 16 insertions(+), 3 deletions(-) > > (...) > >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 15260aeb9a..4c1c8c0208 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >>> >>> int kvm_arch_init(MachineState *ms, KVMState *s) >>> { >>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >>> + >> >> I still can't run a proper test due to unavailable hw but in the >> meantime I tried to virsh define a libvirt guest pointed at qemu (master >> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or >> s390-ccw-virtio-4.2) I get: >> >> virsh define guest.xml >> error: Failed to define domain from /path/to/guest.xml >> error: invalid argument: could not find capabilities for arch=s390x >> domaintype=kvm >> >> Similarly: >> >> virsh domcapabilities >> error: failed to get emulator capabilities >> error: invalid argument: unable to find any emulator to serve 's390x' >> architecture >> >> Rolling back to qemu master, the define and domcapabilities work (with >> no ais of course). >> >> So: there is some incompatibility between the way libvirt invokes qemu >> to detect capabilities and this code. The above line seems to be the >> root problem - if I take your patch and remove 'smc' then libvirt works >> as expected and I can see ais in the domcapabilities. >> >> Looking at those wrappers David mentioned... I suspect you need this >> for the 'none' machine case. I tried a quick hack with the following: >> >> bool ais_allowed(void) >> { >> /* for "none" machine this results in true */ >> return get_machine_class()->kvm_ais_allowed; >> } >> >> and >> >> if (ais_allowed() && >> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >> } >> >> This works and doesn't break libvirt compatibility detection. > > Oh, "none" machine fun again... I think you're on the right track, and > we really need a wrapper. D'oh, so this is the real reason for the wrappers ... ok, I'll respin my patch accordingly. Thanks a lot for the testing, Matthew! Thomas
On 17.01.20 11:38, Thomas Huth wrote: > On 16/01/2020 21.26, Cornelia Huck wrote: >> On Thu, 16 Jan 2020 15:19:13 -0500 >> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >> >>> On 1/16/20 7:20 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. >>>> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>>> target/s390x/kvm.c | 11 ++++++++--- >>>> 3 files changed, 16 insertions(+), 3 deletions(-) >> >> (...) >> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>> index 15260aeb9a..4c1c8c0208 100644 >>>> --- a/target/s390x/kvm.c >>>> +++ b/target/s390x/kvm.c >>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >>>> >>>> int kvm_arch_init(MachineState *ms, KVMState *s) >>>> { >>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >>>> + >>> >>> I still can't run a proper test due to unavailable hw but in the >>> meantime I tried to virsh define a libvirt guest pointed at qemu (master >>> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or >>> s390-ccw-virtio-4.2) I get: >>> >>> virsh define guest.xml >>> error: Failed to define domain from /path/to/guest.xml >>> error: invalid argument: could not find capabilities for arch=s390x >>> domaintype=kvm >>> >>> Similarly: >>> >>> virsh domcapabilities >>> error: failed to get emulator capabilities >>> error: invalid argument: unable to find any emulator to serve 's390x' >>> architecture >>> >>> Rolling back to qemu master, the define and domcapabilities work (with >>> no ais of course). >>> >>> So: there is some incompatibility between the way libvirt invokes qemu >>> to detect capabilities and this code. The above line seems to be the >>> root problem - if I take your patch and remove 'smc' then libvirt works >>> as expected and I can see ais in the domcapabilities. >>> >>> Looking at those wrappers David mentioned... I suspect you need this >>> for the 'none' machine case. I tried a quick hack with the following: >>> >>> bool ais_allowed(void) >>> { >>> /* for "none" machine this results in true */ >>> return get_machine_class()->kvm_ais_allowed; >>> } >>> >>> and >>> >>> if (ais_allowed() && >>> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >>> } >>> >>> This works and doesn't break libvirt compatibility detection. >> >> Oh, "none" machine fun again... I think you're on the right track, and >> we really need a wrapper. > > D'oh, so this is the real reason for the wrappers ... ok, I'll respin my > patch accordingly. Can you add a comment to the wrappers? > > Thanks a lot for the testing, Matthew! > > Thomas >
On 17/01/2020 12.05, Christian Borntraeger wrote: > > > On 17.01.20 11:38, Thomas Huth wrote: >> On 16/01/2020 21.26, Cornelia Huck wrote: >>> On Thu, 16 Jan 2020 15:19:13 -0500 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> On 1/16/20 7:20 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. >>>>> >>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> hw/s390x/s390-virtio-ccw.c | 4 ++++ >>>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ >>>>> target/s390x/kvm.c | 11 ++++++++--- >>>>> 3 files changed, 16 insertions(+), 3 deletions(-) >>> >>> (...) >>> >>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>>> index 15260aeb9a..4c1c8c0208 100644 >>>>> --- a/target/s390x/kvm.c >>>>> +++ b/target/s390x/kvm.c >>>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) >>>>> >>>>> int kvm_arch_init(MachineState *ms, KVMState *s) >>>>> { >>>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); >>>>> + >>>> >>>> I still can't run a proper test due to unavailable hw but in the >>>> meantime I tried to virsh define a libvirt guest pointed at qemu (master >>>> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or >>>> s390-ccw-virtio-4.2) I get: >>>> >>>> virsh define guest.xml >>>> error: Failed to define domain from /path/to/guest.xml >>>> error: invalid argument: could not find capabilities for arch=s390x >>>> domaintype=kvm >>>> >>>> Similarly: >>>> >>>> virsh domcapabilities >>>> error: failed to get emulator capabilities >>>> error: invalid argument: unable to find any emulator to serve 's390x' >>>> architecture >>>> >>>> Rolling back to qemu master, the define and domcapabilities work (with >>>> no ais of course). >>>> >>>> So: there is some incompatibility between the way libvirt invokes qemu >>>> to detect capabilities and this code. The above line seems to be the >>>> root problem - if I take your patch and remove 'smc' then libvirt works >>>> as expected and I can see ais in the domcapabilities. >>>> >>>> Looking at those wrappers David mentioned... I suspect you need this >>>> for the 'none' machine case. I tried a quick hack with the following: >>>> >>>> bool ais_allowed(void) >>>> { >>>> /* for "none" machine this results in true */ >>>> return get_machine_class()->kvm_ais_allowed; >>>> } >>>> >>>> and >>>> >>>> if (ais_allowed() && >>>> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>>> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >>>> } >>>> >>>> This works and doesn't break libvirt compatibility detection. >>> >>> Oh, "none" machine fun again... I think you're on the right track, and >>> we really need a wrapper. >> >> D'oh, so this is the real reason for the wrappers ... ok, I'll respin my >> patch accordingly. > > > Can you add a comment to the wrappers? Yes, good idea, I'll do that! Thomas
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index e7eadd14e8..6f43136396 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -456,6 +456,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; @@ -662,6 +663,9 @@ 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); + + s390mc->kvm_ais_allowed = false; ccw_machine_5_0_class_options(mc); compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); } diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h index 8aa27199c9..f142d379c6 100644 --- a/include/hw/s390x/s390-virtio-ccw.h +++ b/include/hw/s390x/s390-virtio-ccw.h @@ -21,6 +21,9 @@ #define S390_MACHINE_CLASS(klass) \ OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \ + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE) + typedef struct S390CcwMachineState { /*< private >*/ MachineState parent_obj; @@ -40,6 +43,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 */ diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 15260aeb9a..4c1c8c0208 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque) int kvm_arch_init(MachineState *ms, KVMState *s) { + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms); + object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE, false, NULL); @@ -365,10 +367,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 enable this for newer + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. */ - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */ + if (smc->kvm_ais_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;
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. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 4 ++++ include/hw/s390x/s390-virtio-ccw.h | 4 ++++ target/s390x/kvm.c | 11 ++++++++--- 3 files changed, 16 insertions(+), 3 deletions(-)