| Message ID | 20200120132441.11884-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/s390x/kvm: Enable adapter interruption suppression again | expand |
On 1/20/20 8:24 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> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- Took it for a spin with vfio-pci. With this patch applied, I see the appropriate change reflected in guest /proc/cpuinfo. I did some tracing and see the expected behavior changes (ex: hits in host kvm_s390_injrect_airq that show suppression occurring). Data transfer tests worked fine. Also sanity-tested that ais=off behaves as expected. Looks good to me.
On Mon, 20 Jan 2020 14:24:41 +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. > > 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> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function We also might want to move the others in a followup patch, just to avoid bad examples to copy/paste. > > 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(-) Looks good to me, will queue when I get a positive test result.
On Mon, 20 Jan 2020 11:23:37 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 1/20/20 8:24 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> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > Took it for a spin with vfio-pci. With this patch applied, I see the > appropriate change reflected in guest /proc/cpuinfo. I did some tracing > and see the expected behavior changes (ex: hits in host > kvm_s390_injrect_airq that show suppression occurring). Data transfer > tests worked fine. Also sanity-tested that ais=off behaves as expected. > > Looks good to me. > Excellent, thanks for testing! Should I add a Tested-by: ?
On 1/20/20 11:29 AM, Cornelia Huck wrote: > On Mon, 20 Jan 2020 11:23:37 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 1/20/20 8:24 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> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >> >> Took it for a spin with vfio-pci. With this patch applied, I see the >> appropriate change reflected in guest /proc/cpuinfo. I did some tracing >> and see the expected behavior changes (ex: hits in host >> kvm_s390_injrect_airq that show suppression occurring). Data transfer >> tests worked fine. Also sanity-tested that ais=off behaves as expected. >> >> Looks good to me. >> > > Excellent, thanks for testing! > > Should I add a Tested-by: ? > Sure, go ahead. Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
On Mon, 20 Jan 2020 14:24:41 +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. > > 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> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function > > 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/target/s390x/kvm.c b/target/s390x/kvm.c > index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { Hnm, we actually need a kernel irqchip with the kvm flic to get ais to work; else we'll fail with qemu-system-s390x: Failed to inject airq with AIS supported in the kernel_irqchip=off case, as we won't have an I/O adapter registered. Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; comments? > + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); > + } > > kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES); > return 0;
On 1/20/20 12:27 PM, Cornelia Huck wrote: > On Mon, 20 Jan 2020 14:24:41 +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. >> >> 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> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function >> >> 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/target/s390x/kvm.c b/target/s390x/kvm.c >> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { > > Hnm, we actually need a kernel irqchip with the kvm flic to get ais to > work; else we'll fail with > > qemu-system-s390x: Failed to inject airq with AIS supported > > in the kernel_irqchip=off case, as we won't have an I/O adapter > registered. > > Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; > comments? > In spirit, I agree with this idea. But, a quick test shows that putting this check here results in ais=off for the 'none' machine case (libvirt capabilities detection). I think we have to only look at kvm_kernel_irqchip_required() when working with a real machine. >> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); >> + } >> >> kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES); >> return 0; > >
On Tue, 21 Jan 2020 09:33:02 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 1/20/20 12:27 PM, Cornelia Huck wrote: > > On Mon, 20 Jan 2020 14:24:41 +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. > >> > >> 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> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function > >> > >> 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/target/s390x/kvm.c b/target/s390x/kvm.c > >> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { > > > > Hnm, we actually need a kernel irqchip with the kvm flic to get ais to > > work; else we'll fail with > > > > qemu-system-s390x: Failed to inject airq with AIS supported > > > > in the kernel_irqchip=off case, as we won't have an I/O adapter > > registered. > > > > Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; > > comments? > > > > In spirit, I agree with this idea. But, a quick test shows that putting > this check here results in ais=off for the 'none' machine case (libvirt > capabilities detection). I think we have to only look at > kvm_kernel_irqchip_required() when working with a real machine. Sigh, I think you're right again. We need to check for the 'none' machine here; but I can't think of a non-ugly way to do so... > > >> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); > >> + } > >> > >> kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES); > >> return 0; > > > > >
On 21/01/2020 15.46, Cornelia Huck wrote: > On Tue, 21 Jan 2020 09:33:02 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 1/20/20 12:27 PM, Cornelia Huck wrote: >>> On Mon, 20 Jan 2020 14:24:41 +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. >>>> >>>> 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> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function >>>> >>>> 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/target/s390x/kvm.c b/target/s390x/kvm.c >>>> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>> >>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to >>> work; else we'll fail with >>> >>> qemu-system-s390x: Failed to inject airq with AIS supported >>> >>> in the kernel_irqchip=off case, as we won't have an I/O adapter >>> registered. >>> >>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; >>> comments? >>> >> >> In spirit, I agree with this idea. But, a quick test shows that putting >> this check here results in ais=off for the 'none' machine case (libvirt >> capabilities detection). I think we have to only look at >> kvm_kernel_irqchip_required() when working with a real machine. > > Sigh, I think you're right again. We need to check for the 'none' > machine here; but I can't think of a non-ugly way to do so... I think it might work when using kvm_kernel_irqchip_allowed() instead of kvm_kernel_irqchip_required() ... Matthew, could you please give it a try with this patch on top of mine: diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -368,7 +368,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) * support is considered necessary, we only try to enable this for * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available. */ - if (kvm_ais_allowed() && + 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); } Thomas
On 1/21/20 10:22 AM, Thomas Huth wrote: > On 21/01/2020 15.46, Cornelia Huck wrote: >> On Tue, 21 Jan 2020 09:33:02 -0500 >> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >> >>> On 1/20/20 12:27 PM, Cornelia Huck wrote: >>>> On Mon, 20 Jan 2020 14:24:41 +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. >>>>> >>>>> 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> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function >>>>> >>>>> 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/target/s390x/kvm.c b/target/s390x/kvm.c >>>>> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>>> >>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to >>>> work; else we'll fail with >>>> >>>> qemu-system-s390x: Failed to inject airq with AIS supported >>>> >>>> in the kernel_irqchip=off case, as we won't have an I/O adapter >>>> registered. >>>> >>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; >>>> comments? >>>> >>> >>> In spirit, I agree with this idea. But, a quick test shows that putting >>> this check here results in ais=off for the 'none' machine case (libvirt >>> capabilities detection). I think we have to only look at >>> kvm_kernel_irqchip_required() when working with a real machine. >> >> Sigh, I think you're right again. We need to check for the 'none' >> machine here; but I can't think of a non-ugly way to do so... > > I think it might work when using kvm_kernel_irqchip_allowed() instead of > kvm_kernel_irqchip_required() ... Matthew, could you please give it a > try with this patch on top of mine: > Sure. Libvirt detection works with this patch. Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get: qemu-system-s390x: Some features requested in the CPU model are not available in the configuration: ais Which was the same result as Connie's proposal. It reads a bit odd to me at first, but looking at the code quick I think this is the right answer - kvm_kernel_irqchip_allowed() will only return false when kernel_irqchip has been forced off as above, whereas kernel_irqchip_required will also return false in the case where no setting was specified (this is what tripped libvirt up). Looks good to me, thanks Thomas.
On 21/01/2020 17.05, Matthew Rosato wrote: > On 1/21/20 10:22 AM, Thomas Huth wrote: >> On 21/01/2020 15.46, Cornelia Huck wrote: >>> On Tue, 21 Jan 2020 09:33:02 -0500 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> On 1/20/20 12:27 PM, Cornelia Huck wrote: >>>>> On Mon, 20 Jan 2020 14:24:41 +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. >>>>>> >>>>>> 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> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>>> --- >>>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the >>>>>> function >>>>>> >>>>>> 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/target/s390x/kvm.c b/target/s390x/kvm.c >>>>>> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { >>>>> >>>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to >>>>> work; else we'll fail with >>>>> >>>>> qemu-system-s390x: Failed to inject airq with AIS supported >>>>> >>>>> in the kernel_irqchip=off case, as we won't have an I/O adapter >>>>> registered. >>>>> >>>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; >>>>> comments? >>>>> >>>> >>>> In spirit, I agree with this idea. But, a quick test shows that >>>> putting >>>> this check here results in ais=off for the 'none' machine case (libvirt >>>> capabilities detection). I think we have to only look at >>>> kvm_kernel_irqchip_required() when working with a real machine. >>> >>> Sigh, I think you're right again. We need to check for the 'none' >>> machine here; but I can't think of a non-ugly way to do so... >> >> I think it might work when using kvm_kernel_irqchip_allowed() instead of >> kvm_kernel_irqchip_required() ... Matthew, could you please give it a >> try with this patch on top of mine: >> > > Sure. > > Libvirt detection works with this patch. > > Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get: > qemu-system-s390x: Some features requested in the CPU model are not > available in the configuration: ais > > Which was the same result as Connie's proposal. > > It reads a bit odd to me at first, but looking at the code quick I think > this is the right answer - kvm_kernel_irqchip_allowed() will only return > false when kernel_irqchip has been forced off as above, whereas > kernel_irqchip_required will also return false in the case where no > setting was specified (this is what tripped libvirt up). > > Looks good to me, thanks Thomas. Great, thanks for testing! Cornelia, could you squash that kvm_kernel_irqchip_allowed() into the patch, or do you prefer if I send a v4 ? Thomas
On Tue, 21 Jan 2020 11:05:18 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 1/21/20 10:22 AM, Thomas Huth wrote: > > On 21/01/2020 15.46, Cornelia Huck wrote: > >> On Tue, 21 Jan 2020 09:33:02 -0500 > >> Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> > >>> On 1/20/20 12:27 PM, Cornelia Huck wrote: > >>>> On Mon, 20 Jan 2020 14:24:41 +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. > >>>>> > >>>>> 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> > >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>>>> --- > >>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function > >>>>> > >>>>> 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/target/s390x/kvm.c b/target/s390x/kvm.c > >>>>> index 15260aeb9a..cf4fb4f2d9 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_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) { > >>>> > >>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to > >>>> work; else we'll fail with > >>>> > >>>> qemu-system-s390x: Failed to inject airq with AIS supported > >>>> > >>>> in the kernel_irqchip=off case, as we won't have an I/O adapter > >>>> registered. > >>>> > >>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick; > >>>> comments? > >>>> > >>> > >>> In spirit, I agree with this idea. But, a quick test shows that putting > >>> this check here results in ais=off for the 'none' machine case (libvirt > >>> capabilities detection). I think we have to only look at > >>> kvm_kernel_irqchip_required() when working with a real machine. > >> > >> Sigh, I think you're right again. We need to check for the 'none' > >> machine here; but I can't think of a non-ugly way to do so... > > > > I think it might work when using kvm_kernel_irqchip_allowed() instead of > > kvm_kernel_irqchip_required() ... Matthew, could you please give it a > > try with this patch on top of mine: > > > > Sure. > > Libvirt detection works with this patch. Excellent. > > Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get: > qemu-system-s390x: Some features requested in the CPU model are not > available in the configuration: ais > > Which was the same result as Connie's proposal. Yep, that's the expected behaviour. > > It reads a bit odd to me at first, but looking at the code quick I think > this is the right answer - kvm_kernel_irqchip_allowed() will only return > false when kernel_irqchip has been forced off as above, whereas > kernel_irqchip_required will also return false in the case where no > setting was specified (this is what tripped libvirt up). > > Looks good to me, thanks Thomas. Thanks for testing! Thomas, I guess you'll send a v4?
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..cf4fb4f2d9 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_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;