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

From: "Cédric Le Goater" <clg@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: Gustavo Romero <gromero@linux.ibm.com>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	Greg Kurz <groug@kaod.org>
Subject: Re: [PATCH v2 0/6] spapr/xive: Activate StoreEOI in P10 compat guests
Date: Mon, 23 Nov 2020 12:16:10 +0100	[thread overview]
Message-ID: <90801056-75cf-6c79-183e-d2c5686f2871@kaod.org> (raw)
In-Reply-To: <20201123064454.GY521467@yekko.fritz.box>

On 11/23/20 7:44 AM, David Gibson wrote:
> 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?

Yes. It's in the XIVE logic handling the load/store operations on the 
PQ bits.

> Or at least somwhere on the Powerbus.  So, wasn't this just a plain
> XIVE hardware bug?  

It's a theoretical bug in HW. StoreEOI is activated on the P9 systems 
we use for performance testing and it never showed up.

> In which case why is there software involvement as well?

Software is involved as an optimization, because only PQ_10 loads need 
the ordering enforcement.

commit b1f9be9392f0 in Linux says more : 

    There is usually no need to enforce ordering between ESB load and
    store operations as they should lead to the same result. E.g. a store
    trigger and a load EOI can be executed in any order. Assuming the
    interrupt state is PQ=10, a store trigger followed by a load EOI will
    return a Q bit. In the reverse order, it will create a new interrupt
    trigger from HW. In both cases, the handler processing interrupts is
    notified.

    In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to
    disable temporarily the interrupt source (mask/unmask). When the
    source is reenabled, the OS can detect if interrupts were received
    while the source was disabled and reinject them. This process needs
    special care when StoreEOI is activated. The ESB load and store
    operations should be correctly ordered because a XIVE_ESB_STORE_EOI
    operation could leave the source enabled if it has not completed
    before the loads.

    For those cases, we enforce Load-after-Store ordering with a special
    load operation offset. To avoid performance impact, this ordering is
    only enforced when really needed, that is when interrupt sources are
    temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not
    be needed for other loads.

This ordering is a requirement for StoreEOI. 

C.