Re: [PATCH v17 5/7] ppc: spapr: Handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls

[Ganesh <ganeshgr@linux.ibm.com>]

On 12/5/19 10:25 AM, Ganesh wrote:
>
> On 11/19/19 8:09 AM, David Gibson wrote:
>> On Thu, Oct 24, 2019 at 01:13:05PM +0530, Ganesh Goudar wrote:
>>> From: Aravinda Prasad <arawinda.p@gmail.com>
>>>
>>> This patch adds support in QEMU to handle "ibm,nmi-register"
>>> and "ibm,nmi-interlock" RTAS calls.
>>>
>>> The machine check notification address is saved when the
>>> OS issues "ibm,nmi-register" RTAS call.
>>>
>>> This patch also handles the case when multiple processors
>>> experience machine check at or about the same time by
>>> handling "ibm,nmi-interlock" call. In such cases, as per
>>> PAPR, subsequent processors serialize waiting for the first
>>> processor to issue the "ibm,nmi-interlock" call. The second
>>> processor that also received a machine check error waits
>>> till the first processor is done reading the error log.
>>> The first processor issues "ibm,nmi-interlock" call
>>> when the error log is consumed.
>>>
>>> [Move fwnmi registeration to .apply hook]
>> s/registeration/registration/
> Thanks
>>
>>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>>> Signed-off-by: Aravinda Prasad <arawinda.p@gmail.com>
>>> ---
>>>   hw/ppc/spapr_caps.c    |  9 +++++--
>>>   hw/ppc/spapr_rtas.c    | 57 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   include/hw/ppc/spapr.h |  5 +++-
>>>   3 files changed, 68 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index 976d709210..1675ebd45e 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -509,9 +509,14 @@ static void 
>>> cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
>>>            * of software injected faults like duplicate SLBs).
>>>            */
>>>           warn_report("Firmware Assisted Non-Maskable Interrupts not 
>>> supported in TCG");
>> This logic still isn't quite right.  To start with the warn_report()
>> above possible wants to be more weakly worded.  With TCG, FWNMI won't
>> generally *do* anything, and there are some edge cases where the
>> behaviour is arguably incorrect.  However there's no reason we can't
>> make the RTAS calls work basically as expected and in almost all cases
>> things will behave correctly - at least according to the case where no
>> fwnmi events are delivered...
> ok
>>
>>> -    } else if (kvm_enabled() && (kvmppc_set_fwnmi() != 0)) {
>>> -        error_setg(errp,
>>> +    } else if (kvm_enabled()) {
>>> +        if (!kvmppc_set_fwnmi()) {
>>> +            /* Register ibm,nmi-register and ibm,nmi-interlock RTAS 
>>> calls */
>>> +            spapr_fwnmi_register();
>> ..but here you only register the RTAS calls in the KVM case, which
>> breaks that.  If there really is a strong reason to do this, then the
>> warn_report() above should be error_setg() and fail the apply.

Also I found a side effect of moving this fwnmi registration to apply 
hook, I see the following assert

when I reboot the guest. may be I must have a member to indicate if the 
registration is already done.

Requesting system reboot
[  186.368745] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  186.370222] reboot: Restarting system
#######MACHINE RESET######
#####SPAPR_FWNMI_REGISTER######
qemu-system-ppc64: /home/jupit/qemu/hw/ppc/spapr_rtas.c:510: 
spapr_rtas_register: Assertion `!name || !rtas_table[token].name' failed.
Aborted

>>
>>> +        } else {
>>> +            error_setg(errp,
>>>   "Firmware Assisted Non-Maskable Interrupts not supported by KVM, 
>>> try cap-fwnmi-mce=off");
>>> +        }
>>>       }
>>>   }
>>>   diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index 2c066a372d..0328b1f341 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -400,6 +400,55 @@ static void rtas_get_power_level(PowerPCCPU 
>>> *cpu, SpaprMachineState *spapr,
>>>       rtas_st(rets, 1, 100);
>>>   }
>>>   +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>>> +                                  SpaprMachineState *spapr,
>>> +                                  uint32_t token, uint32_t nargs,
>>> +                                  target_ulong args,
>>> +                                  uint32_t nret, target_ulong rets)
>>> +{
>>> +    hwaddr rtas_addr = spapr_get_rtas_addr();
>>> +
>>> +    if (!rtas_addr) {
>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>>> +        return;
>>> +    }
>>> +
>>> +    if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) {
>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>> Actually, since you explicitly test for the cap being enabled here,
>> there's no reason not to *always* register this RTAS call.  Also this
>> test for the feature flag should go first, before delving into the
>> device tree for the RTAS address.
> Sure, will do
>>
>>> +        return;
>>> +    }
>>> +
>>> +    spapr->guest_machine_check_addr = rtas_ld(args, 1);
>>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>> +}
>>> +
>>> +static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
>>> +                                   SpaprMachineState *spapr,
>>> +                                   uint32_t token, uint32_t nargs,
>>> +                                   target_ulong args,
>>> +                                   uint32_t nret, target_ulong rets)
>>> +{
>>> +    if (spapr->guest_machine_check_addr == -1) {
>>> +        /* NMI register not called */
>>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>> +        return;
>>> +    }
>>> +
>>> +    if (spapr->mc_status != cpu->vcpu_id) {
>>> +        /* The vCPU that hit the NMI should invoke 
>>> "ibm,nmi-interlock" */
>>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * vCPU issuing "ibm,nmi-interlock" is done with NMI handling,
>>> +     * hence unset mc_status.
>>> +     */
>>> +    spapr->mc_status = -1;
>>> +    qemu_cond_signal(&spapr->mc_delivery_cond);
>>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>> +}
>>> +
>>>   static struct rtas_call {
>>>       const char *name;
>>>       spapr_rtas_fn fn;
>>> @@ -503,6 +552,14 @@ hwaddr spapr_get_rtas_addr(void)
>>>       return (hwaddr)fdt32_to_cpu(*rtas_data);
>>>   }
>>>   +void spapr_fwnmi_register(void)
>>> +{
>>> +    spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
>>> +                        rtas_ibm_nmi_register);
>>> +    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
>>> +                        rtas_ibm_nmi_interlock);
>>> +}
>>> +
>>>   static void core_rtas_register_types(void)
>>>   {
>>>       spapr_rtas_register(RTAS_DISPLAY_CHARACTER, "display-character",
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 4afa8d4d09..86f0fc8fdd 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -653,8 +653,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
>>> target_ulong opcode,
>>>   #define RTAS_IBM_REMOVE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x28)
>>>   #define RTAS_IBM_RESET_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x29)
>>>   #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
>>> +#define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
>>> +#define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
>>>   -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2B)
>>> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
>>>     /* RTAS ibm,get-system-parameter token values */
>>>   #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>>> @@ -907,4 +909,5 @@ void spapr_check_pagesize(SpaprMachineState 
>>> *spapr, hwaddr pagesize,
>>>     void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
>>>   hwaddr spapr_get_rtas_addr(void);
>>> +void spapr_fwnmi_register(void);
>>>   #endif /* HW_SPAPR_H */