On Wednesday 04 October 2017 07:04 AM, David Gibson wrote: > On Thu, Sep 28, 2017 at 04:08:21PM +0530, Aravinda Prasad wrote: >> Enable the KVM capability KVM_CAP_PPC_FWNMI so that >> the KVM causes guest exit with NMI as exit reason >> when it encounters a machine check exception on the >> address belonging to a guest. Without this capability >> enabled, KVM redirects machine check exceptions to >> guest's 0x200 vector. >> >> Signed-off-by: Aravinda Prasad <aravi...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_rtas.c | 15 +++++++++++++++ >> target/ppc/kvm.c | 13 +++++++++++++ >> target/ppc/kvm_ppc.h | 6 ++++++ >> 3 files changed, 34 insertions(+) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index 08e9a5e..d017a67 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -46,6 +46,7 @@ >> #include "qemu/cutils.h" >> #include "trace.h" >> #include "hw/ppc/fdt.h" >> +#include "kvm_ppc.h" >> >> static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState >> *spapr, >> uint32_t token, uint32_t nargs, >> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu, >> target_ulong args, >> uint32_t nret, target_ulong rets) >> { >> + int ret; >> + >> + ret = kvmppc_fwnmi_enable(cpu); > > If you're enabling it here, doesn't that mean you need to disable it > on reset?
We don't have a way in KVM to disable it once enabled. > >> + if (ret == 1) { >> + rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); >> + return; >> + } >> + >> + if (ret < 0) { >> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR); >> + return; >> + } >> + >> spapr->guest_machine_check_addr = rtas_ld(args, 1); >> rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> } >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >> index 7e4ce02..59b3322 100644 >> --- a/target/ppc/kvm.c >> +++ b/target/ppc/kvm.c >> @@ -92,6 +92,7 @@ static int cap_mmu_radix; >> static int cap_mmu_hash_v3; >> static int cap_resize_hpt; >> static int cap_ppc_pvr_compat; >> +static int cap_fwnmi; >> >> static uint32_t debug_inst_opcode; >> >> @@ -150,6 +151,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) >> cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX); >> cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3); >> cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT); >> + cap_fwnmi = kvm_check_extension(s, KVM_CAP_PPC_FWNMI); >> /* >> * Note: setting it to false because there is not such capability >> * in KVM at this moment. >> @@ -2142,6 +2144,17 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int >> mpic_proxy) >> } >> } >> >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu) >> +{ >> + CPUState *cs = CPU(cpu); >> + >> + if (!cap_fwnmi) { >> + return 1; >> + } >> + >> + return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0); > > Yeah, this is no good. It means migration from a host that's fwnmi > capable to one that isn't will be subtly broken. Instead you need to > make fwnmi capability a machine property. If the property is > requested and the host kernel doesn't support it, you need to outright > fail, rather than try to fall back. Sure. > >> +} >> + >> int kvmppc_smt_threads(void) >> { >> return cap_ppc_smt ? cap_ppc_smt : 1; >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h >> index 0139dae..55b6df2 100644 >> --- a/target/ppc/kvm_ppc.h >> +++ b/target/ppc/kvm_ppc.h >> @@ -28,6 +28,7 @@ void kvmppc_enable_clear_ref_mod_hcalls(void); >> void kvmppc_set_papr(PowerPCCPU *cpu); >> int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr); >> void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu); >> int kvmppc_smt_threads(void); >> void kvmppc_hint_smt_possible(Error **errp); >> int kvmppc_set_smt_threads(int smt); >> @@ -157,6 +158,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU >> *cpu, int mpic_proxy) >> { >> } >> >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu) >> +{ >> + return 1; > > Likewise, this should be available, not banned, on TCG. I think there > are existing problems with TCG<->KVM migration, but there's no > inherent reason they shouldn't work, so we don't want to introduce > extra reasons they don't. > > Even if TCG will never generate fwnmis (for now), it should allow the > guest to register for them. Should this be then changed later only when TCG generates fwnmis? Because we don't want to set spapr->guest_machine_check_addr for TCG in rtas_ibm_nmi_register(). If set then all machine checks are redirected to this address which is not desirable for TCG as we still want machine checks to be passed via 0x200. Regards, Aravinda > >> +} >> + >> static inline int kvmppc_smt_threads(void) >> { >> return 1; >> > -- Regards, Aravinda