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 <mjros...@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 <th...@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 <da...@redhat.com> >>>>>> Signed-off-by: Thomas Huth <th...@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
