Re: [Qemu-devel] [PATCH v5 5/6] ppc: spapr: Enable FWNMI capability

[Aravinda Prasad]

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 <address@hidden>
>> ---
>>  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