On Tue, 21 Jan 2020 11:05:18 -0500 Matthew Rosato <mjros...@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 <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. 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?
