qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Henrique Barboza
Subject: Re: [PATCH] ppc/spapr: Advertise StoreEOI for POWER10 compat guests
Date: Wed, 16 Feb 2022 19:17:31 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0



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

nit:  s/a EOI sequence/an EOI sequence


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>
---
  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);
+        }

I'm afraid I have a lack of understanding of what is happening here but I'll 
try. Up
there, in xive_esb_read(), you forced the load-after-store ordering for the Load
operation that can left the source enabled if we have an out of order 
store-after-load
situation. Given that it's a function in spapr_xive_kvm.c I understand that 
this is
a wrapper for the kvmppc() calls into the host kernel XIVE irqchip, so that 
piece
of code is forcing the load-after-store at all times for that Load operation 
since
the performance penalty is not relevant.

Assuming that what I said is mostly correct, I don't understand why we can't do 
a
similar thing in the hw/intc/xive.c case, where I suppose it's the XIVE 
emulation
controller implementation that we're going to use if we don't have an  irqchip 
to
use with KVM. Can't we force the load-after-store ordering in the emulated
XIVE_ESB_SET_PQ_10 instead of throwing an error?


Everything else in the patch looks fine to me.


Thanks,


Daniel

          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(



reply via email to

[Prev in Thread] Current Thread [Next in Thread]