Re: [PATCH v2 0/6] spapr/xive: Activate StoreEOI in P10 compat guests

["Cédric Le Goater" <clg@kaod.org>]

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. 

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