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

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

[Cédric Le Goater]

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.

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

-- 
2.25.4

[PATCH v2 1/6] spapr/xive: Introduce a StoreEOI capability

[Cédric Le Goater]

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.

The new StoreEOI capability adds StoreEOI support to 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.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h |  4 +++-
 target/ppc/kvm_ppc.h   |  6 ++++++
 hw/ppc/spapr.c         |  1 +
 hw/ppc/spapr_caps.c    | 30 ++++++++++++++++++++++++++++++
 target/ppc/kvm.c       | 18 ++++++++++++++++++
 5 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index bba8736269f4..b701c14b4e09 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -74,8 +74,10 @@ typedef enum {
 #define SPAPR_CAP_CCF_ASSIST            0x09
 /* Implements PAPR FWNMI option */
 #define SPAPR_CAP_FWNMI                 0x0A
+/* Implements XIVE StoreEOI feature */
+#define SPAPR_CAP_STOREEOI              0x0B
 /* Num Caps */
-#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
+#define SPAPR_CAP_NUM                   (SPAPR_CAP_STOREEOI + 1)
 
 /*
  * Capability Values
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 72e05f1cd2fc..c5a487dbba13 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void);
 bool kvmppc_has_cap_mmu_radix(void);
 bool kvmppc_has_cap_mmu_hash_v3(void);
 bool kvmppc_has_cap_xive(void);
+bool kvmppc_has_cap_xive_storeeoi(void);
 int kvmppc_get_cap_safe_cache(void);
 int kvmppc_get_cap_safe_bounds_check(void);
 int kvmppc_get_cap_safe_indirect_branch(void);
@@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void)
     return false;
 }
 
+static inline bool kvmppc_has_cap_xive_storeeoi(void)
+{
+    return false;
+}
+
 static inline int kvmppc_get_cap_safe_cache(void)
 {
     return 0;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4256794f3bed..e83de0580142 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF;
     spapr_caps_add_properties(smc);
     smc->irq = &spapr_irq_dual;
     smc->dr_phb_enabled = true;
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9341e9782a3f..57c62c22e4cc 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
     }
 }
 
+static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
+                               Error **errp)
+{
+    ERRP_GUARD();
+    MachineState *machine = MACHINE(spapr);
+    bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi();
+
+    if (!val) {
+        return; /* Disabled by default */
+    }
+
+    /* Check host support when the KVM device is in use */
+    if (kvm_irqchip_in_kernel()) {
+        if (!kvm_storeeoi) {
+            error_setg(errp, "StoreEOI not supported by KVM");
+            return;
+        }
+    }
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .apply = cap_fwnmi_apply,
     },
+    [SPAPR_CAP_STOREEOI] = {
+        .name = "storeeoi",
+        .description = "Implements XIVE StoreEOI feature",
+        .index = SPAPR_CAP_STOREEOI,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
+        .apply = cap_storeeoi_apply,
+    },
 };
 
 static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
@@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
 SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
 SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
 SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
+SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI);
 
 void spapr_caps_init(SpaprMachineState *spapr)
 {
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index d85ba8ffe00b..9ad637151070 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void)
     return cap_xive;
 }
 
+/*
+ * TODO: Introduce a new KVM capability
+ */
+bool kvmppc_has_cap_xive_storeeoi(void)
+{
+    static const char *compat = "ibm,opal-xive-pe";
+    void *host_fdt;
+    int xive_node;
+
+    host_fdt = load_device_tree_from_sysfs();
+    xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat);
+    if (xive_node < 0) {
+        return false;
+    }
+
+    return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL);
+}
+
 static void kvmppc_get_cpu_characteristics(KVMState *s)
 {
     struct kvm_ppc_cpu_char c;
-- 
2.25.4

[PATCH v2 2/6] spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_caps.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 57c62c22e4cc..b0a9d0227db2 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -535,6 +535,14 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
         return; /* Disabled by default */
     }
 
+    /* For POWER8 CPUs, setting StoreEOI is useless as XIVE is not used */
+    if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_00, 0,
+                               spapr->max_compat_pvr)) {
+        warn_report("StoreEOI is for the XIVE interrupt mode "
+                    "(POWER9 and above)");
+        return;
+    }
+
     /* Check host support when the KVM device is in use */
     if (kvm_irqchip_in_kernel()) {
         if (!kvm_storeeoi) {
-- 
2.25.4

[PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs

[Cédric Le Goater]

StoreEOI on POWER9 CPUs is racy because load-after-store ordering is
not enforced.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_caps.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index b0a9d0227db2..9251badbdc27 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
             error_setg(errp, "StoreEOI not supported by KVM");
             return;
         }
+
+        /*
+         * load-after-store ordering is not enforced on POWER9 CPUs
+         * and StoreEOI can be racy.
+         */
+        if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
+                                  0, spapr->max_compat_pvr)) {
+            warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM.");
+        }
     }
 }
 
-- 
2.25.4

[PATCH v2 4/6] spapr/xive: Enforce load-after-store ordering

[Cédric Le Goater]

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.

QEMU makes use of this offset to quiesce the sources before a
migration. Enforce the load-after-store ordering always when doing so
without querying the characteristics of the sources on the host. The
performance penalty will be very small for QEMU.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xive.h    |  8 ++++++++
 hw/intc/spapr_xive_kvm.c | 12 ++++++++++++
 hw/intc/xive.c           |  6 ++++++
 3 files changed, 26 insertions(+)

diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 445eccfe6b73..39cd273f86d5 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -279,6 +279,14 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)
 #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_kvm.c b/hw/intc/spapr_xive_kvm.c
index 66bf4c06fe55..d428422a7b72 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -357,6 +357,18 @@ 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.
+     *
+     * Enforce the load-after-store ordering always.
+     */
+    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 489e6256ef70..b710ba2df095 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -998,6 +998,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:
-- 
2.25.4

[PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level

[Cédric Le Goater]

When the StoreEOI capability is "on", the H_INT_GET_SOURCE_INFO will
set the StoreEOI flag for all sources. This 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 or
deactivate StoreEOI on the machine.

This is theoretically unsafe on a POWER9 host but it still runs.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/spapr_xive.c | 1 +
 hw/ppc/spapr_irq.c   | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 1fa09f287ac0..41f2719ff93a 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -280,6 +280,7 @@ static void spapr_xive_instance_init(Object *obj)
     SpaprXive *xive = SPAPR_XIVE(obj);
 
     object_initialize_child(obj, "source", &xive->source, TYPE_XIVE_SOURCE);
+    object_property_add_alias(obj, "flags", OBJECT(&xive->source), "flags");
 
     object_initialize_child(obj, "end_source", &xive->end_source,
                             TYPE_XIVE_END_SOURCE);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index f59960339ec3..cdf9f9df4173 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -325,9 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
 
     if (spapr->irq->xive) {
         uint32_t nr_servers = spapr_max_server_number(spapr);
+        uint64_t flags = 0;
         DeviceState *dev;
         int i;
 
+        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_ON) {
+            flags |= XIVE_SRC_STORE_EOI;
+        }
+
         dev = qdev_new(TYPE_SPAPR_XIVE);
         qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
         /*
@@ -337,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
         qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
         object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
                                  &error_abort);
+        object_property_set_int(OBJECT(dev), "flags", flags, &error_abort);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
         spapr->xive = SPAPR_XIVE(dev);
-- 
2.25.4

[PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Cédric Le Goater]

When the StoreEOI capability is set to "cas", let CAS decide when
StoreEOI should be advertised. StoreEOI is safe to use with a P10
compat machine because the OS enforces load-after-store ordering but
not with P9 compat.

The question now is : should we make "cas" the default at the machine
level ?

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr.h      |  1 +
 include/hw/ppc/spapr_xive.h |  1 +
 hw/intc/spapr_xive.c        |  9 +++++++++
 hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
 hw/ppc/spapr_hcall.c        |  7 +++++++
 5 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b701c14b4e09..17e7d873e8dc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -87,6 +87,7 @@ typedef enum {
 #define SPAPR_CAP_ON                    0x01
 
 /* Custom Caps */
+#define SPAPR_CAP_CAS                   0x02
 
 /* Generic */
 #define SPAPR_CAP_BROKEN                0x00
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 26c8d90d7196..8b8aa586e44f 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -75,6 +75,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/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 41f2719ff93a..f57a2681dd91 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1802,3 +1802,12 @@ 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);
 }
+
+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/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9251badbdc27..c55e1fccb9bc 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
     }
 }
 
+SpaprCapPossible cap_storeeoi_possible = {
+    .num = 3,
+    .vals = { "off", "on", "cas" },
+    .help = "off - no StoreEOI, on - StoreEOI, "
+            "cas - negotiated at CAS (POWER10 compat only)",
+};
+
 static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
                                Error **errp)
 {
@@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
             return;
         }
 
+        /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */
+        if (val == SPAPR_CAP_CAS) {
+            return;
+        }
+
         /*
          * load-after-store ordering is not enforced on POWER9 CPUs
          * and StoreEOI can be racy.
@@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     },
     [SPAPR_CAP_STOREEOI] = {
         .name = "storeeoi",
-        .description = "Implements XIVE StoreEOI feature",
+        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
         .index = SPAPR_CAP_STOREEOI,
-        .get = spapr_cap_get_bool,
-        .set = spapr_cap_set_bool,
-        .type = "bool",
+        .get = spapr_cap_get_string,
+        .set = spapr_cap_set_string,
+        .type = "string",
+        .possible = &cap_storeeoi_possible,
         .apply = cap_storeeoi_apply,
     },
 };
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 607740150fa2..158b122b9192 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
 "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. */
+        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
+            bool 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(
-- 
2.25.4

Re: [PATCH v2 1/6] spapr/xive: Introduce a StoreEOI capability

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:42 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> 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.
> 
> The new StoreEOI capability adds StoreEOI support to 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.
> 

Cleaner and even required... a user could possibly run an older
KVM that doesn't know about StoreEOI on a POWER10 host and QEMU
would wrongly assume the feature is supported. Also, I guess this
should rather be an attribute of the XIVE KVM device rather than a
plain KVM property.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr.h |  4 +++-
>  target/ppc/kvm_ppc.h   |  6 ++++++
>  hw/ppc/spapr.c         |  1 +
>  hw/ppc/spapr_caps.c    | 30 ++++++++++++++++++++++++++++++
>  target/ppc/kvm.c       | 18 ++++++++++++++++++
>  5 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bba8736269f4..b701c14b4e09 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -74,8 +74,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* Implements XIVE StoreEOI feature */
> +#define SPAPR_CAP_STOREEOI              0x0B

The name should mention XIVE, ie. SPAPR_CAP_XIVE_STOREEOI

>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_STOREEOI + 1)
>  
>  /*
>   * Capability Values
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 72e05f1cd2fc..c5a487dbba13 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void);
>  bool kvmppc_has_cap_mmu_radix(void);
>  bool kvmppc_has_cap_mmu_hash_v3(void);
>  bool kvmppc_has_cap_xive(void);
> +bool kvmppc_has_cap_xive_storeeoi(void);
>  int kvmppc_get_cap_safe_cache(void);
>  int kvmppc_get_cap_safe_bounds_check(void);
>  int kvmppc_get_cap_safe_indirect_branch(void);
> @@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void)
>      return false;
>  }
>  
> +static inline bool kvmppc_has_cap_xive_storeeoi(void)
> +{
> +    return false;
> +}
> +
>  static inline int kvmppc_get_cap_safe_cache(void)
>  {
>      return 0;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4256794f3bed..e83de0580142 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF;
>      spapr_caps_add_properties(smc);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9341e9782a3f..57c62c22e4cc 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> +                               Error **errp)
> +{
> +    ERRP_GUARD();

From "qapi/error.h":

 * = Why, when and how to use ERRP_GUARD() =
 *
 * Without ERRP_GUARD(), use of the @errp parameter is restricted:
 * - It must not be dereferenced, because it may be null.
 * - It should not be passed to error_prepend() or
 *   error_append_hint(), because that doesn't work with &error_fatal.
 * ERRP_GUARD() lifts these restrictions.
 *
 * To use ERRP_GUARD(), add it right at the beginning of the function.
 * @errp can then be used without worrying about the argument being
 * NULL or &error_fatal.
 *
 * Using it when it's not needed is safe, but please avoid cluttering
 * the source with useless code.
 *

So for this ERRP_GUARD() to be justified, you should come up with
a hint, otherwise you should drop it.

> +    MachineState *machine = MACHINE(spapr);
> +    bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi();
> +
> +    if (!val) {
> +        return; /* Disabled by default */
> +    }
> +
> +    /* Check host support when the KVM device is in use */
> +    if (kvm_irqchip_in_kernel()) {

Hmm... checking kvm_irqchip_in_kernel() is imprecise because
it returns true if either the XIVE or XICS KVM device has
been open, regardless of the flavor we're going to use. This
really depends on the ic-mode setting:

1) xics: we really don't care whether StoreEOI is available or not.
   This is very similar to the case of POWER8 in patch 2. Spit a
   warning and return.

2) xive: at this point the XIVE KVM device is open and we can check
   the availability of StoreEOI with kvm_device_check_attr().

3) dual: this one is problematic because at this point the XICS KVM
   device is open but XIVE KVM won't be open until CAS.

So I think we can only do something sensible for cases 1) and 2),
eg:

    if (!spapr->irq->xive) {
        warn_report(...);
        return;
    }

    if (spapr_xive_in_kernel(spapr->xive)) {
        !kvm_device_check_attr(spapr->xive->fd, ...) {
        error_setg(errp, "StoreEOI not supported by XIVE KVM");
        return;
    }

Case 3) requires a similar check in CAS if the guest asked for XIVE
and cap-xive-storeeoi=on.

> +        if (!kvm_storeeoi) {
> +            error_setg(errp, "StoreEOI not supported by KVM");
> +            return;
> +        }
> +    }
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_STOREEOI] = {
> +        .name = "storeeoi",
> +        .description = "Implements XIVE StoreEOI feature",
> +        .index = SPAPR_CAP_STOREEOI,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_storeeoi_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index d85ba8ffe00b..9ad637151070 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void)
>      return cap_xive;
>  }
>  
> +/*
> + * TODO: Introduce a new KVM capability
> + */

Is there anything that prevents to add such a capability
or KVM device attribute before modifying QEMU ?

> +bool kvmppc_has_cap_xive_storeeoi(void)
> +{
> +    static const char *compat = "ibm,opal-xive-pe";
> +    void *host_fdt;
> +    int xive_node;
> +
> +    host_fdt = load_device_tree_from_sysfs();
> +    xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat);
> +    if (xive_node < 0) {
> +        return false;
> +    }
> +
> +    return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL);
> +}
> +
>  static void kvmppc_get_cpu_characteristics(KVMState *s)
>  {
>      struct kvm_ppc_cpu_char c;

Re: [PATCH v2 2/6] spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:43 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/spapr_caps.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 57c62c22e4cc..b0a9d0227db2 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -535,6 +535,14 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>          return; /* Disabled by default */
>      }
>  
> +    /* For POWER8 CPUs, setting StoreEOI is useless as XIVE is not used */
> +    if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_00, 0,
> +                               spapr->max_compat_pvr)) {

It seems that this check is already done during machine init before
we get here:

 spapr_machine_init()
  spapr_irq_init()
   spapr_irq_check()

So you could maybe just check !spapr->irq->xive I think.

And s/on POWER8 CPUs/with XICS/ in the title.

> +        warn_report("StoreEOI is for the XIVE interrupt mode "
> +                    "(POWER9 and above)");
> +        return;
> +    }
> +
>      /* Check host support when the KVM device is in use */
>      if (kvm_irqchip_in_kernel()) {
>          if (!kvm_storeeoi) {

Re: [PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:44 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is
> not enforced.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/spapr_caps.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index b0a9d0227db2..9251badbdc27 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>              error_setg(errp, "StoreEOI not supported by KVM");
>              return;
>          }
> +
> +        /*
> +         * load-after-store ordering is not enforced on POWER9 CPUs
> +         * and StoreEOI can be racy.
> +         */
> +        if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
> +                                  0, spapr->max_compat_pvr)) {
> +            warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM.");

It all boils down to what "unsafe" really means here... if the outcome is
"very likely hang the guest" as soon as it starts doing I/O, shouldn't
we error out instead ? What is the motivation to use StoreEOI if the
processor doesn't really support it ?

> +        }
>      }
>  }
>  

Re: [PATCH v2 4/6] spapr/xive: Enforce load-after-store ordering

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:45 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> 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.
> 
> QEMU makes use of this offset to quiesce the sources before a
> migration. Enforce the load-after-store ordering always when doing so
> without querying the characteristics of the sources on the host. The
> performance penalty will be very small for QEMU.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/ppc/xive.h    |  8 ++++++++
>  hw/intc/spapr_xive_kvm.c | 12 ++++++++++++
>  hw/intc/xive.c           |  6 ++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 445eccfe6b73..39cd273f86d5 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -279,6 +279,14 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)
>  #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_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 66bf4c06fe55..d428422a7b72 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -357,6 +357,18 @@ 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.
> +     *
> +     * Enforce the load-after-store ordering always.
> +     */
> +    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 489e6256ef70..b710ba2df095 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -998,6 +998,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:

Re: [PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs

[Cédric Le Goater]

On 10/6/20 6:58 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:44 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is
>> not enforced.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/spapr_caps.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index b0a9d0227db2..9251badbdc27 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>>              error_setg(errp, "StoreEOI not supported by KVM");
>>              return;
>>          }
>> +
>> +        /*
>> +         * load-after-store ordering is not enforced on POWER9 CPUs
>> +         * and StoreEOI can be racy.
>> +         */
>> +        if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
>> +                                  0, spapr->max_compat_pvr)) {
>> +            warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM.");
> 
> It all boils down to what "unsafe" really means here... if the outcome is
> "very likely hang the guest" as soon as it starts doing I/O, shouldn't
> we error out instead ? What is the motivation to use StoreEOI if the
> processor doesn't really support it ?

We use it in the lab on P9. We have never seen it failed even under stress. 
But there is a possible race in the logic. 

C.

Re: [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:46 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> When the StoreEOI capability is "on", the H_INT_GET_SOURCE_INFO will
> set the StoreEOI flag for all sources. This 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 or
> deactivate StoreEOI on the machine.
> 
> This is theoretically unsafe on a POWER9 host but it still runs.
> 

Patch looks good but as said before, what is the likeliness of something
really painful to happen on a POWER9 host ?

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive.c | 1 +
>  hw/ppc/spapr_irq.c   | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 1fa09f287ac0..41f2719ff93a 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -280,6 +280,7 @@ static void spapr_xive_instance_init(Object *obj)
>      SpaprXive *xive = SPAPR_XIVE(obj);
>  
>      object_initialize_child(obj, "source", &xive->source, TYPE_XIVE_SOURCE);
> +    object_property_add_alias(obj, "flags", OBJECT(&xive->source), "flags");
>  
>      object_initialize_child(obj, "end_source", &xive->end_source,
>                              TYPE_XIVE_END_SOURCE);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index f59960339ec3..cdf9f9df4173 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -325,9 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>  
>      if (spapr->irq->xive) {
>          uint32_t nr_servers = spapr_max_server_number(spapr);
> +        uint64_t flags = 0;
>          DeviceState *dev;
>          int i;
>  
> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_ON) {
> +            flags |= XIVE_SRC_STORE_EOI;
> +        }
> +
>          dev = qdev_new(TYPE_SPAPR_XIVE);
>          qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
>          /*
> @@ -337,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>          qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
>          object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
>                                   &error_abort);
> +        object_property_set_int(OBJECT(dev), "flags", flags, &error_abort);
>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>  
>          spapr->xive = SPAPR_XIVE(dev);

Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Greg Kurz]

On Mon, 5 Oct 2020 18:51:47 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> When the StoreEOI capability is set to "cas", let CAS decide when
> StoreEOI should be advertised. StoreEOI is safe to use with a P10
> compat machine because the OS enforces load-after-store ordering but
> not with P9 compat.
> 
> The question now is : should we make "cas" the default at the machine
> level ?
> 

Hmm, spapr capabilities are knobs for the end user to provide a specific
and consistent behavior to the guest... so the "let CAS decide depending
on what the guest asked for" thing looks awkward... I mean that making
"cas" the default at the machine level looks like you don't really want
this to be a capability at all.

If the user asked for StoreEOI but the guest chose a compat mode that
can't support it, QEMU should simply exit IMHO.

Or alternatively I guess you can just forget about the spapr capability
and do all the checks (XIVE interrupt controller, KVM support, guest
asked for P10) at CAS.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr.h      |  1 +
>  include/hw/ppc/spapr_xive.h |  1 +
>  hw/intc/spapr_xive.c        |  9 +++++++++
>  hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
>  hw/ppc/spapr_hcall.c        |  7 +++++++
>  5 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b701c14b4e09..17e7d873e8dc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -87,6 +87,7 @@ typedef enum {
>  #define SPAPR_CAP_ON                    0x01
>  
>  /* Custom Caps */
> +#define SPAPR_CAP_CAS                   0x02
>  
>  /* Generic */
>  #define SPAPR_CAP_BROKEN                0x00
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 26c8d90d7196..8b8aa586e44f 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -75,6 +75,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/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 41f2719ff93a..f57a2681dd91 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1802,3 +1802,12 @@ 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);
>  }
> +
> +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/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9251badbdc27..c55e1fccb9bc 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +SpaprCapPossible cap_storeeoi_possible = {
> +    .num = 3,
> +    .vals = { "off", "on", "cas" },
> +    .help = "off - no StoreEOI, on - StoreEOI, "
> +            "cas - negotiated at CAS (POWER10 compat only)",
> +};
> +
>  static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>                                 Error **errp)
>  {
> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>              return;
>          }
>  
> +        /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */
> +        if (val == SPAPR_CAP_CAS) {
> +            return;
> +        }
> +
>          /*
>           * load-after-store ordering is not enforced on POWER9 CPUs
>           * and StoreEOI can be racy.
> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      },
>      [SPAPR_CAP_STOREEOI] = {
>          .name = "storeeoi",
> -        .description = "Implements XIVE StoreEOI feature",
> +        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
>          .index = SPAPR_CAP_STOREEOI,
> -        .get = spapr_cap_get_bool,
> -        .set = spapr_cap_set_bool,
> -        .type = "bool",
> +        .get = spapr_cap_get_string,
> +        .set = spapr_cap_set_string,
> +        .type = "string",
> +        .possible = &cap_storeeoi_possible,
>          .apply = cap_storeeoi_apply,
>      },
>  };
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 607740150fa2..158b122b9192 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
>  "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. */
> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
> +            bool 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(

Re: [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level

[Cédric Le Goater]

On 10/6/20 7:06 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:46 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> When the StoreEOI capability is "on", the H_INT_GET_SOURCE_INFO will
>> set the StoreEOI flag for all sources. This 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 or
>> deactivate StoreEOI on the machine.
>>
>> This is theoretically unsafe on a POWER9 host but it still runs.
>>
> 
> Patch looks good but as said before, what is the likeliness of something
> really painful to happen on a POWER9 host ?

Nothing will happen because POWER9 systems deactivate StoreEOI. You need
a custom skiboot to add it back.

C.


>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/spapr_xive.c | 1 +
>>  hw/ppc/spapr_irq.c   | 6 ++++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 1fa09f287ac0..41f2719ff93a 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -280,6 +280,7 @@ static void spapr_xive_instance_init(Object *obj)
>>      SpaprXive *xive = SPAPR_XIVE(obj);
>>  
>>      object_initialize_child(obj, "source", &xive->source, TYPE_XIVE_SOURCE);
>> +    object_property_add_alias(obj, "flags", OBJECT(&xive->source), "flags");
>>  
>>      object_initialize_child(obj, "end_source", &xive->end_source,
>>                              TYPE_XIVE_END_SOURCE);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index f59960339ec3..cdf9f9df4173 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -325,9 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>>  
>>      if (spapr->irq->xive) {
>>          uint32_t nr_servers = spapr_max_server_number(spapr);
>> +        uint64_t flags = 0;
>>          DeviceState *dev;
>>          int i;
>>  
>> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_ON) {
>> +            flags |= XIVE_SRC_STORE_EOI;
>> +        }
>> +
>>          dev = qdev_new(TYPE_SPAPR_XIVE);
>>          qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
>>          /*
>> @@ -337,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>>          qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
>>          object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
>>                                   &error_abort);
>> +        object_property_set_int(OBJECT(dev), "flags", flags, &error_abort);
>>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>  
>>          spapr->xive = SPAPR_XIVE(dev);
> 

Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Cédric Le Goater]

On 10/6/20 7:39 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:47 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> When the StoreEOI capability is set to "cas", let CAS decide when
>> StoreEOI should be advertised. StoreEOI is safe to use with a P10
>> compat machine because the OS enforces load-after-store ordering but
>> not with P9 compat.
>>
>> The question now is : should we make "cas" the default at the machine
>> level ?
>>
> 
> Hmm, spapr capabilities are knobs for the end user to provide a specific
> and consistent behavior to the guest... so the "let CAS decide depending
> on what the guest asked for" thing looks awkward... 

The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI
if the kernel is P10 compat and will not if it's P9 to make sure that
it can be migrated to a P9 host.  

> I mean that making
> "cas" the default at the machine level looks like you don't really want
> this to be a capability at all.

It means that you are not forcing a behaviour "off" or "on". It depends
on the guest support, P10 or P9, which is what we want as a default 
behavior to be in sync with pHyp.

> If the user asked for StoreEOI but the guest chose a compat mode that
> can't support it, QEMU should simply exit IMHO
It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use 
StoreEOI. So it should be considered more like a mitigation. But anyhow, 
the P9 systems currenly shipped never activate StoreEOI. So you will get 
the error.
 
> Or alternatively I guess you can just forget about the spapr capability
> and do all the checks (XIVE interrupt controller, KVM support, guest
> asked for P10) at CAS.

That's how I did in the fisrt patches but then you loose the ability to 
switch it off on P10 and switch on on P9. Which is good to have for tests 
and performance and for possible issues if we ever have to handle a source 
which has different charateristics. In that case, we would switch it off. 

Thanks,

C.


 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr.h      |  1 +
>>  include/hw/ppc/spapr_xive.h |  1 +
>>  hw/intc/spapr_xive.c        |  9 +++++++++
>>  hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
>>  hw/ppc/spapr_hcall.c        |  7 +++++++
>>  5 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index b701c14b4e09..17e7d873e8dc 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -87,6 +87,7 @@ typedef enum {
>>  #define SPAPR_CAP_ON                    0x01
>>  
>>  /* Custom Caps */
>> +#define SPAPR_CAP_CAS                   0x02
>>  
>>  /* Generic */
>>  #define SPAPR_CAP_BROKEN                0x00
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 26c8d90d7196..8b8aa586e44f 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -75,6 +75,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/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 41f2719ff93a..f57a2681dd91 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -1802,3 +1802,12 @@ 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);
>>  }
>> +
>> +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/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 9251badbdc27..c55e1fccb9bc 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>      }
>>  }
>>  
>> +SpaprCapPossible cap_storeeoi_possible = {
>> +    .num = 3,
>> +    .vals = { "off", "on", "cas" },
>> +    .help = "off - no StoreEOI, on - StoreEOI, "
>> +            "cas - negotiated at CAS (POWER10 compat only)",
>> +};
>> +
>>  static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>>                                 Error **errp)
>>  {
>> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>>              return;
>>          }
>>  
>> +        /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */
>> +        if (val == SPAPR_CAP_CAS) {
>> +            return;
>> +        }
>> +
>>          /*
>>           * load-after-store ordering is not enforced on POWER9 CPUs
>>           * and StoreEOI can be racy.
>> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>      },
>>      [SPAPR_CAP_STOREEOI] = {
>>          .name = "storeeoi",
>> -        .description = "Implements XIVE StoreEOI feature",
>> +        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
>>          .index = SPAPR_CAP_STOREEOI,
>> -        .get = spapr_cap_get_bool,
>> -        .set = spapr_cap_set_bool,
>> -        .type = "bool",
>> +        .get = spapr_cap_get_string,
>> +        .set = spapr_cap_set_string,
>> +        .type = "string",
>> +        .possible = &cap_storeeoi_possible,
>>          .apply = cap_storeeoi_apply,
>>      },
>>  };
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 607740150fa2..158b122b9192 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
>>  "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. */
>> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
>> +            bool 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(
> 

Re: [PATCH v2 1/6] spapr/xive: Introduce a StoreEOI capability

[Cédric Le Goater]

On 10/6/20 6:42 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:42 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> 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.
>>
>> The new StoreEOI capability adds StoreEOI support to 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.
>>
> 
> Cleaner and even required... a user could possibly run an older
> KVM that doesn't know about StoreEOI on a POWER10 host and QEMU
> would wrongly assume the feature is supported. 

Well, no, because the ESB pages of the interrupts being passthrough 
in the guest, it should be safe to use StoreEOI in a guest even if 
the host kernel is not aware of it. As long as HW is correctly 
configured by FW of course, which is what the DT property says.

I agree it's a bit of shortcut but it works. 

> Also, I guess this
> should rather be an attribute of the XIVE KVM device rather than a
> plain KVM property.

It should even be a source attribute in theory. Since QEMU does not, 
an attribute of the XIVE KVM device is fine but we would just be
looking at the device tree from KVM. So may be, it's easier to make
it global to VM without relying on the XIVE device.

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr.h |  4 +++-
>>  target/ppc/kvm_ppc.h   |  6 ++++++
>>  hw/ppc/spapr.c         |  1 +
>>  hw/ppc/spapr_caps.c    | 30 ++++++++++++++++++++++++++++++
>>  target/ppc/kvm.c       | 18 ++++++++++++++++++
>>  5 files changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index bba8736269f4..b701c14b4e09 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -74,8 +74,10 @@ typedef enum {
>>  #define SPAPR_CAP_CCF_ASSIST            0x09
>>  /* Implements PAPR FWNMI option */
>>  #define SPAPR_CAP_FWNMI                 0x0A
>> +/* Implements XIVE StoreEOI feature */
>> +#define SPAPR_CAP_STOREEOI              0x0B
> 
> The name should mention XIVE, ie. SPAPR_CAP_XIVE_STOREEOI

ok.

> 
>>  /* Num Caps */
>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_STOREEOI + 1)
>>  
>>  /*
>>   * Capability Values
>> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
>> index 72e05f1cd2fc..c5a487dbba13 100644
>> --- a/target/ppc/kvm_ppc.h
>> +++ b/target/ppc/kvm_ppc.h
>> @@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void);
>>  bool kvmppc_has_cap_mmu_radix(void);
>>  bool kvmppc_has_cap_mmu_hash_v3(void);
>>  bool kvmppc_has_cap_xive(void);
>> +bool kvmppc_has_cap_xive_storeeoi(void);
>>  int kvmppc_get_cap_safe_cache(void);
>>  int kvmppc_get_cap_safe_bounds_check(void);
>>  int kvmppc_get_cap_safe_indirect_branch(void);
>> @@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void)
>>      return false;
>>  }
>>  
>> +static inline bool kvmppc_has_cap_xive_storeeoi(void)
>> +{
>> +    return false;
>> +}
>> +
>>  static inline int kvmppc_get_cap_safe_cache(void)
>>  {
>>      return 0;
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 4256794f3bed..e83de0580142 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
>> +    smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF;
>>      spapr_caps_add_properties(smc);
>>      smc->irq = &spapr_irq_dual;
>>      smc->dr_phb_enabled = true;
>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>> index 9341e9782a3f..57c62c22e4cc 100644
>> --- a/hw/ppc/spapr_caps.c
>> +++ b/hw/ppc/spapr_caps.c
>> @@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>>      }
>>  }
>>  
>> +static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>> +                               Error **errp)
>> +{
>> +    ERRP_GUARD();
> 
> From "qapi/error.h":
> 
>  * = Why, when and how to use ERRP_GUARD() =
>  *
>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>  * - It must not be dereferenced, because it may be null.
>  * - It should not be passed to error_prepend() or
>  *   error_append_hint(), because that doesn't work with &error_fatal.
>  * ERRP_GUARD() lifts these restrictions.
>  *
>  * To use ERRP_GUARD(), add it right at the beginning of the function.
>  * @errp can then be used without worrying about the argument being
>  * NULL or &error_fatal.
>  *
>  * Using it when it's not needed is safe, but please avoid cluttering
>  * the source with useless code.
>  *
> 
> So for this ERRP_GUARD() to be justified, you should come up with
> a hint, otherwise you should drop it.

OK. 

>> +    MachineState *machine = MACHINE(spapr);
>> +    bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi();
>> +
>> +    if (!val) {
>> +        return; /* Disabled by default */
>> +    }
>> +
>> +    /* Check host support when the KVM device is in use */
>> +    if (kvm_irqchip_in_kernel()) {
> 
> Hmm... checking kvm_irqchip_in_kernel() is imprecise because
> it returns true if either the XIVE or XICS KVM device has
> been open, regardless of the flavor we're going to use. This
> really depends on the ic-mode setting:
True.
> 1) xics: we really don't care whether StoreEOI is available or not.
>    This is very similar to the case of POWER8 in patch 2. Spit a
>    warning and return.

yes

> 2) xive: at this point the XIVE KVM device is open and we can check
>    the availability of StoreEOI with kvm_device_check_attr().

yes and similar to kvmppc_has_cap_xive_storeeoi() in a sense. 

> 3) dual: this one is problematic because at this point the XICS KVM
>    device is open but XIVE KVM won't be open until CAS.
> 
> So I think we can only do something sensible for cases 1) and 2),
> eg:
> 
>     if (!spapr->irq->xive) {
>         warn_report(...);
>         return;
>     }
> 
>     if (spapr_xive_in_kernel(spapr->xive)) {
>         !kvm_device_check_attr(spapr->xive->fd, ...) {
>         error_setg(errp, "StoreEOI not supported by XIVE KVM");
>         return;
>     }
> 
> Case 3) requires a similar check in CAS if the guest asked for XIVE
> and cap-xive-storeeoi=on.

Yes. dual is more complex because we could be using the XIVE emulated
device which does not have limitations on P9. I need to take a closer 
look.


>> +        if (!kvm_storeeoi) {
>> +            error_setg(errp, "StoreEOI not supported by KVM");
>> +            return;
>> +        }
>> +    }
>> +}
>> +
>>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>      [SPAPR_CAP_HTM] = {
>>          .name = "htm",
>> @@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>          .type = "bool",
>>          .apply = cap_fwnmi_apply,
>>      },
>> +    [SPAPR_CAP_STOREEOI] = {
>> +        .name = "storeeoi",
>> +        .description = "Implements XIVE StoreEOI feature",
>> +        .index = SPAPR_CAP_STOREEOI,
>> +        .get = spapr_cap_get_bool,
>> +        .set = spapr_cap_set_bool,
>> +        .type = "bool",
>> +        .apply = cap_storeeoi_apply,
>> +    },
>>  };
>>  
>>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
>> @@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>> +SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI);
>>  
>>  void spapr_caps_init(SpaprMachineState *spapr)
>>  {
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index d85ba8ffe00b..9ad637151070 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void)
>>      return cap_xive;
>>  }
>>  
>> +/*
>> + * TODO: Introduce a new KVM capability
>> + */
> 
> Is there anything that prevents to add such a capability
> or KVM device attribute before modifying QEMU ?

no. I was just lazy as the device tree check is good enough.

Thanks for the review,

C. 


>> +bool kvmppc_has_cap_xive_storeeoi(void)
>> +{
>> +    static const char *compat = "ibm,opal-xive-pe";
>> +    void *host_fdt;
>> +    int xive_node;
>> +
>> +    host_fdt = load_device_tree_from_sysfs();
>> +    xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat);
>> +    if (xive_node < 0) {
>> +        return false;
>> +    }
>> +
>> +    return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL);
>> +}
>> +
>>  static void kvmppc_get_cpu_characteristics(KVMState *s)
>>  {
>>      struct kvm_ppc_cpu_char c;
> 

Re: [PATCH v2 1/6] spapr/xive: Introduce a StoreEOI capability

[Greg Kurz]

On Wed, 7 Oct 2020 07:59:26 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> 
> 
> On 10/6/20 6:42 PM, Greg Kurz wrote:
> > On Mon, 5 Oct 2020 18:51:42 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> 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.
> >>
> >> The new StoreEOI capability adds StoreEOI support to 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.
> >>
> > 
> > Cleaner and even required... a user could possibly run an older
> > KVM that doesn't know about StoreEOI on a POWER10 host and QEMU
> > would wrongly assume the feature is supported. 
> 
> Well, no, because the ESB pages of the interrupts being passthrough 
> in the guest, it should be safe to use StoreEOI in a guest even if 
> the host kernel is not aware of it. As long as HW is correctly 
> configured by FW of course, which is what the DT property says.
> 
> I agree it's a bit of shortcut but it works. 
> 

Ok this makes sense. I suggest you write this down in a comment in
kvmppc_has_cap_xive_storeeoi() then because it is only _obvious_ to
the very few people that know about XIVE internals.

> > Also, I guess this
> > should rather be an attribute of the XIVE KVM device rather than a
> > plain KVM property.
> 
> It should even be a source attribute in theory. Since QEMU does not, 
> an attribute of the XIVE KVM device is fine but we would just be
> looking at the device tree from KVM. So may be, it's easier to make
> it global to VM without relying on the XIVE device.
> 

Well if KVM doesn't explicitly exploit StoreEOI, not sure this
would bring much to expose this through a KVM interface at all
in the end... 

> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/spapr.h |  4 +++-
> >>  target/ppc/kvm_ppc.h   |  6 ++++++
> >>  hw/ppc/spapr.c         |  1 +
> >>  hw/ppc/spapr_caps.c    | 30 ++++++++++++++++++++++++++++++
> >>  target/ppc/kvm.c       | 18 ++++++++++++++++++
> >>  5 files changed, 58 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index bba8736269f4..b701c14b4e09 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -74,8 +74,10 @@ typedef enum {
> >>  #define SPAPR_CAP_CCF_ASSIST            0x09
> >>  /* Implements PAPR FWNMI option */
> >>  #define SPAPR_CAP_FWNMI                 0x0A
> >> +/* Implements XIVE StoreEOI feature */
> >> +#define SPAPR_CAP_STOREEOI              0x0B
> > 
> > The name should mention XIVE, ie. SPAPR_CAP_XIVE_STOREEOI
> 
> ok.
> 
> > 
> >>  /* Num Caps */
> >> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> >> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_STOREEOI + 1)
> >>  
> >>  /*
> >>   * Capability Values
> >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> >> index 72e05f1cd2fc..c5a487dbba13 100644
> >> --- a/target/ppc/kvm_ppc.h
> >> +++ b/target/ppc/kvm_ppc.h
> >> @@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void);
> >>  bool kvmppc_has_cap_mmu_radix(void);
> >>  bool kvmppc_has_cap_mmu_hash_v3(void);
> >>  bool kvmppc_has_cap_xive(void);
> >> +bool kvmppc_has_cap_xive_storeeoi(void);
> >>  int kvmppc_get_cap_safe_cache(void);
> >>  int kvmppc_get_cap_safe_bounds_check(void);
> >>  int kvmppc_get_cap_safe_indirect_branch(void);
> >> @@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void)
> >>      return false;
> >>  }
> >>  
> >> +static inline bool kvmppc_has_cap_xive_storeeoi(void)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >>  static inline int kvmppc_get_cap_safe_cache(void)
> >>  {
> >>      return 0;
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 4256794f3bed..e83de0580142 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> >>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> >>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> >> +    smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF;
> >>      spapr_caps_add_properties(smc);
> >>      smc->irq = &spapr_irq_dual;
> >>      smc->dr_phb_enabled = true;
> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >> index 9341e9782a3f..57c62c22e4cc 100644
> >> --- a/hw/ppc/spapr_caps.c
> >> +++ b/hw/ppc/spapr_caps.c
> >> @@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
> >>      }
> >>  }
> >>  
> >> +static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> >> +                               Error **errp)
> >> +{
> >> +    ERRP_GUARD();
> > 
> > From "qapi/error.h":
> > 
> >  * = Why, when and how to use ERRP_GUARD() =
> >  *
> >  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> >  * - It must not be dereferenced, because it may be null.
> >  * - It should not be passed to error_prepend() or
> >  *   error_append_hint(), because that doesn't work with &error_fatal.
> >  * ERRP_GUARD() lifts these restrictions.
> >  *
> >  * To use ERRP_GUARD(), add it right at the beginning of the function.
> >  * @errp can then be used without worrying about the argument being
> >  * NULL or &error_fatal.
> >  *
> >  * Using it when it's not needed is safe, but please avoid cluttering
> >  * the source with useless code.
> >  *
> > 
> > So for this ERRP_GUARD() to be justified, you should come up with
> > a hint, otherwise you should drop it.
> 
> OK. 
> 
> >> +    MachineState *machine = MACHINE(spapr);
> >> +    bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi();
> >> +
> >> +    if (!val) {
> >> +        return; /* Disabled by default */
> >> +    }
> >> +
> >> +    /* Check host support when the KVM device is in use */
> >> +    if (kvm_irqchip_in_kernel()) {
> > 
> > Hmm... checking kvm_irqchip_in_kernel() is imprecise because
> > it returns true if either the XIVE or XICS KVM device has
> > been open, regardless of the flavor we're going to use. This
> > really depends on the ic-mode setting:
> True.
> > 1) xics: we really don't care whether StoreEOI is available or not.
> >    This is very similar to the case of POWER8 in patch 2. Spit a
> >    warning and return.
> 
> yes
> 
> > 2) xive: at this point the XIVE KVM device is open and we can check
> >    the availability of StoreEOI with kvm_device_check_attr().
> 
> yes and similar to kvmppc_has_cap_xive_storeeoi() in a sense. 
> 
> > 3) dual: this one is problematic because at this point the XICS KVM
> >    device is open but XIVE KVM won't be open until CAS.
> > 
> > So I think we can only do something sensible for cases 1) and 2),
> > eg:
> > 
> >     if (!spapr->irq->xive) {
> >         warn_report(...);
> >         return;
> >     }
> > 
> >     if (spapr_xive_in_kernel(spapr->xive)) {
> >         !kvm_device_check_attr(spapr->xive->fd, ...) {
> >         error_setg(errp, "StoreEOI not supported by XIVE KVM");
> >         return;
> >     }
> > 
> > Case 3) requires a similar check in CAS if the guest asked for XIVE
> > and cap-xive-storeeoi=on.
> 
> Yes. dual is more complex because we could be using the XIVE emulated
> device which does not have limitations on P9. I need to take a closer 
> look.
> 

Yeah, we should take kernel-irqchip into account as well...

> 
> >> +        if (!kvm_storeeoi) {
> >> +            error_setg(errp, "StoreEOI not supported by KVM");
> >> +            return;
> >> +        }
> >> +    }
> >> +}
> >> +
> >>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>      [SPAPR_CAP_HTM] = {
> >>          .name = "htm",
> >> @@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>          .type = "bool",
> >>          .apply = cap_fwnmi_apply,
> >>      },
> >> +    [SPAPR_CAP_STOREEOI] = {
> >> +        .name = "storeeoi",
> >> +        .description = "Implements XIVE StoreEOI feature",
> >> +        .index = SPAPR_CAP_STOREEOI,
> >> +        .get = spapr_cap_get_bool,
> >> +        .set = spapr_cap_set_bool,
> >> +        .type = "bool",
> >> +        .apply = cap_storeeoi_apply,
> >> +    },
> >>  };
> >>  
> >>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> >> @@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> >>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
> >>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> >>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> >> +SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI);
> >>  
> >>  void spapr_caps_init(SpaprMachineState *spapr)
> >>  {
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index d85ba8ffe00b..9ad637151070 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void)
> >>      return cap_xive;
> >>  }
> >>  
> >> +/*
> >> + * TODO: Introduce a new KVM capability
> >> + */
> > 
> > Is there anything that prevents to add such a capability
> > or KVM device attribute before modifying QEMU ?
> 
> no. I was just lazy as the device tree check is good enough.
> 
> Thanks for the review,
> 
> C. 
> 
> 
> >> +bool kvmppc_has_cap_xive_storeeoi(void)
> >> +{
> >> +    static const char *compat = "ibm,opal-xive-pe";
> >> +    void *host_fdt;
> >> +    int xive_node;
> >> +
> >> +    host_fdt = load_device_tree_from_sysfs();
> >> +    xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat);
> >> +    if (xive_node < 0) {
> >> +        return false;
> >> +    }
> >> +
> >> +    return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL);
> >> +}
> >> +
> >>  static void kvmppc_get_cpu_characteristics(KVMState *s)
> >>  {
> >>      struct kvm_ppc_cpu_char c;
> > 

Re: [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level

[Greg Kurz]

On Tue, 6 Oct 2020 19:41:28 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 10/6/20 7:06 PM, Greg Kurz wrote:
> > On Mon, 5 Oct 2020 18:51:46 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> When the StoreEOI capability is "on", the H_INT_GET_SOURCE_INFO will
> >> set the StoreEOI flag for all sources. This 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 or
> >> deactivate StoreEOI on the machine.
> >>
> >> This is theoretically unsafe on a POWER9 host but it still runs.
> >>
> > 
> > Patch looks good but as said before, what is the likeliness of something
> > really painful to happen on a POWER9 host ?
> 
> Nothing will happen because POWER9 systems deactivate StoreEOI. You need
> a custom skiboot to add it back.
> 

Ok, then:

Reviewed-by: Greg Kurz <groug@kaod.org>

> C.
> 
> 
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/intc/spapr_xive.c | 1 +
> >>  hw/ppc/spapr_irq.c   | 6 ++++++
> >>  2 files changed, 7 insertions(+)
> >>
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 1fa09f287ac0..41f2719ff93a 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -280,6 +280,7 @@ static void spapr_xive_instance_init(Object *obj)
> >>      SpaprXive *xive = SPAPR_XIVE(obj);
> >>  
> >>      object_initialize_child(obj, "source", &xive->source, TYPE_XIVE_SOURCE);
> >> +    object_property_add_alias(obj, "flags", OBJECT(&xive->source), "flags");
> >>  
> >>      object_initialize_child(obj, "end_source", &xive->end_source,
> >>                              TYPE_XIVE_END_SOURCE);
> >> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >> index f59960339ec3..cdf9f9df4173 100644
> >> --- a/hw/ppc/spapr_irq.c
> >> +++ b/hw/ppc/spapr_irq.c
> >> @@ -325,9 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> >>  
> >>      if (spapr->irq->xive) {
> >>          uint32_t nr_servers = spapr_max_server_number(spapr);
> >> +        uint64_t flags = 0;
> >>          DeviceState *dev;
> >>          int i;
> >>  
> >> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_ON) {
> >> +            flags |= XIVE_SRC_STORE_EOI;
> >> +        }
> >> +
> >>          dev = qdev_new(TYPE_SPAPR_XIVE);
> >>          qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + SPAPR_XIRQ_BASE);
> >>          /*
> >> @@ -337,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> >>          qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
> >>          object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
> >>                                   &error_abort);
> >> +        object_property_set_int(OBJECT(dev), "flags", flags, &error_abort);
> >>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> >>  
> >>          spapr->xive = SPAPR_XIVE(dev);
> > 
> 

Re: [PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs

[Greg Kurz]

On Tue, 6 Oct 2020 19:03:28 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 10/6/20 6:58 PM, Greg Kurz wrote:
> > On Mon, 5 Oct 2020 18:51:44 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is
> >> not enforced.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/ppc/spapr_caps.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >> index b0a9d0227db2..9251badbdc27 100644
> >> --- a/hw/ppc/spapr_caps.c
> >> +++ b/hw/ppc/spapr_caps.c
> >> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> >>              error_setg(errp, "StoreEOI not supported by KVM");
> >>              return;
> >>          }
> >> +
> >> +        /*
> >> +         * load-after-store ordering is not enforced on POWER9 CPUs
> >> +         * and StoreEOI can be racy.
> >> +         */
> >> +        if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
> >> +                                  0, spapr->max_compat_pvr)) {
> >> +            warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM.");
> > 

The error message should mention XIVE KVM device actually since this only
depends on kernel-irqchip.

> > It all boils down to what "unsafe" really means here... if the outcome is
> > "very likely hang the guest" as soon as it starts doing I/O, shouldn't
> > we error out instead ? What is the motivation to use StoreEOI if the
> > processor doesn't really support it ?
> 
> We use it in the lab on P9. We have never seen it failed even under stress. 
> But there is a possible race in the logic. 
> 
> C.

Thinking again. P9 boston systems on the field only use emulated XIVE and
we certainly want to be able to migrate to P9 systems that use in-kernel
XIVE and vice-versa. So, even if StoreEOI is technically supported by
the emulated XIVE, maybe we should disallow it anyway ?

Re: [PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs

[Cédric Le Goater]

On 10/7/20 10:56 AM, Greg Kurz wrote:
> On Tue, 6 Oct 2020 19:03:28 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 10/6/20 6:58 PM, Greg Kurz wrote:
>>> On Mon, 5 Oct 2020 18:51:44 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is
>>>> not enforced.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  hw/ppc/spapr_caps.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>> index b0a9d0227db2..9251badbdc27 100644
>>>> --- a/hw/ppc/spapr_caps.c
>>>> +++ b/hw/ppc/spapr_caps.c
>>>> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
>>>>              error_setg(errp, "StoreEOI not supported by KVM");
>>>>              return;
>>>>          }
>>>> +
>>>> +        /*
>>>> +         * load-after-store ordering is not enforced on POWER9 CPUs
>>>> +         * and StoreEOI can be racy.
>>>> +         */
>>>> +        if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
>>>> +                                  0, spapr->max_compat_pvr)) {
>>>> +            warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM.");
>>>
> 
> The error message should mention XIVE KVM device actually since this only
> depends on kernel-irqchip.
> 
>>> It all boils down to what "unsafe" really means here... if the outcome is
>>> "very likely hang the guest" as soon as it starts doing I/O, shouldn't
>>> we error out instead ? What is the motivation to use StoreEOI if the
>>> processor doesn't really support it ?
>>
>> We use it in the lab on P9. We have never seen it failed even under stress. 
>> But there is a possible race in the logic. 
>>
>> C.
> 
> Thinking again. P9 boston systems on the field only use emulated XIVE and
> we certainly want to be able to migrate to P9 systems that use in-kernel
> XIVE and vice-versa. So, even if StoreEOI is technically supported by
> the emulated XIVE, maybe we should disallow it anyway ?

yes. But to activate it, one would need to set the capability to "on"
because the default is "off". 

Even with the "cas" capability value introduced at the end of the 
patchset, StoreEOI would not be advertised to guests running under 
P9 compat. 

C.

Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Greg Kurz]

On Tue, 6 Oct 2020 19:56:20 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 10/6/20 7:39 PM, Greg Kurz wrote:
> > On Mon, 5 Oct 2020 18:51:47 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> When the StoreEOI capability is set to "cas", let CAS decide when
> >> StoreEOI should be advertised. StoreEOI is safe to use with a P10
> >> compat machine because the OS enforces load-after-store ordering but
> >> not with P9 compat.

Hmm... OS enforcing load-after-store based on a PVR ? I can't find
such a thing in the kernel sources. What I find is:

	if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
		offset |= XIVE_ESB_LD_ST_MO;

where xd->flags comes from H_INT_GET_SOURCE_INFO, so this essentially
depends on what the hypervisor advertises. For example a P9 compat
guest could use it if QEMU has an emulated XIVE, right ?

> >>
> >> The question now is : should we make "cas" the default at the machine
> >> level ?
> >>
> > 
> > Hmm, spapr capabilities are knobs for the end user to provide a specific
> > and consistent behavior to the guest... so the "let CAS decide depending
> > on what the guest asked for" thing looks awkward... 
> 
> The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI
> if the kernel is P10 compat and will not if it's P9 to make sure that
> it can be migrated to a P9 host.  
> 

Yeah I know that the guest doesn't ask for StoreEOI. What I mean is that
the idea behind spapr capabilities is only about saying "this machine type
offers this feature and the host supports it". The fact that the guest
might use it or not is another matter. So I don't quite see how "cas" fits
into this model...

> > I mean that making
> > "cas" the default at the machine level looks like you don't really want
> > this to be a capability at all.
> 
> It means that you are not forcing a behaviour "off" or "on". It depends
> on the guest support, P10 or P9, which is what we want as a default 
> behavior to be in sync with pHyp.
> 

I think there's confusion here. Setting the capability to "on"/"off"
doesn't mean your force anything. As said above, this isn't related
to what the guest supports but essentially to what the host supports.
"on" means that we can offer the feature to the guest in a reliable
manner because we have verified it can work, and "off" simply means
we don't offer the feature (but of course the guest is still free
to shoot itself in the foot).

The reference to what pHyp does is thus irrelevant here because we
won't ever migrate a guest between KVM and PowerVM. We could decide
to do nothing and let the OS only do "LoadEOI" operations as it does
today.

If we want StoreEOI to be usable by default, then the default value
should definitely be "on". This should be kept to "off" for older
machine types to stay compatible with existing setups.

Let's only consider the KVM case only because we don't explicitly
support KVM<->TCG migration. The availability of StoreEOI is:
- emulated XIVE on P9 system => ok
- emulated XIVE on P10 system => ok
- in-kernel XIVE on P10 system => ok
- in-kernel XIVE on P9 system => not ok

This means that on a P9 system we must fallback on emulated XIVE, like
we already do with bostons today. This might look sub-optimal from a
performance standpoint but this is the price to pay to guarantee
migration doesn't fail with the default settings.

> > If the user asked for StoreEOI but the guest chose a compat mode that
> > can't support it, QEMU should simply exit IMHO
> It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use 
> StoreEOI. So it should be considered more like a mitigation. But anyhow, 
> the P9 systems currenly shipped never activate StoreEOI. So you will get 
> the error.
>  
> > Or alternatively I guess you can just forget about the spapr capability
> > and do all the checks (XIVE interrupt controller, KVM support, guest
> > asked for P10) at CAS.
> 
> That's how I did in the fisrt patches but then you loose the ability to 
> switch it off on P10 and switch on on P9. Which is good to have for tests 
> and performance and for possible issues if we ever have to handle a source 
> which has different charateristics. In that case, we would switch it off. 
> 

I understand that having fine grain knobs is cool for tests and performance
but keep in mind that we mostly target end-users who expect this to work
out of the box with the default settings. This being said, if some end-users
might hit a corner case where it would be valuable to switch it off, then
it makes sense to keep the ability to do so of course.

> Thanks,
> 
> C.
> 
> 
>  
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/spapr.h      |  1 +
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  hw/intc/spapr_xive.c        |  9 +++++++++
> >>  hw/ppc/spapr_caps.c         | 21 +++++++++++++++++----
> >>  hw/ppc/spapr_hcall.c        |  7 +++++++
> >>  5 files changed, 35 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index b701c14b4e09..17e7d873e8dc 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -87,6 +87,7 @@ typedef enum {
> >>  #define SPAPR_CAP_ON                    0x01
> >>  
> >>  /* Custom Caps */
> >> +#define SPAPR_CAP_CAS                   0x02
> >>  
> >>  /* Generic */
> >>  #define SPAPR_CAP_BROKEN                0x00
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 26c8d90d7196..8b8aa586e44f 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -75,6 +75,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/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 41f2719ff93a..f57a2681dd91 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -1802,3 +1802,12 @@ 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);
> >>  }
> >> +
> >> +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/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >> index 9251badbdc27..c55e1fccb9bc 100644
> >> --- a/hw/ppc/spapr_caps.c
> >> +++ b/hw/ppc/spapr_caps.c
> >> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
> >>      }
> >>  }
> >>  
> >> +SpaprCapPossible cap_storeeoi_possible = {
> >> +    .num = 3,
> >> +    .vals = { "off", "on", "cas" },
> >> +    .help = "off - no StoreEOI, on - StoreEOI, "
> >> +            "cas - negotiated at CAS (POWER10 compat only)",
> >> +};
> >> +
> >>  static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> >>                                 Error **errp)
> >>  {
> >> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val,
> >>              return;
> >>          }
> >>  
> >> +        /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */
> >> +        if (val == SPAPR_CAP_CAS) {
> >> +            return;
> >> +        }
> >> +
> >>          /*
> >>           * load-after-store ordering is not enforced on POWER9 CPUs
> >>           * and StoreEOI can be racy.
> >> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >>      },
> >>      [SPAPR_CAP_STOREEOI] = {
> >>          .name = "storeeoi",
> >> -        .description = "Implements XIVE StoreEOI feature",
> >> +        .description = "Implements XIVE StoreEOI feature (off, on, cas)",
> >>          .index = SPAPR_CAP_STOREEOI,
> >> -        .get = spapr_cap_get_bool,
> >> -        .set = spapr_cap_set_bool,
> >> -        .type = "bool",
> >> +        .get = spapr_cap_get_string,
> >> +        .set = spapr_cap_set_string,
> >> +        .type = "string",
> >> +        .possible = &cap_storeeoi_possible,
> >>          .apply = cap_storeeoi_apply,
> >>      },
> >>  };
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 607740150fa2..158b122b9192 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
> >>  "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. */
> >> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) {
> >> +            bool 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(
> > 
> 

Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

[Cédric Le Goater]

> I understand that having fine grain knobs is cool for tests and performance
> but keep in mind that we mostly target end-users who expect this to work
> out of the box with the default settings. This being said, if some end-users
> might hit a corner case where it would be valuable to switch it off, then
> it makes sense to keep the ability to do so of course.

Yes. Since StoreEOI only applies to P10, we could simplify. We can address 
that when there is a need in mainline. 

Thanks for the inputs,

C. 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

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 form of load that has the ordering, only on available 
on P10. It's a no-op on P9. 

Linux commit b1f9be9392f0 ("powerpc/xive: Enforce load-after-store  
ordering when StoreEOI is active") introduced the Load-after-Store 
ordering offset and P10 support was added in the same 5.8 release.

This is why StoreEOI should be advertised on P10 compat kernels only. 
I would have preferred to introduce some extra CAS bits. that would 
have been cleaner than mix the two.


The basic requirement is to advertise StoreEOI when the CPU compat
allows it. I have used the capabilities to toggle the feature on/off.
It seemed a clean way to cover all the extra needs : 

 - switch it off on P10 if needed
 - switch it on on P9 for tests

 
> Also, weirdly, despite the series being addressed to me, only some of
> the patches ended up in my inbox, rather than the list folder :/.


Yes. I have received a few ot these : 
 
The original message was received at Mon, 5 Oct 2020 12:51:56 -0400
from m0098419.ppops.net [127.0.0.1]

   ----- The following addresses had permanent fatal errors -----
<david@gibson.dropbear.id.au>

   ----- Transcript of session follows -----
<david@gibson.dropbear.id.au>... Deferred: Name server: gibson.dropbear.id.au.: host name lookup failure
Message could not be delivered for 1 day
Message will be deleted from queue


Reporting-MTA: dns; mx0b-001b2d01.pphosted.com
Arrival-Date: Mon, 5 Oct 2020 12:51:56 -0400

Final-Recipient: RFC822; david@gibson.dropbear.id.au
Action: failed
Status: 4.4.7
Remote-MTA: DNS; gibson.dropbear.id.au
Last-Attempt-Date: Tue, 6 Oct 2020 13:06:28 -0400


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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]

On Fri, Oct 09, 2020 at 07:57:32AM +0200, Cédric Le Goater wrote:
> 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 form of load that has the ordering, only on available 
> on P10. It's a no-op on P9.

no-op as in the load will have regular semantics, or as in the whole
load won't do anything?

I assume this meanse XIVE code needs to be updated to use that special
load for all accesses to XIVE registers... 

> Linux commit b1f9be9392f0 ("powerpc/xive: Enforce load-after-store  
> ordering when StoreEOI is active") introduced the Load-after-Store 
> ordering offset and P10 support was added in the same 5.8 release.

.. which I guess this does?

> This is why StoreEOI should be advertised on P10 compat kernels only. 
> I would have preferred to introduce some extra CAS bits. that would 
> have been cleaner than mix the two.

Ok.

> The basic requirement is to advertise StoreEOI when the CPU compat
> allows it. I have used the capabilities to toggle the feature on/off.
> It seemed a clean way to cover all the extra needs : 
> 
>  - switch it off on P10 if needed
>  - switch it on on P9 for tests

Ok, seems reasonable

> > Also, weirdly, despite the series being addressed to me, only some of
> > the patches ended up in my inbox, rather than the list folder :/.
> 
> 
> Yes. I have received a few ot these : 
>  
> The original message was received at Mon, 5 Oct 2020 12:51:56 -0400
> from m0098419.ppops.net [127.0.0.1]
> 
>    ----- The following addresses had permanent fatal errors -----
> <david@gibson.dropbear.id.au>

Drat, I guess ozlabs.org fell off the net for a while.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5118 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cédric Le Goater]

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.