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

[David Gibson]

On Tue, May 14, 2019 at 11:02:07AM +0530, Aravinda Prasad wrote:
> 
> 
> On Tuesday 14 May 2019 10:17 AM, David Gibson wrote:
> > On Mon, May 13, 2019 at 04:00:43PM +0530, Aravinda Prasad wrote:
> >>
> >>
> >> On Friday 10 May 2019 03:23 PM, David Gibson wrote:
> >>> On Fri, May 10, 2019 at 12:45:29PM +0530, Aravinda Prasad wrote:
> >>>>
> >>>>
> >>>> On Friday 10 May 2019 12:16 PM, David Gibson wrote:
> >>>>> On Mon, Apr 22, 2019 at 12:33:35PM +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.
> >>>>>>
> >>>>>> This patch also deals with the case when a guest with
> >>>>>> the KVM_CAP_PPC_FWNMI capability enabled is attempted
> >>>>>> to migrate to a host that does not support this
> >>>>>> capability.
> >>>>>>
> >>>>>> Signed-off-by: Aravinda Prasad <address@hidden>
> >>>>>> ---
> >>>>>>  hw/ppc/spapr.c         |    1 +
> >>>>>>  hw/ppc/spapr_caps.c    |   26 ++++++++++++++++++++++++++
> >>>>>>  hw/ppc/spapr_rtas.c    |   14 ++++++++++++++
> >>>>>>  include/hw/ppc/spapr.h |    4 +++-
> >>>>>>  target/ppc/kvm.c       |   14 ++++++++++++++
> >>>>>>  target/ppc/kvm_ppc.h   |    6 ++++++
> >>>>>>  6 files changed, 64 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>> index ffd1715..44e09bb 100644
> >>>>>> --- a/hw/ppc/spapr.c
> >>>>>> +++ b/hw/ppc/spapr.c
> >>>>>> @@ -4372,6 +4372,7 @@ static void spapr_machine_class_init(ObjectClass 
> >>>>>> *oc, void *data)
> >>>>>>      smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> >>>>>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = 
> >>>>>> SPAPR_CAP_ON;
> >>>>>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> >>>>>> +    smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> >>>>>>      spapr_caps_add_properties(smc, &error_abort);
> >>>>>>      smc->irq = &spapr_irq_xics;
> >>>>>>      smc->dr_phb_enabled = true;
> >>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >>>>>> index edc5ed0..5b3af04 100644
> >>>>>> --- a/hw/ppc/spapr_caps.c
> >>>>>> +++ b/hw/ppc/spapr_caps.c
> >>>>>> @@ -473,6 +473,22 @@ static void 
> >>>>>> cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> >>>>>>      }
> >>>>>>  }
> >>>>>>  
> >>>>>> +static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
> >>>>>> +                                Error **errp)
> >>>>>> +{
> >>>>>> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >>>>>> +
> >>>>>> +    if (!val) {
> >>>>>> +        return; /* Disabled by default */
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    if (kvm_enabled()) {
> >>>>>> +        if (kvmppc_fwnmi_enable(cpu)) {
> >>>>>> +            error_setg(errp, "Requested fwnmi capability not support 
> >>>>>> by KVM");
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>>>>>      [SPAPR_CAP_HTM] = {
> >>>>>>          .name = "htm",
> >>>>>> @@ -571,6 +587,15 @@ SpaprCapabilityInfo 
> >>>>>> capability_table[SPAPR_CAP_NUM] = {
> >>>>>>          .type = "bool",
> >>>>>>          .apply = cap_ccf_assist_apply,
> >>>>>>      },
> >>>>>> +    [SPAPR_CAP_FWNMI_MCE] = {
> >>>>>> +        .name = "fwnmi-mce",
> >>>>>> +        .description = "Handle fwnmi machine check exceptions",
> >>>>>> +        .index = SPAPR_CAP_FWNMI_MCE,
> >>>>>> +        .get = spapr_cap_get_bool,
> >>>>>> +        .set = spapr_cap_set_bool,
> >>>>>> +        .type = "bool",
> >>>>>> +        .apply = cap_fwnmi_mce_apply,
> >>>>>> +    },
> >>>>>>  };
> >>>>>>  
> >>>>>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState 
> >>>>>> *spapr,
> >>>>>> @@ -706,6 +731,7 @@ SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
> >>>>>>  SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> >>>>>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> >>>>>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> >>>>>> +SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI_MCE);
> >>>>>>  
> >>>>>>  void spapr_caps_init(SpaprMachineState *spapr)
> >>>>>>  {
> >>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>>>>> index d3499f9..997cf19 100644
> >>>>>> --- a/hw/ppc/spapr_rtas.c
> >>>>>> +++ b/hw/ppc/spapr_rtas.c
> >>>>>> @@ -49,6 +49,7 @@
> >>>>>>  #include "hw/ppc/fdt.h"
> >>>>>>  #include "target/ppc/mmu-hash64.h"
> >>>>>>  #include "target/ppc/mmu-book3s-v3.h"
> >>>>>> +#include "kvm_ppc.h"
> >>>>>>  
> >>>>>>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState 
> >>>>>> *spapr,
> >>>>>>                                     uint32_t token, uint32_t nargs,
> >>>>>> @@ -354,6 +355,7 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> >>>>>>                                    target_ulong args,
> >>>>>>                                    uint32_t nret, target_ulong rets)
> >>>>>>  {
> >>>>>> +    int ret;
> >>>>>>      uint64_t rtas_addr = spapr_get_rtas_addr();
> >>>>>>  
> >>>>>>      if (!rtas_addr) {
> >>>>>> @@ -361,6 +363,18 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> >>>>>>          return;
> >>>>>>      }
> >>>>>>  
> >>>>>> +    ret = kvmppc_fwnmi_enable(cpu);
> >>>>>
> >>>>> You shouldn't need this here as well as in cap_fwnmi_mce_apply().
> >>>>>
> >>>>> Instead, you should unconditionally fail the nmi-register if the
> >>>>> capability is not enabled.
> >>>>
> >>>> cap_fwnmi is not enabled by default, because if it is enabled by default
> >>>> them KVM will start routing machine check exceptions via guest exit
> >>>> instead of routing it to guest's 0x200.
> >>>>
> >>>> During early boot since guest has not yet issued nmi-register, KVM is
> >>>> expected to route exceptions to 0x200. Therefore we enable cap_fwnmi
> >>>> only when a guest issues nmi-register.
> >>>
> >>> Except that's not true - you enable it in cap_fwnmi_mce_apply() which
> >>> will be executed whenever the machine capability is enabled.
> >>
> >> I enable cap_fwnmi in cap_fwnmi_mce_apply() only when the "val" argument
> >> (which is the effective cap value) is set. In early boot "val" is not
> >> set as cap_fwnmi by default is not set, hence cap_fwnmi is not
> >> enabled.
> > 
> > Uh.. if that's true, something else is horribly wrong.  SPAPR caps are
> > designed to have a fixed value for the lifetime of the VM.  Otherwise
> > they will fail in their purpose of making sure we have a consistent
> > environment across migrations.  So if the 'val' changes after the
> > first call to apply(), then something is broken.
> 
> If SPAPR caps are initialized before boot that do not change later, then
> for cap_fwnmi, the default value is off at boot and the cap is enabled
> only when guest issues "ibm,nmi-register" call. Should SPAPR caps be
> updated when "ibm,nmi-register" is called?

So the confusing thing here is that there are spapr machine caps, and
those are separate from the KVM caps for the VM.  Then the KVM caps
also have whether the cap is possible and whether it is current
activated.

The spapr machine caps *must* remain static for the VM's lifetime and
only cover possibilities, not runtime configuration.  KVM caps may be
activated as necessary.

So you can leave activating the KVM cap until nmi-register.  But if
the spapr cap is disabled you must prohibit nmi-register.

The apply() functions are responsible for checking if the spapr caps
are possible on the KVM implementation.  So if cap_fwnmi_mci_apply()
is called with val==1 and KVM doesn't support the fwnmi extensions, it
must fail outright.

> >> My understanding is that, cap_fwnmi_mce_apply() is also called during
> >> migration on the target machine.
> > 
> > Only in the sense that the machine is initialized before processing
> > the incoming migration.  The capability values must be equal on either
> > side of the migration (that's checked elsewhere).  Well, actually,
> > you're allowed to increase the cap value across a migration, just not
> > decrease it.
> 
> Ah.. ok.. I am still familiarizing myself with the migration code..
> 
> > 
> >> If effective cap for cap_fwnmi is
> >> enabled on source machine than I think "val" will be set when
> >> cap_fwnmi_mce_apply() is called on target machine.
> > 
> > Nope.  The migrated value of the cap will be *validated* against the
> > value set on the destination setup, but it won't *alter* the value on
> > the destination (the result is that you have it enabled on the source,
> > but not the destination, the migration will fail).
> 
> But if cap_fwnmi is set on the host, which function is responsible to

I'm not sure what you mean by "the host" here.

> enable it on the destination? I think cap_fwnmi_mce_apply() is
> responsible for enabling it on the destination.

Enabling the spapr cap?  It is set based on the command line and
machine type, just like on the source machine.

> If that is the case
> cap_fwnmi_mce_apply() should know if cap_fwnmi is set on the host and
> the only way it can check that is based on the "val" argument passed on
> to it.
> 
> Or am I missing something here?

Probably, but I'm not sure exactly what.

The val argument to apply() is set to the value of the spapr cap.
This is based on the qemu command line and machine type, and must be
the same on source and destination.  As a general rule, qemu requires
that the same machine options are used on source and destination.

-- 
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

signature.asc
Description: PGP signature