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

[David Gibson]

On Mon, Aug 25, 2014 at 07:15:45PM +0530, 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 */

You should construct the instruction from the KVMPPC_H_REPORT_ERR
constant, otherwise it's an undocumented magic dependency between
parts of the code.

> +        0x6063f004,    /* ori     r3,r3,f004  */
> +        /* Issue H_CALL */
> +        0x44000022,    /*  sc      1     */
> +        0x7c9243a6,    /* mtspr r4 sprg2 */
> +        0xe8830000,    /* ld r4, 0(r3) */
> +        0x7c9a03a6,    /* mtspr r4, srr0 */
> +        0xe8830008,    /* ld r4, 8(r3) */
> +        0x7c9b03a6,    /* mtspr r4, srr1 */
> +        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;

The guest has absolutely no influence over this, so this should be an
assert() rather than returning an rtas error.  Or better yet, detect
the failure at compile time, since it doesn't depend on anything
runtime either.

> +    }
> +
> +    /*
> +     * Update the branch instruction in trampoline with the absolute
> +     * machine check address requested by OS
> +     */
> +    branch_inst |= guest_machine_check_addr;

Surely there should be some sanity checking of the address here, or it
can clobber any part of that branch instruction.

> +    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);
> +}
> +

This nmi_interlock call isn't described in the commit message.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpseSUEcX80R.pgp
Description: PGP signature