Re: [RFC PATCH] spapr: Add SPAPR_CAP_AIL_MODES for supported AIL modes for H_SET_MODE hcall

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of February 1, 2022 1:51 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> The behaviour of the Address Translation Mode on Interrupt resource is
>> not consistently supported by all CPU versions or all KVM versions.  In
>> particular KVM HV only supports mode 0 on POWER7 processors, and does
>> not support mode 2 on any processors. KVM PR only supports mode 0. TCG
>> can support all modes (0,2,3).
>>
>> This leads to inconsistencies in guest behaviour and could cause
>> problems migrating guests.
>>
>> This was not too noticable for Linux guests for a long time because the
>> kernel only used mode 0 or 3, and it used to consider AIL to be somewhat
>> advisory (KVM would not always honor it either) and it kept both sets of
>> interrupt vectors around.
>>
>> Recent Linux guests depend on the AIL mode working as defined by the ISA
>> to support the SCV facility interrupt. If AIL mode 3 can not be provided,
>> then Linux must be given an error so it can disable the SCV facility.
>>
>> Add the ail-modes capability which is a bitmap of the supported values
>> for the H_SET_MODE Address Translation Mode on Interrupt resource. Add
>> a new KVM CAP that exports the same thing, and provide defaults for PR
>> and HV KVM that predate the cap.
>> ---
>>
>> I just wanted to get some feedback on the approach before submitting a
>> patch for the KVM cap.
> 
> Could you expand a bit on what is the use case for setting this in the
> QEMU cmdline? I looks to me we already have all the information we need
> with just the KVM cap.

To be able to match TCG with KVM HV or PR behaviour here.
I guess I'm not sure how much that is actually needed though.

>> +    if (kvm_enabled()) {
>> +        if (val & (0x01 << 2)) {
>> +            error_setg(errp, "KVM does not support cap-ail-modes mode 
>> AIL=2");
> 
> Isn't this something KVM should tell us via the capability?

Yeah, might as well do that. I changed some of the interfaces halfway
through and didn't clean this up.

>> +            error_append_hint(errp,
>> +                              "Ensure bit 2 (value 4) is clear in 
>> cap-ail-modes\n");
>> +            if (kvmppc_has_cap_ail_3()) {
>> +                error_append_hint(errp, "Try appending -machine 
>> cap-ail-modes=9\n");
>> +            } else {
>> +                error_append_hint(errp, "Try appending -machine 
>> cap-ail-modes=1\n");
>> +            }
>> +            return;
>> +        }
>> +        if ((val & (0x01 << 3)) && !kvmppc_has_cap_ail_3()) {
>> +            error_setg(errp, "KVM implementation does not support 
>> cap-ail-modes AIL=3");
>> +            error_append_hint(errp,
>> +                              "Ensure bit 3 (value 8) is clear in 
>> cap-ail-modes\n");
>> +            error_append_hint(errp, "Try appending -machine 
>> cap-ail-modes=1\n");
>> +            return;
>> +        }
>> +    }
>> +}
> 
> I think the error reporting here is too complex. A user who just wants
> to make their guest start will not bother thinking about binary
> representation. There's also some room for confusion in having three
> numbers present in the error message (bit #, decimal value and AIL
> mode). Imagine dealing with this in a bug report, for instance.
> 
> I would just tell outright what the supported values are. Perhaps in a
> little table:
> 
> Supported AIL modes:
>  AIL = 0   | cap-ail-modes=1
>  AIL = 2   | cap-ail-modes=5
>  AIL = 3   | cap-ail-modes=9
>  AIL = 2&3 | cap-ail-modes=13
> 
> We could then make the code a bit more generic. Roughly:

Yeah I didn't like the interface either :P

The nicest option I guess is to be able to give it a list

cap-ail-modes=0,2,3

Maybe there's already some parsing to be able to do that. I'll
look a bit harder.

Thanks,
Nick