Re: [Qemu-ppc] [Qemu-devel] [PATCH 4/5] target-ppc: Handle ibm, nmi-register RTAS call

[Aravinda Prasad]

On Wednesday 27 August 2014 04:07 PM, Alexander Graf wrote:
> 
> 
> On 25.08.14 15:45, Aravinda Prasad wrote:
>> This patch adds FWNMI support in qemu for powerKVM
>> guests by handling the ibm,nmi-register rtas call.
>> Whenever OS issues ibm,nmi-register RTAS call, the
>> machine check notification address is saved and the
>> machine check interrupt vector 0x200 is patched to
>> issue a private hcall.
>>
>> Signed-off-by: Aravinda Prasad <address@hidden>
>> ---
>>  hw/ppc/spapr_rtas.c    |   91 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |    8 ++++
>>  2 files changed, 98 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 02ddbf9..1135d2b 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -277,6 +277,91 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU 
>> *cpu,
>>      rtas_st(rets, 0, ret);
>>  }
>>  
>> +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>> +                                  sPAPREnvironment *spapr,
>> +                                  uint32_t token, uint32_t nargs,
>> +                                  target_ulong args,
>> +                                  uint32_t nret, target_ulong rets)
>> +{
>> +    int i;
>> +    uint32_t branch_inst = 0x48000002;
>> +    target_ulong guest_machine_check_addr;
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> +    /*
>> +     * Trampoline saves r3 in sprg2 and issues private hcall
>> +     * to request qemu to build error log. QEMU builds the
>> +     * error log, copies to rtas-blob and returns the address.
>> +     * The initial 16 bytes in rtas-blob consists of saved srr0
>> +     * and srr1 which we restore and pass on the actual error
>> +     * log address to OS handled mcachine check notification
>> +     * routine
>> +     */
>> +    uint32_t trampoline[] = {
>> +        0x7c7243a6,    /* mtspr   SPRN_SPRG2,r3 */
>> +        0x38600000,    /* li      r3,0   */
>> +        /* 0xf004 is the KVMPPC_H_REPORT_ERR private HCALL */
>> +        0x6063f004,    /* ori     r3,r3,f004  */
>> +        /* Issue H_CALL */
>> +        0x44000022,    /*  sc      1     */
> 
> So up to here we're saving r3 in SPRG2 (how do we know that we can
> clobber it?) and call our special hypercall.
> 
> But what does all the cruft below here do?

The saved r3 in SPRG2 is consumed in KVMPPC_H_REPORT_ERR hcall, hence we
can clobber SPRG2 after hcall returns. I have included a comment in
patch 3/5 while building error log. I think better I add one here as well.

> 
>> +        0x7c9243a6,    /* mtspr r4 sprg2 */
> 
> Apart from th fact that your order is wrong, this destroys the value of
> r3 that we saved above again.

SPRG2 is saved inside hcall and hence we don't need SPRG2 further after
KVMPPC_H_REPORT_ERR hcall returns.

> 
>> +        0xe8830000,    /* ld r4, 0(r3) */
>> +        0x7c9a03a6,    /* mtspr r4, srr0 */
>> +        0xe8830008,    /* ld r4, 8(r3) */
>> +        0x7c9b03a6,    /* mtspr r4, srr1 */
> 
> Can't we just set srr0 and srr1 directly?

I checked for instructions in ISA which set srr0/1 directly given an
address, but could not find any such instructions.

> 
>> +        0x38630010,    /* addi r3,r3,16 */
>> +        0x7c9242a6,    /* mfspr r4 sprg2 */
> 
> 
>> +        0x48000002,    /* Branch to address registered
>> +                        * by OS. The branch address is
>> +                        * patched below */
>> +        0x48000000,    /* b . */
>> +    };
>> +    int total_inst = sizeof(trampoline) / sizeof(uint32_t);
>> +
>> +    /* Store the system reset and machine check address */
>> +    guest_machine_check_addr = rtas_ld(args, 1);
>> +
>> +    /* Safety Check */
>> +    if (sizeof(trampoline) >= MC_INTERRUPT_VECTOR_SIZE) {
>> +        fprintf(stderr, "Unable to register ibm,nmi_register: "
>> +                "Trampoline size exceeded\n");
>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Update the branch instruction in trampoline with the absolute
>> +     * machine check address requested by OS
>> +     */
>> +    branch_inst |= guest_machine_check_addr;
> 
> Does this even work? You're creating a relative branch here.

We do an absolute branch here. guest_machine_check_addr contains the
physical address of machine check handler. We update the branch
instruction operand to do an absolute branch to this physical address.
Also when executing 0x200 address translations are off.

During testing I am able to see the machine check handler invoked by
this branch instruction. The disassembly of instructions from gdb at
0x200 correctly reflected the branch target.

Regards,
Aravinda

> 
> 
> Alex
> 
>> +    memcpy(&trampoline[11], &branch_inst, sizeof(branch_inst));
>> +
>> +    /* Handle all Host/Guest LE/BE combinations */
>> +    if ((*pcc->interrupts_big_endian)(cpu)) {
>> +        for (i = 0; i < total_inst; i++) {
>> +            trampoline[i] = cpu_to_be32(trampoline[i]);
>> +        }
>> +    } else {
>> +        for (i = 0; i < total_inst; i++) {
>> +            trampoline[i] = cpu_to_le32(trampoline[i]);
>> +        }
>> +    }
>> +
>> +    /* Patch 0x200 NMI interrupt vector memory area of guest */
>> +    cpu_physical_memory_write(MC_INTERRUPT_VECTOR, trampoline,
>> +                              sizeof(trampoline));
>> +
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +}
>> +
>> +static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
>> +                                   sPAPREnvironment *spapr,
>> +                                   uint32_t token, uint32_t nargs,
>> +                                   target_ulong args,
>> +                                   uint32_t nret, target_ulong rets)
>> +{
>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +}
>> +
>>  static struct rtas_call {
>>      const char *name;
>>      spapr_rtas_fn fn;
>> @@ -404,6 +489,12 @@ static void core_rtas_register_types(void)
>>      spapr_rtas_register(RTAS_IBM_SET_SYSTEM_PARAMETER,
>>                          "ibm,set-system-parameter",
>>                          rtas_ibm_set_system_parameter);
>> +    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 57199f5..8c854ca 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -386,8 +386,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool 
>> msi);
>>  #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
>>  #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
>>  #define RTAS_IBM_EXTENDED_OS_TERM               (RTAS_TOKEN_BASE + 0x20)
>> +#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 0x21)
>> +#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 0x22)
>>  
>> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x21)
>> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x23)
>>  
>>  /* RTAS ibm,get-system-parameter token values */
>>  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>> @@ -485,4 +487,8 @@ int spapr_dma_dt(void *fdt, int node_off, const char 
>> *propname,
>>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>>                        sPAPRTCETable *tcet);
>>  
>> +/* Machine Check Interrupt related macros */
>> +#define MC_INTERRUPT_VECTOR           0x200
>> +#define MC_INTERRUPT_VECTOR_SIZE      0x100
>> +
>>  #endif /* !defined (__HW_SPAPR_H__) */
>>
>>
> 

-- 
Regards,
Aravinda