Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/4] spapr: Handle "ibm, nmi-register" and "ibm, nmi-interlock" RTAS calls

[Aravinda Prasad <aravinda@linux.vnet.ibm.com>]

On Thursday 17 December 2015 09:21 AM, David Gibson wrote:
> On Wed, Dec 16, 2015 at 11:38:22AM +0530, Aravinda Prasad wrote:
>> 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 waits till the first processor, which also
>> received a machine check error, is done reading the error
>> log. The first processor issues "ibm,nmi-interlock" call
>> when the error log is consumed. This patch implements the
>> releasing part of the error-log while subsequent patch
>> (which builds error log) handles the locking part.
>>
>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_rtas.c    |   36 ++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |   10 +++++++++-
>>  2 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 9869bc9..17c4672 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -597,6 +597,38 @@ out:
>>      rtas_st(rets, 0, rc);
>>  }
>>  
>> +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)
>> +{
>> +    spapr->mc_in_progress = false;
>> +    qemu_cond_init(&spapr->mc_delivery_cond);
> 
> I think initializing mc_in_progress and the condition variable should
> go in the overall initialization (in fact during system reset) instead
> of here.  Things using these shouldn't be invoked until after the
> nmi_register, but it's safer to have the qemu internal variables
> initialized beforehand.

sure

> 
>> +    spapr->guest_machine_check_addr = rtas_ld(args, 1);
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +}
> 
> It also looks like you need to add code to clear
> guest_machine_check_addr if the system is reset

will add it

> 
>> +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) {
>> +        /* NMI register not called */
>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>> +    } else {
>> +        /*
>> +         * VCPU issuing "ibm,nmi-interlock" is done with NMI handling,
>> +         * hence unset mc_in_progress.
>> +         */
>> +        spapr->mc_in_progress = false;
>> +        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;
>> @@ -747,6 +779,10 @@ static void core_rtas_register_types(void)
>>                          rtas_get_sensor_state);
>>      spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
>>                          rtas_ibm_configure_connector);
>> +    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);
>>  }
>>  
>>  type_init(core_rtas_register_types)
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index b5cadd7..de84a4e 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -74,6 +74,12 @@ struct sPAPRMachineState {
>>      /* RTAS state */
>>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>  
>> +    /* State related to "ibm,nmi-register" and "ibm,nmi-interlock" calls */
>> +    target_ulong guest_machine_check_addr;
>> +    bool mc_in_progress;
>> +    int mc_cpu;
> 
> mc_cpu doesn't appear to be used.

mc_cpu is used in patch 3.

> 
> I think guest_machine_check_addr and mc_in_progress will also need to
> be added to migration state.

sure. I have not looked into migration state before, but I think I need
to check if the KVM on target after migration has the KVM_CAP_PPC_FWNMI
capability.

Regards,
Aravinda

> 
>> +    QemuCond mc_delivery_cond;
>> +
>>      /*< public >*/
>>      char *kvm_type;
>>  };
>> @@ -458,8 +464,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>>  #define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
>>  #define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
>>  #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
>> +#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x26)
>> +#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x27)
>>  
>> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
>> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
>>  
>>  /* RTAS ibm,get-system-parameter token values */
>>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>>
> 

-- 
Regards,
Aravinda