On Wed, Nov 05, 2014 at 12:43:15PM +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. > > This patch also handles the cases when multi-processors > experience machine check at or about the same time. > As per PAPR, subsequent processors serialize waiting > for the first processor to issue the ibm,nmi-interlock call. > The second processor retries if the first processor which > received a machine check is still reading the error log > and is yet to issue ibm,nmi-interlock call. > > Signed-off-by: Aravinda Prasad <aravi...@linux.vnet.ibm.com>
[snip] > +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 ori_inst = 0x60630000; > + uint32_t branch_inst = 0x48000002; > + target_ulong guest_machine_check_addr; > + uint32_t trampoline[TRAMPOLINE_INSTS]; > + int total_inst = sizeof(trampoline) / sizeof(uint32_t); > + PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); You should sanity check the RTAS arguments before doing anything - in particular verify that nargs and nrets have the expected values. > + > + /* Store the system reset and machine check address */ > + guest_machine_check_addr = rtas_ld(args, 1); > + > + /* > + * Read the trampoline instructions from RTAS Blob and patch > + * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest > + * machine check address before copying to 0x200 vector > + */ > + cpu_physical_memory_read(spapr->rtas_addr + RTAS_TRAMPOLINE_OFFSET, > + trampoline, sizeof(trampoline)); > + > + /* Safety Check */ > + QEMU_BUILD_BUG_ON(sizeof(trampoline) > MC_INTERRUPT_VECTOR_SIZE); > + > + /* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */ > + ori_inst |= KVMPPC_H_REPORT_MC_ERR; > + memcpy(&trampoline[TRAMPOLINE_ORI_INST_INDEX], &ori_inst, > + sizeof(ori_inst)); Given that we already code the KVMPPC_H_RTAS value directly into the .S file, I don't think it's worth the trouble of patching the H_REPORT_MC_ERR value. As Alex says, it has to stay the same for migration anyway. > + /* > + * Sanity check guest_machine_check_addr to prevent clobbering > + * operator value in branch instruction > + */ > + if (guest_machine_check_addr & BRANCH_INST_MASK) { > + fprintf(stderr, "Unable to register ibm,nmi_register: " > + "Invalid machine check handler address\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; > + memcpy(&trampoline[TRAMPOLINE_BR_INST_INDEX], &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) > +{ Again you should sanity check the arguments - at least check nargs and nrets. > + /* > + * VCPU issuing ibm,nmi-interlock is done with NMI handling, > + * hence unset mc_in_progress. > + */ > + mc_in_progress = 0; > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > +} > + > static struct rtas_call { > const char *name; > spapr_rtas_fn fn; > @@ -419,6 +506,12 @@ static void core_rtas_register_types(void) > rtas_ibm_set_system_parameter); > spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term", > rtas_ibm_os_term); > + 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 a2d67e9..98d0a6c 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -384,8 +384,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool > msi); > #define RTAS_GET_SENSOR_STATE (RTAS_TOKEN_BASE + 0x1D) > #define RTAS_IBM_CONFIGURE_CONNECTOR (RTAS_TOKEN_BASE + 0x1E) > #define RTAS_IBM_OS_TERM (RTAS_TOKEN_BASE + 0x1F) > +#define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x20) > +#define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x21) > > -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x20) > +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x22) > > /* RTAS ibm,get-system-parameter token values */ > #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 > @@ -488,4 +490,17 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const > char *propname, > #define RTAS_TRAMPOLINE_OFFSET 0x200 > #define RTAS_ERRLOG_OFFSET 0x800 > > +/* Machine Check Trampoline related macros > + * > + * These macros should co-relate to the code we > + * have in pc-bios/spapr-rtas/spapr-rtas.S > + */ > +#define TRAMPOLINE_INSTS 17 > +#define TRAMPOLINE_ORI_INST_INDEX 2 > +#define TRAMPOLINE_BR_INST_INDEX 15 > + > +/* Machine Check Interrupt related macros */ > +#define MC_INTERRUPT_VECTOR 0x200 > +#define MC_INTERRUPT_VECTOR_SIZE 0x100 > + > #endif /* !defined (__HW_SPAPR_H__) */ > diff --git a/pc-bios/spapr-rtas/spapr-rtas.S b/pc-bios/spapr-rtas/spapr-rtas.S > index 903bec2..c315332 100644 > --- a/pc-bios/spapr-rtas/spapr-rtas.S > +++ b/pc-bios/spapr-rtas/spapr-rtas.S > @@ -35,3 +35,41 @@ _start: > ori 3,3,KVMPPC_H_RTAS@l > sc 1 > blr > + . = 0x200 > + /* > + * 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 return adress consist of saved > + * srr0 and srr1 which we restore and pass on the actual error > + * log address to OS handled mcachine check notification > + * routine > + * > + * All the below instructions are copied to interrupt vector > + * 0x200 at the time of handling ibm,nmi-register rtas call. > + */ > + mtsprg 2,3 > + li 3,0 > + /* > + * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR > + * value is patched below > + */ > +1: ori 3,3,0 > + sc 1 /* Issue H_CALL */ > + cmpdi cr0,3,0 > + beq cr0,1b /* retry KVMPPC_H_REPORT_MC_ERR */ Having to retry the hcall from here seems very awkward. This is a private hcall, so you can define it to do whatever retries are necessary internally (and I don't think your current implementation can fail anyway). > + mtsprg 2,4 Um.. doesn't this clobber the value of r3 you saved in SPRG2 just above. > + ld 4,0(3) > + mtsrr0 4 /* Restore srr0 */ > + ld 4,8(3) > + mtsrr1 4 /* Restore srr1 */ > + ld 4,16(3) > + mtcrf 0,4 /* Restore cr */ mtcrf? aren't you restoring the whole CR? > + addi 3,3,24 > + mfsprg 4,2 > + /* > + * Branch to address registered by OS. The branch address is > + * patched in the ibm,nmi-register rtas call. > + */ > + ba 0x0 > + b . The branch to self is pointless. Even if the instruction above is not patched, or patched incorrectly, it's a ba, so you're not likely to end up at the instruction underneath. Actually, what would probably make more sense would be to just have a "b ." *instead* of the ba, and have the qemu patching replace it with the correct ba instruction. That will limit the damage if it somehow gets executed without being patched. -- 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
pgpiwZtu2DtQ3.pgp
Description: PGP signature