Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again

[Thomas Huth <thuth@redhat.com>]

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