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

[Aravinda Prasad]

On Thursday 17 August 2017 07:09 AM, David Gibson wrote:
> What's with the extra spaces in the subject line?

I don't see any. Can you pls point out?

> 
> On Wed, Aug 16, 2017 at 02:42:21PM +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 <address@hidden>
>> ---
>>  hw/ppc/spapr.c         |    8 ++++++++
>>  hw/ppc/spapr_rtas.c    |   35 +++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |   10 +++++++++-
>>  3 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2a3e53d..0bb2c4a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1441,6 +1441,11 @@ static void ppc_spapr_reset(void)
>>      first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
>>  
>>      spapr->cas_reboot = false;
>> +
>> +    spapr->mc_in_progress = false;
>> +    spapr->guest_machine_check_addr = 0;
>> +    qemu_cond_destroy(&spapr->mc_delivery_cond);
>> +    qemu_cond_init(&spapr->mc_delivery_cond);
>>  }
>>  
>>  static void spapr_create_nvram(sPAPRMachineState *spapr)
>> @@ -2491,6 +2496,9 @@ static void ppc_spapr_init(MachineState *machine)
>>  
>>          kvmppc_spapr_enable_inkernel_multitce();
>>      }
>> +
>> +    spapr->mc_in_progress = false;
>> +    qemu_cond_init(&spapr->mc_delivery_cond);
>>  }
>>  
>>  static int spapr_kvm_type(const char *vm_type)
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 94a2799..2f3c47b 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -348,6 +348,37 @@ 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)
>> +{
>> +    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) {
>> +        /* 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;
>> @@ -489,6 +520,10 @@ static void core_rtas_register_types(void)
>>                          rtas_set_power_level);
>>      spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
>>                          rtas_get_power_level);
>> +    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 46012b3..eee8d33 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -123,6 +123,12 @@ struct sPAPRMachineState {
>>       * occurs during the unplug process. */
>>      QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
>>  
>> +    /* 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 isn't actually used yet in this patch.  In any case it and
> mc_in_progress could probably be folded together, no?

It is possible to fold mc_cpu and mc_in_progress together with the
convention that if it is set to -1 mc is not in progress otherwise it is
set to the CPU handling the mc.

> 
> These values will also need to be migrated, AFAICT.

I am thinking of how to handle the migration when machine check handling
is in progress. Probably wait for machine check handling to complete
before migrating as the error could be irrelevant once migrated to a new
hardware. If that is the case we don't need to migrate these values.

Regards,
Aravinda

> 
>> +    QemuCond mc_delivery_cond;
>> +
>>      /*< public >*/
>>      char *kvm_type;
>>      MemoryHotplugState hotplug_memory;
>> @@ -519,8 +525,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
>> target_ulong opcode,
>>  #define RTAS_IBM_CREATE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x27)
>>  #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_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x2A)
>> +#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x2B)
>>  
>> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2A)
>> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2C)
>>  
>>  /* RTAS ibm,get-system-parameter token values */
>>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>>
> 

-- 
Regards,
Aravinda