Re: [PATCH 1/5] ppc/spapr: KVM FWNMI should not be enabled until guest requests it

[Nicholas Piggin]

Greg Kurz's on March 17, 2020 9:02 pm:
> On Tue, 17 Mar 2020 15:02:11 +1000
> Nicholas Piggin <address@hidden> wrote:
> 
>> The KVM FWNMI capability should be enabled with the "ibm,nmi-register"
>> rtas call. Although MCEs from KVM will be delivered as architected
>> interrupts to the guest before "ibm,nmi-register" is called, KVM has
>> different behaviour depending on whether the guest has enabled FWNMI
>> (it attempts to do more recovery on behalf of a non-FWNMI guest).
>> 
>> Signed-off-by: Nicholas Piggin <address@hidden>
>> ---
>>  hw/ppc/spapr_caps.c  | 5 +++--
>>  hw/ppc/spapr_rtas.c  | 7 +++++++
>>  target/ppc/kvm.c     | 7 +++++++
>>  target/ppc/kvm_ppc.h | 6 ++++++
>>  4 files changed, 23 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 679ae7959f..eb5521d0c2 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -517,9 +517,10 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
>> uint8_t val,
>>      }
>>  
>>      if (kvm_enabled()) {
>> -        if (kvmppc_set_fwnmi() < 0) {
>> +        if (!kvmppc_get_fwnmi()) {
>>              error_setg(errp, "Firmware Assisted Non-Maskable 
>> Interrupts(FWNMI) "
>> -                             "not supported by KVM");
>> +                             "not supported by KVM, "
>> +                             "try appending -machine cap-fwnmi=off");
> 
> It is usually preferred to keep error message strings on one
> line for easier grepping. Also hints should be specified with
> error_append_hint() because they are treated differently by
> QMP (ie. not printed).
> 
> Something like:
> 
>         if (!kvmppc_get_fwnmi()) {
>             error_setg(errp,
>        "Firmware Assisted Non-Maskable Interrupts(FWNMI) not supported by 
> KVM");
>             error_append_hint(errp, "Try appending -machine cap-fwnmi=off\n");
>         }

Hmm, okay.

> 
> Note that the current error handling code has an issue that
> prevents hints to be printed when errp == &error_fatal, which
> is exactly what spapr_caps_apply() does. Since this affects
> a lot of locations in the code base, there's an on-going
> effort to fix that globally:
> 
> https://patchwork.ozlabs.org/project/qemu-devel/list/?series=163907
> 
> I don't know if this will make it for 5.0, but in any case I
> think you should call error_append_hint() in this patch anyway
> and the code will just work at some later point.
> 
> Rest looks good.

Thanks will do,
Nick