Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests

[Daniel Henrique Barboza]

On 2/14/22 11:11, Cédric Le Goater wrote:
> When an interrupt has been handled, the OS notifies the interrupt
> controller with a EOI sequence. On a POWER9 and POWER10 systems using
> the XIVE interrupt controller, 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 timeframe
> because of ordering issues. POWER9 systems use the LoadEOI instead.
> POWER10 compat guests should have fixed the issue with
> Load-after-Store ordering and StoreEOI can be activated for them
> again.
>
> To maintain performance, this ordering is only enforced for the
> XIVE_ESB_SET_PQ_10 load operation. This operation can be used to
> disable temporarily an interrupt source. If StoreEOI is active, a
> source could be left enabled if the load and store operations come
> out of order.
>
> Add a check in our XIVE emulation model for Load-after-Store when
> StoreEOI is active. It should catch unreliable sequences. Other load
> operations should be fine without it.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   include/hw/ppc/spapr_xive.h |  1 +
>   include/hw/ppc/xive.h       |  8 ++++++++
>   hw/intc/spapr_xive.c        | 15 +++++++++++++++
>   hw/intc/spapr_xive_kvm.c    | 15 +++++++++++++++
>   hw/intc/xive.c              |  6 ++++++
>   hw/ppc/spapr_hcall.c        |  7 +++++++
>   6 files changed, 52 insertions(+)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index b282960ad90d..9c247d8bf57d 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -73,6 +73,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/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 126e4e2c3a17..133f308c2792 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -285,6 +285,14 @@ uint8_t xive_esb_set(uint8_t *pq, uint8_t value);
>   #define XIVE_ESB_SET_PQ_10      0xe00 /* Load */
>   #define XIVE_ESB_SET_PQ_11      0xf00 /* Load */
>   
> +/*
> + * Load-after-store ordering
> + *
> + * Adding this offset to the load address will enforce
> + * load-after-store ordering. This is required to use with StoreEOI.
> + */
> +#define XIVE_ESB_LD_ST_MO       0x40 /* Load-after-store ordering */
> +
>   uint8_t xive_source_esb_get(XiveSource *xsrc, uint32_t srcno);
>   uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq);
>   
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index dc641cc604bf..0b8a246ad594 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -25,6 +25,7 @@
>   #include "hw/ppc/xive_regs.h"
>   #include "hw/qdev-properties.h"
>   #include "trace.h"
> +#include "cpu-models.h"
>   
>   /*
>    * XIVE Virtualization Controller BAR and Thread Managment BAR that we
> @@ -1854,3 +1855,17 @@ 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);
>   }
> +
> +/*
> + * Advertise StoreEOI for a P10 compat guest. OS is required to
> + * enforce load-after-store ordering.
> + */
> +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/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 61fe7bd2d322..bd023407bd7f 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -296,6 +296,21 @@ static uint64_t xive_esb_rw(XiveSource *xsrc, int srcno, 
> uint32_t offset,
>   
>   static uint8_t xive_esb_read(XiveSource *xsrc, int srcno, uint32_t offset)
>   {
> +    /*
> +     * The XIVE_ESB_SET_PQ_10 load operation is used to disable
> +     * temporarily an interrupt source. If StoreEOI is active, a
> +     * source could be left enabled if the load and store operations
> +     * come out of order.
> +     *
> +     * As we don't know the characteristics of the host source
> +     * interrupts (StoreEOI or not), enforce the load-after-store
> +     * ordering always. The performance penalty will be very small for
> +     * QEMU.
> +     */
> +    if (offset == XIVE_ESB_SET_PQ_10) {
> +        offset |= XIVE_ESB_LD_ST_MO;
> +    }
> +
>       return xive_esb_rw(xsrc, srcno, offset, 0, 0) & 0x3;
>   }
>   
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b8e4c7294d59..d62881873b1b 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1024,6 +1024,12 @@ static uint64_t xive_source_esb_read(void *opaque, 
> hwaddr addr, unsigned size)
>       case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
>       case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
>       case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
> +        if (offset == XIVE_ESB_SET_PQ_10 &&
> +            xsrc->esb_flags & XIVE_SRC_STORE_EOI) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "XIVE: load-after-store ordering "
> +                          "not enforced with Store EOI active for IRQ %d\n",
> +                          srcno);
> +        }
>           ret = xive_source_esb_set(xsrc, srcno, (offset >> 8) & 0x3);
>           break;
>       default:
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 8ffb13ada08e..6b888c963ac4 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1210,11 +1210,18 @@ target_ulong do_client_architecture_support(PowerPCCPU 
> *cpu,
>        * otherwise terminate the boot.
>        */
>       if (guest_xive) {
> +        bool enable;
> +
>           if (!spapr->irq->xive) {
>               error_report(
>   "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. */
> +        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(