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

[Thomas Huth]

On 20/01/2020 11.30, Cornelia Huck wrote:
> On Mon, 20 Jan 2020 10:49:01 +0100
> Thomas Huth <address@hidden> 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
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  Matthew, could you please give this another try on your system? Thanks!
>>
>>  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(-)
>>
> 
>> @@ -658,6 +669,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);
> 
> One thing I've noticed: You set the _allowed value to false, and
> afterwards apply the options from any 'later' class; this is the same
> order as for the other _allowed values. There's also
> css_migration_enabled, which is set to false _after_ the class options
> from 'later' classes have been applied.
> 
> Both variants end up the same, as we only ever set the value to true in
> the base class and to false just in a single class option callback; but
> I wonder whether it would be cleaner to set it to false after the other
> options have been applied. Or am I thinking backwards here?

You're right, it should not matter right now, but it might be better
(from the copy-n-paste perspective) / more future-proof to do it the
other way round. I'll send a v3...

 Thomas