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

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

[Cédric Le Goater]

Hello,

The first patch is a little improvement on how we deal with priorities
reserved by the hypervisor. The rest is about activating StoreEOI safely
on a host supporting it, P10 hosts are the target but experimental
P9 firmwares activate also StoreEOI.

The patchset adds a new KVM device command KVM_DEV_XIVE_GRP_SOURCE_INFO
(yet to be sent but nothing really complex) which lets QEMU query the
XIVE characteristics of the underlying HW interrupts. It also enforces
StoreEOI on P10 compat guests only to make sure that P9 compat guests
can not be migrated to a P9 host with StoreEOI activated. Nevertheless,
as this is useful, a new Spapr IRQ backend 'xive-storeeoi' allows the
user run a P9 compat guest with StoreEOI enabled.

Please comment because there might be a better alternative to address
these needs. 

Thanks,

C.

Cédric Le Goater (8):
  spapr/xive: Add a 'hv-prio' property to represent the KVM escalation
    priority
  linux-headers: Update for KVM_DEV_XIVE_GRP_SOURCE_INFO
  spapr/xive: Query the characteristics of a source in KVM
  spapr/xive: Activate StoreEOI for POWER10 only
  spapr/xive: Enforce the load-after-store ordering
  spapr/xive: Activate StoreEOI by default
  spapr/xive: Use the xics flag to check for XIVE-only IRQ backends
  spapr/xive: Introduce a XIVE StoreEOI IRQ backend

 include/hw/ppc/spapr_irq.h      |  1 +
 include/hw/ppc/spapr_xive.h     |  5 +++
 include/hw/ppc/xive.h           |  8 +++++
 linux-headers/asm-powerpc/kvm.h |  8 +++++
 hw/intc/spapr_xive.c            | 56 ++++++++++++++++++++++-----------
 hw/intc/spapr_xive_kvm.c        | 38 ++++++++++++++++++++++
 hw/intc/xive.c                  |  6 ++++
 hw/ppc/spapr.c                  |  6 +++-
 hw/ppc/spapr_irq.c              | 36 ++++++++++++++++++++-
 9 files changed, 143 insertions(+), 21 deletions(-)

-- 
2.25.4

[PATCH 1/8] spapr/xive: Add a 'hv-prio' property to represent the KVM escalation priority

[Cédric Le Goater]

On POWER9, the KVM XIVE device uses priority 7 for the escalation
interrupts. On POWER10, the host can use a reduced set of priorities
and KVM will configure the escalation priority to a lower number. In
any case, the guest is allowed to use priorities in a single range :

    [ 0 .. (maxprio - 1) ].

Introduce a 'hv-prio' property to represent the escalation priority
number and use it to compute the "ibm,plat-res-int-priorities"
property defining the priority ranges reserved by the hypervisor.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_xive.h |  2 ++
 hw/intc/spapr_xive.c        | 33 ++++++++++++++-------------------
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 0ffbe0be0280..1dddcbcb9cdd 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -49,6 +49,8 @@ typedef struct SpaprXive {
     void          *tm_mmap;
     MemoryRegion  tm_mmio_kvm;
     VMChangeStateEntry *change;
+
+    uint8_t       hv_prio;
 } SpaprXive;
 
 typedef struct SpaprXiveClass {
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 4bd0d606ba17..1fa09f287ac0 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -595,6 +595,7 @@ static Property spapr_xive_properties[] = {
     DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0),
     DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE),
     DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE),
+    DEFINE_PROP_UINT8("hv-prio", SpaprXive, hv_prio, 7),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -692,12 +693,13 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
         cpu_to_be32(16), /* 64K */
     };
     /*
-     * The following array is in sync with the reserved priorities
-     * defined by the 'spapr_xive_priority_is_reserved' routine.
+     * QEMU/KVM only needs to define a single range to reserve the
+     * escalation priority. A priority bitmask would have been more
+     * appropriate.
      */
     uint32_t plat_res_int_priorities[] = {
-        cpu_to_be32(7),    /* start */
-        cpu_to_be32(0xf8), /* count */
+        cpu_to_be32(xive->hv_prio),    /* start */
+        cpu_to_be32(0xff - xive->hv_prio), /* count */
     };
 
     /* Thread Interrupt Management Area : User (ring 3) and OS (ring 2) */
@@ -844,19 +846,12 @@ type_init(spapr_xive_register_types)
  */
 
 /*
- * Linux hosts under OPAL reserve priority 7 for their own escalation
- * interrupts (DD2.X POWER9). So we only allow the guest to use
- * priorities [0..6].
+ * On POWER9, the KVM XIVE device uses priority 7 for the escalation
+ * interrupts. So we only allow the guest to use priorities [0..6].
  */
-static bool spapr_xive_priority_is_reserved(uint8_t priority)
+static bool spapr_xive_priority_is_reserved(SpaprXive *xive, uint8_t priority)
 {
-    switch (priority) {
-    case 0 ... 6:
-        return false;
-    case 7: /* OPAL escalation queue */
-    default:
-        return true;
-    }
+    return priority >= xive->hv_prio;
 }
 
 /*
@@ -1053,7 +1048,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu,
         new_eas.w = eas.w & cpu_to_be64(~EAS_MASKED);
     }
 
-    if (spapr_xive_priority_is_reserved(priority)) {
+    if (spapr_xive_priority_is_reserved(xive, priority)) {
         qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
                       " is reserved\n", priority);
         return H_P4;
@@ -1212,7 +1207,7 @@ static target_ulong h_int_get_queue_info(PowerPCCPU *cpu,
      * This is not needed when running the emulation under QEMU
      */
 
-    if (spapr_xive_priority_is_reserved(priority)) {
+    if (spapr_xive_priority_is_reserved(xive, priority)) {
         qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
                       " is reserved\n", priority);
         return H_P3;
@@ -1299,7 +1294,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
      * This is not needed when running the emulation under QEMU
      */
 
-    if (spapr_xive_priority_is_reserved(priority)) {
+    if (spapr_xive_priority_is_reserved(xive, priority)) {
         qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
                       " is reserved\n", priority);
         return H_P3;
@@ -1466,7 +1461,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
      * This is not needed when running the emulation under QEMU
      */
 
-    if (spapr_xive_priority_is_reserved(priority)) {
+    if (spapr_xive_priority_is_reserved(xive, priority)) {
         qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
                       " is reserved\n", priority);
         return H_P3;
-- 
2.25.4

[PATCH 2/8] linux-headers: Update for KVM_DEV_XIVE_GRP_SOURCE_INFO

[Cédric Le Goater]

To be sent with the linux-headers update when support is merged.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 linux-headers/asm-powerpc/kvm.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
index 264e266a85bf..aeb8e8c4633b 100644
--- a/linux-headers/asm-powerpc/kvm.h
+++ b/linux-headers/asm-powerpc/kvm.h
@@ -690,6 +690,7 @@ struct kvm_ppc_cpu_char {
 #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG	3	/* 64-bit source identifier */
 #define KVM_DEV_XIVE_GRP_EQ_CONFIG	4	/* 64-bit EQ identifier */
 #define KVM_DEV_XIVE_GRP_SOURCE_SYNC	5       /* 64-bit source identifier */
+#define KVM_DEV_XIVE_GRP_SOURCE_INFO	6       /* 64-bit source identifier */
 
 /* Layout of 64-bit XIVE source attribute values */
 #define KVM_XIVE_LEVEL_SENSITIVE	(1ULL << 0)
@@ -721,6 +722,13 @@ struct kvm_ppc_xive_eq {
 	__u8  pad[40];
 };
 
+#define KVM_XIVE_SOURCE_FLAG_STORE_EOI	0x00000001
+
+/* Layout of source characteristics (8 bytes) */
+struct kvm_ppc_xive_src {
+	__u64 flags;
+};
+
 #define KVM_XIVE_EQ_ALWAYS_NOTIFY	0x00000001
 
 #define KVM_XIVE_TIMA_PAGE_OFFSET	0
-- 
2.25.4

[PATCH 3/8] spapr/xive: Query the characteristics of a source in KVM

[Cédric Le Goater]

When running a guest with a kernel IRQ chip enabled, the XIVE
characteristics of the interrupts are advertised to the guest in the
H_INT_GET_SOURCE_INFO hcall. These characteristics depend on the
underlying HW interrupts but today, QEMU simply advertises its own
without checking what the host supports. It is not a problem for the
moment, but POWER10 will (re)add support for StoreEOI and we need a
way to in sync with the host.

The KVM_DEV_XIVE_GRP_SOURCE_INFO command lets QEMU query the XIVE
characteristics of the underlying HW interrupts and override any
previous setting done by QEMU. This allows the fallback mode, when the
XIVE device is emulated by QEMU, to use its own custom settings on the
sources but makes sure that we don't let a guest run with features
incompatible with KVM.

It only applies to the StoreEOI feature for the moment.

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

diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 1dddcbcb9cdd..3f325723ea74 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -84,6 +84,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc);
 void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
 int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
                                   Error **errp);
+int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
+                                 Error **errp);
 void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
 uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
                             uint64_t data, bool write);
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 1fa09f287ac0..943b9958a68b 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -932,6 +932,26 @@ static target_ulong h_int_get_source_info(PowerPCCPU *cpu,
         args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
     }
 
+    if (kvm_irqchip_in_kernel()) {
+        Error *local_err = NULL;
+        uint64_t flags = 0;
+
+        kvmppc_xive_get_source_info(xive, lisn, &flags, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            return H_HARDWARE;
+        }
+
+        /*
+         * Override QEMU settings with KVM values
+         */
+        if (flags & XIVE_SRC_STORE_EOI) {
+            args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
+        } else {
+            args[0] &= ~SPAPR_XIVE_SRC_STORE_EOI;
+        }
+    }
+
     /*
      * Force the use of the H_INT_ESB hcall in case of an LSI
      * interrupt. This is necessary under KVM to re-trigger the
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index e8667ce5f621..90f4509e6959 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -217,6 +217,32 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
                              &kvm_src, true, errp);
 }
 
+int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
+                                 Error **errp)
+{
+    struct kvm_ppc_xive_src kvm_src = { 0 };
+    int ret;
+
+    /*
+     * Check that KVM supports the new attribute to query source
+     * characteristics.
+     */
+    if (!kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, 0)) {
+        return 0;
+    }
+
+    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, lisn,
+                            &kvm_src, false, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (kvm_src.flags & KVM_XIVE_SOURCE_FLAG_STORE_EOI) {
+        *flags |= XIVE_SRC_STORE_EOI;
+    }
+    return 0;
+}
+
 void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
 {
     kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_SYNC, lisn,
-- 
2.25.4

[PATCH 4/8] spapr/xive: Activate StoreEOI for POWER10 only

[Cédric Le Goater]

The StoreEOI features is safe to use with a P10 compat machine but not
with P9 compat, as it can not be migrated to a P9 host.

Introdude a "hw-storeeoi" property in the SpaprXive model to check for
the availability of StoreEOI at the HW level when a kernel IRQ chip is
in use. XIVE emulated is not impacted.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_xive.h |  1 +
 hw/intc/spapr_xive.c        |  3 ++-
 hw/ppc/spapr_irq.c          | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 3f325723ea74..402e38a7cf5e 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -51,6 +51,7 @@ typedef struct SpaprXive {
     VMChangeStateEntry *change;
 
     uint8_t       hv_prio;
+    bool          hw_storeeoi;
 } SpaprXive;
 
 typedef struct SpaprXiveClass {
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 943b9958a68b..d184d17e30e7 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -596,6 +596,7 @@ static Property spapr_xive_properties[] = {
     DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE),
     DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE),
     DEFINE_PROP_UINT8("hv-prio", SpaprXive, hv_prio, 7),
+    DEFINE_PROP_BOOL("hw-storeeoi", SpaprXive, hw_storeeoi, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -945,7 +946,7 @@ static target_ulong h_int_get_source_info(PowerPCCPU *cpu,
         /*
          * Override QEMU settings with KVM values
          */
-        if (flags & XIVE_SRC_STORE_EOI) {
+        if (xive->hw_storeeoi && flags & XIVE_SRC_STORE_EOI) {
             args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
         } else {
             args[0] &= ~SPAPR_XIVE_SRC_STORE_EOI;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 72bb938375ef..80cf1c3d6bb2 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -199,6 +199,23 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
     return 0;
 }
 
+static bool spapr_irq_xive_hw_storeeoi(SpaprMachineState *spapr)
+{
+    MachineState *machine = MACHINE(spapr);
+
+    /*
+     * All P10 compat kernels should enforce load-after-store ordering
+     * for StoreEOI.
+     */
+    if (ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10,
+                              0, spapr->max_compat_pvr)) {
+        return true;
+    }
+
+    /* StoreEOI on P9 compat is unsafe */
+    return false;
+}
+
 /*
  * sPAPR IRQ frontend routines for devices
  */
@@ -325,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
 
     if (spapr->irq->xive) {
         uint32_t nr_servers = spapr_max_server_number(spapr);
+        bool storeeoi = spapr_irq_xive_hw_storeeoi(spapr);
         DeviceState *dev;
         int i;
 
@@ -337,6 +355,8 @@ 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_bool(OBJECT(dev), "hw-storeeoi", storeeoi,
+                                 &error_abort);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
         spapr->xive = SPAPR_XIVE(dev);
-- 
2.25.4

[PATCH 5/8] spapr/xive: Enforce the 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 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 2c42ae92d287..c061230ea802 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 90f4509e6959..3eea4cb1c49f 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -317,6 +317,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;
 }
 
-- 
2.25.4

[PATCH 6/8] spapr/xive: Activate StoreEOI by default

[Cédric Le Goater]

Now that we check the XIVE characteristics of the sources at the KVM
level, we can configure the sources to use StoreEOI by default. This
feature will be activated for the emulated mode and possibly for KVM
also if compatible.

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

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index d184d17e30e7..e0765c0de696 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -317,6 +317,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp)
      */
     object_property_set_int(OBJECT(xsrc), "nr-irqs", xive->nr_irqs,
                             &error_fatal);
+    object_property_set_int(OBJECT(xsrc), "flags", XIVE_SRC_STORE_EOI,
+                            &error_fatal);
     object_property_set_link(OBJECT(xsrc), "xive", OBJECT(xive), &error_abort);
     if (!qdev_realize(DEVICE(xsrc), NULL, errp)) {
         return;
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 7/8] spapr/xive: Use the xics flag to check for XIVE-only IRQ backends

[Cédric Le Goater]

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

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 80cf1c3d6bb2..d036c8fef519 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -172,7 +172,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
          * To cover both and not confuse the OS, add an early failure in
          * QEMU.
          */
-        if (spapr->irq == &spapr_irq_xive) {
+        if (!spapr->irq->xics) {
             error_setg(errp, "XIVE-only machines require a POWER9 CPU");
             return -1;
         }
-- 
2.25.4

[PATCH 8/8] spapr/xive: Introduce a XIVE StoreEOI IRQ backend

[Cédric Le Goater]

As it is still useful to run a P9 compat guest with StoreEOI enabled,
introduce a new IRQ backend to allow that. May be we should add a
migration blocker.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/spapr_irq.h |  1 +
 hw/ppc/spapr.c             |  6 +++++-
 hw/ppc/spapr_irq.c         | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index ca8cb4421374..548895a89cca 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -90,6 +90,7 @@ typedef struct SpaprIrq {
 extern SpaprIrq spapr_irq_xics;
 extern SpaprIrq spapr_irq_xics_legacy;
 extern SpaprIrq spapr_irq_xive;
+extern SpaprIrq spapr_irq_xive_storeeoi;
 extern SpaprIrq spapr_irq_dual;
 
 void spapr_irq_init(SpaprMachineState *spapr, Error **errp);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a5bb0736e237..23f26d50f598 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3248,6 +3248,8 @@ static char *spapr_get_ic_mode(Object *obj, Error **errp)
         return g_strdup("xics");
     } else if (spapr->irq == &spapr_irq_xive) {
         return g_strdup("xive");
+    } else if (spapr->irq == &spapr_irq_xive_storeeoi) {
+        return g_strdup("xive-storeeoi");
     } else if (spapr->irq == &spapr_irq_dual) {
         return g_strdup("dual");
     }
@@ -3268,6 +3270,8 @@ static void spapr_set_ic_mode(Object *obj, const char *value, Error **errp)
         spapr->irq = &spapr_irq_xics;
     } else if (strcmp(value, "xive") == 0) {
         spapr->irq = &spapr_irq_xive;
+    } else if (strcmp(value, "xive-storeeoi") == 0) {
+        spapr->irq = &spapr_irq_xive_storeeoi;
     } else if (strcmp(value, "dual") == 0) {
         spapr->irq = &spapr_irq_dual;
     } else {
@@ -3350,7 +3354,7 @@ static void spapr_instance_init(Object *obj)
     object_property_add_str(obj, "ic-mode", spapr_get_ic_mode,
                             spapr_set_ic_mode);
     object_property_set_description(obj, "ic-mode",
-                 "Specifies the interrupt controller mode (xics, xive, dual)");
+                 "Specifies the interrupt controller mode (xics, xive, xive-storeeoi, dual)");
 
     object_property_add_str(obj, "host-model",
         spapr_get_host_model, spapr_set_host_model);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index d036c8fef519..c2e83fd0b34d 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -119,6 +119,15 @@ SpaprIrq spapr_irq_xive = {
     .xive        = true,
 };
 
+/*
+ * XIVE IRQ backend + StoreEOI activated
+ */
+
+SpaprIrq spapr_irq_xive_storeeoi = {
+    .xics        = false,
+    .xive        = true,
+};
+
 /*
  * Dual XIVE and XICS IRQ backend.
  *
@@ -213,6 +222,11 @@ static bool spapr_irq_xive_hw_storeeoi(SpaprMachineState *spapr)
     }
 
     /* StoreEOI on P9 compat is unsafe */
+    if (spapr->irq == &spapr_irq_xive_storeeoi) {
+        warn_report("HW Store EOI on a POWER9 CPU is unsafe.");
+        return true;
+    }
+
     return false;
 }
 
-- 
2.25.4

Re: [PATCH 1/8] spapr/xive: Add a 'hv-prio' property to represent the KVM escalation priority

[David Gibson]

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

On Wed, Aug 19, 2020 at 03:08:36PM +0200, Cédric Le Goater wrote:
> On POWER9, the KVM XIVE device uses priority 7 for the escalation
> interrupts. On POWER10, the host can use a reduced set of priorities
> and KVM will configure the escalation priority to a lower number. In
> any case, the guest is allowed to use priorities in a single range :
> 
>     [ 0 .. (maxprio - 1) ].
> 
> Introduce a 'hv-prio' property to represent the escalation priority
> number and use it to compute the "ibm,plat-res-int-priorities"
> property defining the priority ranges reserved by the hypervisor.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-5.2.

> ---
>  include/hw/ppc/spapr_xive.h |  2 ++
>  hw/intc/spapr_xive.c        | 33 ++++++++++++++-------------------
>  2 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 0ffbe0be0280..1dddcbcb9cdd 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -49,6 +49,8 @@ typedef struct SpaprXive {
>      void          *tm_mmap;
>      MemoryRegion  tm_mmio_kvm;
>      VMChangeStateEntry *change;
> +
> +    uint8_t       hv_prio;
>  } SpaprXive;
>  
>  typedef struct SpaprXiveClass {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 4bd0d606ba17..1fa09f287ac0 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -595,6 +595,7 @@ static Property spapr_xive_properties[] = {
>      DEFINE_PROP_UINT32("nr-ends", SpaprXive, nr_ends, 0),
>      DEFINE_PROP_UINT64("vc-base", SpaprXive, vc_base, SPAPR_XIVE_VC_BASE),
>      DEFINE_PROP_UINT64("tm-base", SpaprXive, tm_base, SPAPR_XIVE_TM_BASE),
> +    DEFINE_PROP_UINT8("hv-prio", SpaprXive, hv_prio, 7),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -692,12 +693,13 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
>          cpu_to_be32(16), /* 64K */
>      };
>      /*
> -     * The following array is in sync with the reserved priorities
> -     * defined by the 'spapr_xive_priority_is_reserved' routine.
> +     * QEMU/KVM only needs to define a single range to reserve the
> +     * escalation priority. A priority bitmask would have been more
> +     * appropriate.
>       */
>      uint32_t plat_res_int_priorities[] = {
> -        cpu_to_be32(7),    /* start */
> -        cpu_to_be32(0xf8), /* count */
> +        cpu_to_be32(xive->hv_prio),    /* start */
> +        cpu_to_be32(0xff - xive->hv_prio), /* count */
>      };
>  
>      /* Thread Interrupt Management Area : User (ring 3) and OS (ring 2) */
> @@ -844,19 +846,12 @@ type_init(spapr_xive_register_types)
>   */
>  
>  /*
> - * Linux hosts under OPAL reserve priority 7 for their own escalation
> - * interrupts (DD2.X POWER9). So we only allow the guest to use
> - * priorities [0..6].
> + * On POWER9, the KVM XIVE device uses priority 7 for the escalation
> + * interrupts. So we only allow the guest to use priorities [0..6].
>   */
> -static bool spapr_xive_priority_is_reserved(uint8_t priority)
> +static bool spapr_xive_priority_is_reserved(SpaprXive *xive, uint8_t priority)
>  {
> -    switch (priority) {
> -    case 0 ... 6:
> -        return false;
> -    case 7: /* OPAL escalation queue */
> -    default:
> -        return true;
> -    }
> +    return priority >= xive->hv_prio;
>  }
>  
>  /*
> @@ -1053,7 +1048,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu,
>          new_eas.w = eas.w & cpu_to_be64(~EAS_MASKED);
>      }
>  
> -    if (spapr_xive_priority_is_reserved(priority)) {
> +    if (spapr_xive_priority_is_reserved(xive, priority)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
>                        " is reserved\n", priority);
>          return H_P4;
> @@ -1212,7 +1207,7 @@ static target_ulong h_int_get_queue_info(PowerPCCPU *cpu,
>       * This is not needed when running the emulation under QEMU
>       */
>  
> -    if (spapr_xive_priority_is_reserved(priority)) {
> +    if (spapr_xive_priority_is_reserved(xive, priority)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
>                        " is reserved\n", priority);
>          return H_P3;
> @@ -1299,7 +1294,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
>       * This is not needed when running the emulation under QEMU
>       */
>  
> -    if (spapr_xive_priority_is_reserved(priority)) {
> +    if (spapr_xive_priority_is_reserved(xive, priority)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
>                        " is reserved\n", priority);
>          return H_P3;
> @@ -1466,7 +1461,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
>       * This is not needed when running the emulation under QEMU
>       */
>  
> -    if (spapr_xive_priority_is_reserved(priority)) {
> +    if (spapr_xive_priority_is_reserved(xive, priority)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "XIVE: priority " TARGET_FMT_ld
>                        " is reserved\n", priority);
>          return H_P3;

-- 
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 2/8] linux-headers: Update for KVM_DEV_XIVE_GRP_SOURCE_INFO

[David Gibson]

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

On Wed, Aug 19, 2020 at 03:08:37PM +0200, Cédric Le Goater wrote:
> To be sent with the linux-headers update when support is merged.

Ah, so this isn't ready to go just yet.

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  linux-headers/asm-powerpc/kvm.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
> index 264e266a85bf..aeb8e8c4633b 100644
> --- a/linux-headers/asm-powerpc/kvm.h
> +++ b/linux-headers/asm-powerpc/kvm.h
> @@ -690,6 +690,7 @@ struct kvm_ppc_cpu_char {
>  #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG	3	/* 64-bit source identifier */
>  #define KVM_DEV_XIVE_GRP_EQ_CONFIG	4	/* 64-bit EQ identifier */
>  #define KVM_DEV_XIVE_GRP_SOURCE_SYNC	5       /* 64-bit source identifier */
> +#define KVM_DEV_XIVE_GRP_SOURCE_INFO	6       /* 64-bit source identifier */
>  
>  /* Layout of 64-bit XIVE source attribute values */
>  #define KVM_XIVE_LEVEL_SENSITIVE	(1ULL << 0)
> @@ -721,6 +722,13 @@ struct kvm_ppc_xive_eq {
>  	__u8  pad[40];
>  };
>  
> +#define KVM_XIVE_SOURCE_FLAG_STORE_EOI	0x00000001
> +
> +/* Layout of source characteristics (8 bytes) */
> +struct kvm_ppc_xive_src {
> +	__u64 flags;
> +};
> +
>  #define KVM_XIVE_EQ_ALWAYS_NOTIFY	0x00000001
>  
>  #define KVM_XIVE_TIMA_PAGE_OFFSET	0

-- 
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 3/8] spapr/xive: Query the characteristics of a source in KVM

[David Gibson]

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

On Wed, Aug 19, 2020 at 03:08:38PM +0200, Cédric Le Goater wrote:
> When running a guest with a kernel IRQ chip enabled, the XIVE
> characteristics of the interrupts are advertised to the guest in the
> H_INT_GET_SOURCE_INFO hcall. These characteristics depend on the
> underlying HW interrupts but today, QEMU simply advertises its own
> without checking what the host supports. It is not a problem for the
> moment, but POWER10 will (re)add support for StoreEOI and we need a
> way to in sync with the host.
> 
> The KVM_DEV_XIVE_GRP_SOURCE_INFO command lets QEMU query the XIVE
> characteristics of the underlying HW interrupts and override any
> previous setting done by QEMU. This allows the fallback mode, when the
> XIVE device is emulated by QEMU, to use its own custom settings on the
> sources but makes sure that we don't let a guest run with features
> incompatible with KVM.
> 
> It only applies to the StoreEOI feature for the moment.

Urgh.  This means that the source characteristics can change across a
migration, that's kind of a problem.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/spapr_xive.h |  2 ++
>  hw/intc/spapr_xive.c        | 20 ++++++++++++++++++++
>  hw/intc/spapr_xive_kvm.c    | 26 ++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 1dddcbcb9cdd..3f325723ea74 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -84,6 +84,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc);
>  void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
>  int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>                                    Error **errp);
> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
> +                                 Error **errp);
>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
>  uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
>                              uint64_t data, bool write);
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 1fa09f287ac0..943b9958a68b 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -932,6 +932,26 @@ static target_ulong h_int_get_source_info(PowerPCCPU *cpu,
>          args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
>      }
>  
> +    if (kvm_irqchip_in_kernel()) {
> +        Error *local_err = NULL;
> +        uint64_t flags = 0;
> +
> +        kvmppc_xive_get_source_info(xive, lisn, &flags, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return H_HARDWARE;
> +        }
> +
> +        /*
> +         * Override QEMU settings with KVM values
> +         */
> +        if (flags & XIVE_SRC_STORE_EOI) {
> +            args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
> +        } else {
> +            args[0] &= ~SPAPR_XIVE_SRC_STORE_EOI;
> +        }
> +    }
> +
>      /*
>       * Force the use of the H_INT_ESB hcall in case of an LSI
>       * interrupt. This is necessary under KVM to re-trigger the
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index e8667ce5f621..90f4509e6959 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -217,6 +217,32 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>                               &kvm_src, true, errp);
>  }
>  
> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
> +                                 Error **errp)
> +{
> +    struct kvm_ppc_xive_src kvm_src = { 0 };
> +    int ret;
> +
> +    /*
> +     * Check that KVM supports the new attribute to query source
> +     * characteristics.
> +     */
> +    if (!kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, 0)) {
> +        return 0;
> +    }
> +
> +    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, lisn,
> +                            &kvm_src, false, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (kvm_src.flags & KVM_XIVE_SOURCE_FLAG_STORE_EOI) {
> +        *flags |= XIVE_SRC_STORE_EOI;
> +    }
> +    return 0;
> +}
> +
>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
>  {
>      kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_SYNC, lisn,

-- 
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 7/8] spapr/xive: Use the xics flag to check for XIVE-only IRQ backends

[David Gibson]

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

On Wed, Aug 19, 2020 at 03:08:42PM +0200, Cédric Le Goater wrote:

I can see why this is a good idea, but it really needs a rationale in
the comment for posterity.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/spapr_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 80cf1c3d6bb2..d036c8fef519 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -172,7 +172,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
>           * To cover both and not confuse the OS, add an early failure in
>           * QEMU.
>           */
> -        if (spapr->irq == &spapr_irq_xive) {
> +        if (!spapr->irq->xics) {
>              error_setg(errp, "XIVE-only machines require a POWER9 CPU");
>              return -1;
>          }

-- 
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 2/8] linux-headers: Update for KVM_DEV_XIVE_GRP_SOURCE_INFO

[Cédric Le Goater]

On 8/20/20 2:58 AM, David Gibson wrote:
> On Wed, Aug 19, 2020 at 03:08:37PM +0200, Cédric Le Goater wrote:
>> To be sent with the linux-headers update when support is merged.
> 
> Ah, so this isn't ready to go just yet.

No indeed. We should get the QEMU part correct first, and handle 
safely the source characteristics of the host.

C.  

 
> 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  linux-headers/asm-powerpc/kvm.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
>> index 264e266a85bf..aeb8e8c4633b 100644
>> --- a/linux-headers/asm-powerpc/kvm.h
>> +++ b/linux-headers/asm-powerpc/kvm.h
>> @@ -690,6 +690,7 @@ struct kvm_ppc_cpu_char {
>>  #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG	3	/* 64-bit source identifier */
>>  #define KVM_DEV_XIVE_GRP_EQ_CONFIG	4	/* 64-bit EQ identifier */
>>  #define KVM_DEV_XIVE_GRP_SOURCE_SYNC	5       /* 64-bit source identifier */
>> +#define KVM_DEV_XIVE_GRP_SOURCE_INFO	6       /* 64-bit source identifier */
>>  
>>  /* Layout of 64-bit XIVE source attribute values */
>>  #define KVM_XIVE_LEVEL_SENSITIVE	(1ULL << 0)
>> @@ -721,6 +722,13 @@ struct kvm_ppc_xive_eq {
>>  	__u8  pad[40];
>>  };
>>  
>> +#define KVM_XIVE_SOURCE_FLAG_STORE_EOI	0x00000001
>> +
>> +/* Layout of source characteristics (8 bytes) */
>> +struct kvm_ppc_xive_src {
>> +	__u64 flags;
>> +};
>> +
>>  #define KVM_XIVE_EQ_ALWAYS_NOTIFY	0x00000001
>>  
>>  #define KVM_XIVE_TIMA_PAGE_OFFSET	0
> 

Re: [PATCH 3/8] spapr/xive: Query the characteristics of a source in KVM

[Cédric Le Goater]

On 8/20/20 3:33 AM, David Gibson wrote:
> On Wed, Aug 19, 2020 at 03:08:38PM +0200, Cédric Le Goater wrote:
>> When running a guest with a kernel IRQ chip enabled, the XIVE
>> characteristics of the interrupts are advertised to the guest in the
>> H_INT_GET_SOURCE_INFO hcall. These characteristics depend on the
>> underlying HW interrupts but today, QEMU simply advertises its own
>> without checking what the host supports. It is not a problem for the
>> moment, but POWER10 will (re)add support for StoreEOI and we need a
>> way to in sync with the host.
>>
>> The KVM_DEV_XIVE_GRP_SOURCE_INFO command lets QEMU query the XIVE
>> characteristics of the underlying HW interrupts and override any
>> previous setting done by QEMU. This allows the fallback mode, when the
>> XIVE device is emulated by QEMU, to use its own custom settings on the
>> sources but makes sure that we don't let a guest run with features
>> incompatible with KVM.
>>
>> It only applies to the StoreEOI feature for the moment.
> 
> Urgh.  This means that the source characteristics can change across a
> migration, that's kind of a problem.

yes.

The official plan is : 

  * P9 compat : no StoreEOI 
  * P10 : StoreEOI 

and the obvious solution I started with was to change the global source 
flags at CAS time. 

But, we have experimental P9 firmwares in which StoreEOI is activated
and migration of a P9 compat guest from a P10 host to another P10 host 
should work. I am looking for a smarter solution.

Forbidding StoreEOI on P9 compat guests is ok, since this is already 
the default, but an option to change it would be nice to have. 

As for P10, all HW sources and IC sources now have StoreEOI but we
might want to disable it for some reason. This is already in place 
at the firmware and PowerNV level, QEMU is the next layer to address.

Thanks,

C. 




> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/spapr_xive.h |  2 ++
>>  hw/intc/spapr_xive.c        | 20 ++++++++++++++++++++
>>  hw/intc/spapr_xive_kvm.c    | 26 ++++++++++++++++++++++++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 1dddcbcb9cdd..3f325723ea74 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -84,6 +84,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc);
>>  void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
>>  int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>>                                    Error **errp);
>> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
>> +                                 Error **errp);
>>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
>>  uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
>>                              uint64_t data, bool write);
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 1fa09f287ac0..943b9958a68b 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -932,6 +932,26 @@ static target_ulong h_int_get_source_info(PowerPCCPU *cpu,
>>          args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
>>      }
>>  
>> +    if (kvm_irqchip_in_kernel()) {
>> +        Error *local_err = NULL;
>> +        uint64_t flags = 0;
>> +
>> +        kvmppc_xive_get_source_info(xive, lisn, &flags, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            return H_HARDWARE;
>> +        }
>> +
>> +        /*
>> +         * Override QEMU settings with KVM values
>> +         */
>> +        if (flags & XIVE_SRC_STORE_EOI) {
>> +            args[0] |= SPAPR_XIVE_SRC_STORE_EOI;
>> +        } else {
>> +            args[0] &= ~SPAPR_XIVE_SRC_STORE_EOI;
>> +        }
>> +    }
>> +
>>      /*
>>       * Force the use of the H_INT_ESB hcall in case of an LSI
>>       * interrupt. This is necessary under KVM to re-trigger the
>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
>> index e8667ce5f621..90f4509e6959 100644
>> --- a/hw/intc/spapr_xive_kvm.c
>> +++ b/hw/intc/spapr_xive_kvm.c
>> @@ -217,6 +217,32 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>>                               &kvm_src, true, errp);
>>  }
>>  
>> +int kvmppc_xive_get_source_info(SpaprXive *xive, uint32_t lisn, uint64_t *flags,
>> +                                 Error **errp)
>> +{
>> +    struct kvm_ppc_xive_src kvm_src = { 0 };
>> +    int ret;
>> +
>> +    /*
>> +     * Check that KVM supports the new attribute to query source
>> +     * characteristics.
>> +     */
>> +    if (!kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, 0)) {
>> +        return 0;
>> +    }
>> +
>> +    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_INFO, lisn,
>> +                            &kvm_src, false, errp);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (kvm_src.flags & KVM_XIVE_SOURCE_FLAG_STORE_EOI) {
>> +        *flags |= XIVE_SRC_STORE_EOI;
>> +    }
>> +    return 0;
>> +}
>> +
>>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
>>  {
>>      kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_SYNC, lisn,
> 

Re: [PATCH 7/8] spapr/xive: Use the xics flag to check for XIVE-only IRQ backends

[Cédric Le Goater]

On 8/20/20 3:36 AM, David Gibson wrote:
> On Wed, Aug 19, 2020 at 03:08:42PM +0200, Cédric Le Goater wrote:
> 
> I can see why this is a good idea, but it really needs a rationale in
> the comment for posterity.

yes. I can send this one independently.

Thanks,

C.


> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/spapr_irq.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 80cf1c3d6bb2..d036c8fef519 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -172,7 +172,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
>>           * To cover both and not confuse the OS, add an early failure in
>>           * QEMU.
>>           */
>> -        if (spapr->irq == &spapr_irq_xive) {
>> +        if (!spapr->irq->xics) {
>>              error_setg(errp, "XIVE-only machines require a POWER9 CPU");
>>              return -1;
>>          }
>