Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Greg Kurz]

On Tue, 6 Oct 2020 19:56:20 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 10/6/20 7:39 PM, Greg Kurz wrote:
> > On Mon, 5 Oct 2020 18:51:47 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> When the StoreEOI capability is set to "cas", let CAS decide when
> >> StoreEOI should be advertised. StoreEOI is safe to use with a P10
> >> compat machine because the OS enforces load-after-store ordering but
> >> not with P9 compat.

Hmm... OS enforcing load-after-store based on a PVR ? I can't find
such a thing in the kernel sources. What I find is:

        if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
                offset |= XIVE_ESB_LD_ST_MO;

where xd->flags comes from H_INT_GET_SOURCE_INFO, so this essentially
depends on what the hypervisor advertises. For example a P9 compat
guest could use it if QEMU has an emulated XIVE, right ?

> >>
> >> The question now is : should we make "cas" the default at the machine
> >> level ?
> >>
> > 
> > Hmm, spapr capabilities are knobs for the end user to provide a specific
> > and consistent behavior to the guest... so the "let CAS decide depending
> > on what the guest asked for" thing looks awkward... 
> 
> The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI
> if the kernel is P10 compat and will not if it's P9 to make sure that
> it can be migrated to a P9 host.  
> 

Yeah I know that the guest doesn't ask for StoreEOI. What I mean is that
the idea behind spapr capabilities is only about saying "this machine type
offers this feature and the host supports it". The fact that the guest
might use it or not is another matter. So I don't quite see how "cas" fits
into this model...

> > I mean that making
> > "cas" the default at the machine level looks like you don't really want
> > this to be a capability at all.
> 
> It means that you are not forcing a behaviour "off" or "on". It depends
> on the guest support, P10 or P9, which is what we want as a default 
> behavior to be in sync with pHyp.
> 

I think there's confusion here. Setting the capability to "on"/"off"
doesn't mean your force anything. As said above, this isn't related
to what the guest supports but essentially to what the host supports.
"on" means that we can offer the feature to the guest in a reliable
manner because we have verified it can work, and "off" simply means
we don't offer the feature (but of course the guest is still free
to shoot itself in the foot).

The reference to what pHyp does is thus irrelevant here because we
won't ever migrate a guest between KVM and PowerVM. We could decide
to do nothing and let the OS only do "LoadEOI" operations as it does
today.

If we want StoreEOI to be usable by default, then the default value
should definitely be "on". This should be kept to "off" for older
machine types to stay compatible with existing setups.

Let's only consider the KVM case only because we don't explicitly
support KVM<->TCG migration. The availability of StoreEOI is:
- emulated XIVE on P9 system => ok
- emulated XIVE on P10 system => ok
- in-kernel XIVE on P10 system => ok
- in-kernel XIVE on P9 system => not ok

This means that on a P9 system we must fallback on emulated XIVE, like
we already do with bostons today. This might look sub-optimal from a
performance standpoint but this is the price to pay to guarantee
migration doesn't fail with the default settings.

> > If the user asked for StoreEOI but the guest chose a compat mode that
> > can't support it, QEMU should simply exit IMHO
> It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use 
> StoreEOI. So it should be considered more like a mitigation. But anyhow, 
> the P9 systems currenly shipped never activate StoreEOI. So you will get 
> the error.
>  
> > Or alternatively I guess you can just forget about the spapr capability
> > and do all the checks (XIVE interrupt controller, KVM support, guest
> > asked for P10) at CAS.
> 
> That's how I did in the fisrt patches but then you loose the ability to 
> switch it off on P10 and switch on on P9. Which is good to have for tests 
> and performance and for possible issues if we ever have to handle a source 
> which has different charateristics. In that case, we would switch it off. 
> 

I understand that having fine grain knobs is cool for tests and performance
but keep in mind that we mostly target end-users who expect this to work
out of the box with the default settings. This being said, if some end-users
might hit a corner case where it would be valuable to switch it off, then
it makes sense to keep the ability to do so of course.

> Thanks,
> 
> C.
> 
> 
>  
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/spapr.h      |  1 +
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  hw/intc/spapr_xive.c        |  9 +++++++++
> >>  hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
> >>  hw/ppc/spapr_hcall.c        |  7 +++++++
> >>  5 files changed, 35 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index b701c14b4e09..17e7d873e8dc 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -87,6 +87,7 @@ typedef enum {
> >>  #define SPAPR_CAP_ON                    0x01
> >>  
> >>  /* Custom Caps */
> >> +#define SPAPR_CAP_CAS                   0x02
> >>  
> >>  /* Generic */
> >>  #define SPAPR_CAP_BROKEN                0x00
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 26c8d90d7196..8b8aa586e44f 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
> >>  
> >>  int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
> >>                               uint32_t *out_server, uint8_t *out_prio);
> >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable);
> >>  
> >>  /*
> >>   * KVM XIVE device helpers
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 41f2719ff93a..f57a2681dd91 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr)
> >>      spapr_register_hypercall(H_INT_SYNC, h_int_sync);
> >>      spapr_register_hypercall(H_INT_RESET, h_int_reset);
> >>  }
> >> +
> >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable)
> >> +{
> >> +    if (enable) {
> >> +        xive->source.esb_flags |= XIVE_SRC_STORE_EOI;
> >> +    } else {
> >> +        xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI;
> >> +    }
> >> +}
> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >> index 9251badbdc27..c55e1fccb9bc 100644
> >> --- a/hw/ppc/spapr_caps.c
> >> +++ b/hw/ppc/spapr_caps.c
> >> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
> >> uint8_t val,
> >>      }
> >>  }
> >>  
> >> +SpaprCapPossible cap_storeeoi_possible = {
> >> +    .num = 3,
> >> +    .vals = { "off", "on", "cas" },
> >> +    .help = "off - no StoreEOI, on - StoreEOI, "
> >> +            "cas - negotiated at CAS (POWER10 compat only)",
> >> +};
> >> +
> >>  static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> >>                                 Error **errp)
> >>  {
> >> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState 
> >> *spapr, uint8_t val,
> >>              return;
> >>          }
> >>  
> >> +        /* CAS will decide to advertise StoreEOI (P10 compat kernels 
> >> only) */
> >> +        if (val == SPAPR_CAP_CAS) {
> >> +            return;
> >> +        }
> >> +
> >>          /*
> >>           * load-after-store ordering is not enforced on POWER9 CPUs
> >>           * and StoreEOI can be racy.
> >> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] 
> >> = {
> >>      },
> >>      [SPAPR_CAP_STOREEOI] = {
> >>          .name = "storeeoi",
> >> -        .description = "Implements XIVE StoreEOI feature",
> >> +        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
> >>          .index = SPAPR_CAP_STOREEOI,
> >> -        .get = spapr_cap_get_bool,
> >> -        .set = spapr_cap_set_bool,
> >> -        .type = "bool",
> >> +        .get = spapr_cap_get_string,
> >> +        .set = spapr_cap_set_string,
> >> +        .type = "string",
> >> +        .possible = &cap_storeeoi_possible,
> >>          .apply = cap_storeeoi_apply,
> >>      },
> >>  };
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 607740150fa2..158b122b9192 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1804,6 +1804,13 @@ target_ulong 
> >> do_client_architecture_support(PowerPCCPU *cpu,
> >>  "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive 
> >> or ic-mode=dual machine property");
> >>              exit(EXIT_FAILURE);
> >>          }
> >> +
> >> +        /* Advertise StoreEOI for a P10 compat guest. */
> >> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
> >> +            bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 
> >> 0,
> >> +                                           cpu->compat_pvr);
> >> +            spapr_xive_enable_store_eoi(spapr->xive, enable);
> >> +        }
> >>      } else {
> >>          if (!spapr->irq->xics) {
> >>              error_report(
> > 
>