On Mon, Nov 02, 2020 at 02:22:35PM +0100, Cédric Le Goater wrote:
> Sorry for the late answer I was out for a couple of weeks.
> 
> On 10/9/20 2:23 AM, David Gibson wrote:
> > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote:
> >> Hello,
> >>
> >> When an interrupt has been handled, the OS notifies the interrupt
> >> controller with an EOI sequence. On the XIVE interrupt controller
> >> (POWER9 and POWER10), this can be done with a load or a store
> >> operation on the ESB interrupt management page of the interrupt. The
> >> StoreEOI operation has less latency and improves interrupt handling
> >> performance but it was deactivated during the POWER9 DD2.0 time-frame
> >> because of ordering issues. POWER9 systems use the LoadEOI instead.
> >> POWER10 has fixed the issue with a special load command which enforces
> >> Load-after-Store ordering and StoreEOI can be safely used.
> > 
> > Do you mean that ordering is *always* enforced on P10?  Or it's a
> > special form of load that has the ordering?
> 
> It's a special load offset that has the ordering. Oring 0x40 to the load
> address : 
> 
>   #define XIVE_ESB_LOAD_EOI	0x000 /* Load */
>   #define XIVE_ESB_GET		0x800 /* Load */
>   #define XIVE_ESB_SET_PQ_00	0xc00 /* Load */
>   #define XIVE_ESB_SET_PQ_01	0xd00 /* Load */
>   #define XIVE_ESB_SET_PQ_10	0xe00 /* Load */
>   #define XIVE_ESB_SET_PQ_11	0xf00 /* Load */
> 
> will enforce load-after-store ordering.

Oh... I had assumed the problem was to do with the load/store ordering
within the CPU core itself (or maybe the L1, I guess).  But if the
address used can change it, the problem must be within the XIVE, yes?
Or at least somwhere on the Powerbus.  So, wasn't this just a plain
XIVE hardware bug?  In which case why is there software involvement as
well?

> We only need it for XIVE_ESB_SET_PQ_10. See commit b1f9be9392f0 
> ("powerpc/xive: Enforce load-after-store ordering when StoreEOI is active") 
> in Linux.
> 
> C. 
> 
> 
> > 
> > Also, weirdly, despite the series being addressed to me, only some of
> > the patches ended up in my inbox, rather than the list folder :/.
> > 
> >> These changes add a new StoreEOI capability which activate StoreEOI
> >> support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When
> >> the machine is using an emulated interrupt controller, TCG or without
> >> kernel IRQ chip, there are no limitations and activating StoreEOI is
> >> not an issue. However, when running with a kernel IRQ chip, some
> >> verification needs to be done on the host. This is done through the
> >> DT, which tells us that firmware has configured the HW for StoreEOI,
> >> but a new KVM capability would be cleaner.
> >>
> >> The last patch introduces a new 'cas' value to the capability which
> >> lets the hypervisor decide at CAS time if StoreEOI should be
> >> advertised to the guest OS. P10 compat kernel are considered safe
> >> because the OS enforces load-after-store ordering but not with P9.
> >>
> >> The StoreEOI capability is a global setting and does not take into
> >> account the characteristics of a single source. It could be an issue
> >> if StoreEOI is not supported on a specific source, of a passthrough
> >> device for instance. In that case, we could either introduce a new KVM
> >> ioctl to query the characteristics of the source at the HW level (like
> >> in v1) or deactivate StoreEOI on the machine.
> >>
> >> We are using these patches today on P10 and P9 (with a custom FW
> >> activating StoreEOI) systems to benchmark interrupt performance on
> >> large guests but there's no hurry to take them. Let's discuss this new
> >> approach.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >> Changes in v2:
> >>
> >>  - completely approach using a capability
> >>
> >> Cédric Le Goater (6):
> >>   spapr/xive: Introduce a StoreEOI capability
> >>   spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs
> >>   spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs
> >>   spapr/xive: Enforce load-after-store ordering
> >>   spapr/xive: Activate StoreEOI at the source level
> >>   spapr/xive: Introduce a new CAS value for the StoreEOI capability
> >>
> >>  include/hw/ppc/spapr.h      |  5 +++-
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  include/hw/ppc/xive.h       |  8 +++++
> >>  target/ppc/kvm_ppc.h        |  6 ++++
> >>  hw/intc/spapr_xive.c        | 10 +++++++
> >>  hw/intc/spapr_xive_kvm.c    | 12 ++++++++
> >>  hw/intc/xive.c              |  6 ++++
> >>  hw/ppc/spapr.c              |  1 +
> >>  hw/ppc/spapr_caps.c         | 60 +++++++++++++++++++++++++++++++++++++
> >>  hw/ppc/spapr_hcall.c        |  7 +++++
> >>  hw/ppc/spapr_irq.c          |  6 ++++
> >>  target/ppc/kvm.c            | 18 +++++++++++
> >>  12 files changed, 139 insertions(+), 1 deletion(-)
> >>
> > 
> 

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