[PATCH v3 for-5.2 0/3] spapr: Cleanups for XIVE

[PATCH v3 for-5.2 0/3] spapr: Cleanups for XIVE

[Greg Kurz]

Recent cleanup patch "spapr: Simplify error handling in spapr_phb_realize"
had to be dropped from ppc-for-5.2 because it would cause QEMU to crash
at init time on some POWER9 setups (eg. Boston systems), as reported by
Daniel.

The crash was happening because the kvmppc_xive_source_reset_one() function
would get called at some point (eg. initializing the LSI table of PHB0) and
fail (because XIVE KVM hasn't been created yet) without calling error_setg(),
which the caller doesn't expect when the above patch is applied.

The issue isn't really about a missing call to error_setg() but why do
we end up trying to claim an IRQ number in a XIVE KVM device that doesn't
exist ? The root cause for this is that we guard calls to the XIVE KVM
code with kvm_irqchip_in_kernel(), which might return true when the XICS
KVM device is active, even though the XIVE one is not. This series
upgrade the guarding code to also check if the device is actually open.

A similar cleanup could be performed on XICS.

v3: - rework the ESB MMIO of the XIVE source in a preliminary patch.
      This gets rid of an anoying user of kvm_irqchip_in_kernel()
    - drop "spapr: Simplify error handling in spapr_phb_realize". Will
      be posted later with other "error handling" cleanups in a separate
      series

v2: - patch 1 and 2 already applied but not yet visible on github
    - new approach with abstract methods in the base XIVE classes

---

Greg Kurz (3):
      ppc/xive: Rework setup of XiveSource::esb_mmio
      ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers
      spapr/xive: Convert KVM device fd checks to assert()


 hw/intc/spapr_xive.c     |   45 +++++++++++++++++++++++++++++++--------------
 hw/intc/spapr_xive_kvm.c |   39 +++++++++------------------------------
 hw/intc/xive.c           |   36 +++++++++++++++++++++++++-----------
 include/hw/ppc/xive.h    |    7 +++++++
 4 files changed, 72 insertions(+), 55 deletions(-)

--
Greg

[PATCH v3 for-5.2 1/3] ppc/xive: Rework setup of XiveSource::esb_mmio

[Greg Kurz]

Depending on whether XIVE is emultated or backed with a KVM XIVE device,
the ESB MMIOs of a XIVE source point to an I/O memory region or a mapped
memory region.

This is currently handled by checking kvm_irqchip_in_kernel() returns
false in xive_source_realize(). This is a bit awkward as we usually
need to do extra things when we're using the in-kernel backend, not
less. But most important, we can do better: turn the existing "xive.esb"
memory region into a plain container, introduce an "xive.esb-emulated"
I/O subregion and rename the existing "xive.esb" subregion in the KVM
code to "xive.esb-kvm". Since "xive.esb-kvm" is added with overlap
and a higher priority, it prevails over "xive.esb-emulated" (ie.
a guest using KVM XIVE will interact with "xive.esb-kvm" instead of
the default "xive.esb-emulated" region.

While here, consolidate the computation of the MMIO region size in
a common helper.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |    4 ++--
 hw/intc/xive.c           |   11 ++++++-----
 include/hw/ppc/xive.h    |    6 ++++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 893a1ee77e70..6130882be678 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -742,7 +742,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     SpaprXive *xive = SPAPR_XIVE(intc);
     XiveSource *xsrc = &xive->source;
     Error *local_err = NULL;
-    size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
+    size_t esb_len = xive_source_esb_len(xsrc);
     size_t tima_len = 4ull << TM_SHIFT;
     CPUState *cs;
     int fd;
@@ -788,7 +788,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     }
 
     memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
-                                      "xive.esb", esb_len, xsrc->esb_mmap);
+                                      "xive.esb-kvm", esb_len, xsrc->esb_mmap);
     memory_region_add_subregion_overlap(&xsrc->esb_mmio, 0,
                                         &xsrc->esb_mmio_kvm, 1);
 
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 9b55e0356c62..561d746cd1da 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -1128,6 +1128,7 @@ static void xive_source_reset(void *dev)
 static void xive_source_realize(DeviceState *dev, Error **errp)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
+    size_t esb_len = xive_source_esb_len(xsrc);
 
     assert(xsrc->xive);
 
@@ -1147,11 +1148,11 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
     xsrc->status = g_malloc0(xsrc->nr_irqs);
     xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
 
-    if (!kvm_irqchip_in_kernel()) {
-        memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
-                              &xive_source_esb_ops, xsrc, "xive.esb",
-                              (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
-    }
+    memory_region_init(&xsrc->esb_mmio, OBJECT(xsrc), "xive.esb", esb_len);
+    memory_region_init_io(&xsrc->esb_mmio_emulated, OBJECT(xsrc),
+                          &xive_source_esb_ops, xsrc, "xive.esb-emulated",
+                          esb_len);
+    memory_region_add_subregion(&xsrc->esb_mmio, 0, &xsrc->esb_mmio_emulated);
 
     qemu_register_reset(xive_source_reset, dev);
 }
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 705cf48176fc..82a61eaca74f 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -191,6 +191,7 @@ typedef struct XiveSource {
     uint64_t        esb_flags;
     uint32_t        esb_shift;
     MemoryRegion    esb_mmio;
+    MemoryRegion    esb_mmio_emulated;
 
     /* KVM support */
     void            *esb_mmap;
@@ -215,6 +216,11 @@ static inline bool xive_source_esb_has_2page(XiveSource *xsrc)
         xsrc->esb_shift == XIVE_ESB_4K_2PAGE;
 }
 
+static inline size_t xive_source_esb_len(XiveSource *xsrc)
+{
+    return (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
+}
+
 /* The trigger page is always the first/even page */
 static inline hwaddr xive_source_esb_page(XiveSource *xsrc, uint32_t srcno)
 {

[PATCH v3 for-5.2 2/3] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[Greg Kurz]

Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This
ensures that QEMU won't try to use the device if KVM is disabled or if
an in-kernel irqchip isn't required.

When using ic-mode=dual with the pseries machine, we have two possible
interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper
will return true as soon as any of the KVM device is created. It might
lure QEMU to think that the other one is also around, while it is not.
This is exactly what happens with ic-mode=dual at machine init when
claiming IRQ numbers, which must be done on all possible IRQ backends,
eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device
is active but we end up calling kvmppc_xive_source_reset_one() anyway,
which fails. This doesn't cause any trouble because of another bug :
kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't
see the failure.

Most of the other kvmppc_xive_* functions have similar xive->fd
checks to filter out the case when KVM XIVE isn't active. It
might look safer to have idempotent functions but it doesn't
really help to understand what's going on when debugging.

Since we already have all the kvm_irqchip_in_kernel() in place,
also have the callers to check xive->fd as well before calling
KVM XIVE specific code. This is straight-forward for the spapr
specific XIVE code. Some more care is needed for the platform
agnostic XIVE code since it cannot access xive->fd directly.
Introduce new in_kernel() methods in some base XIVE classes
for this purpose and implement them only in spapr.

In all cases, we still need to call kvm_irqchip_in_kernel() so that
compilers can optimize the kvmppc_xive_* calls away when CONFIG_KVM
isn't defined, thus avoiding the need for stubs.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v3: Method for XiveNotifierClass no longer needed

v2: Introduce in_kernel() abstract methods in the base XIVE classes
---
 hw/intc/spapr_xive.c  |   45 +++++++++++++++++++++++++++++++--------------
 hw/intc/xive.c        |   25 +++++++++++++++++++------
 include/hw/ppc/xive.h |    1 +
 3 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 89c8cd96670b..3c84f64dc464 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end,
     xive_end_queue_pic_print_info(end, 6, mon);
 }
 
+/*
+ * kvm_irqchip_in_kernel() will cause the compiler to turn this
+ * info a nop if CONFIG_KVM isn't defined.
+ */
+#define spapr_xive_in_kernel(xive) \
+    (kvm_irqchip_in_kernel() && (xive)->fd != -1)
+
 void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
 {
     XiveSource *xsrc = &xive->source;
     int i;
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_synchronize_state(xive, &local_err);
@@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = {
 
 static int vmstate_spapr_xive_pre_save(void *opaque)
 {
-    if (kvm_irqchip_in_kernel()) {
-        return kvmppc_xive_pre_save(SPAPR_XIVE(opaque));
+    SpaprXive *xive = SPAPR_XIVE(opaque);
+
+    if (spapr_xive_in_kernel(xive)) {
+        return kvmppc_xive_pre_save(xive);
     }
 
     return 0;
@@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque)
  */
 static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id)
 {
-    if (kvm_irqchip_in_kernel()) {
-        return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id);
+    SpaprXive *xive = SPAPR_XIVE(intc);
+
+    if (spapr_xive_in_kernel(xive)) {
+        return kvmppc_xive_post_load(xive, version_id);
     }
 
     return 0;
@@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn,
         xive_source_irq_set_lsi(xsrc, lisn);
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         return kvmppc_xive_source_reset_one(xsrc, lisn, errp);
     }
 
@@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
 {
     SpaprXive *xive = SPAPR_XIVE(intc);
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         kvmppc_xive_source_set_irq(&xive->source, irq, val);
     } else {
         xive_source_set_irq(&xive->source, irq, val);
@@ -749,11 +760,16 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
 
     spapr_xive_mmio_set_enabled(xive, false);
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         kvmppc_xive_disconnect(intc);
     }
 }
 
+static bool spapr_xive_in_kernel_xptr(const XivePresenter *xptr)
+{
+    return spapr_xive_in_kernel(SPAPR_XIVE(xptr));
+}
+
 static void spapr_xive_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -788,6 +804,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
     sicc->post_load = spapr_xive_post_load;
 
     xpc->match_nvt  = spapr_xive_match_nvt;
+    xpc->in_kernel  = spapr_xive_in_kernel_xptr;
 }
 
 static const TypeInfo spapr_xive_info = {
@@ -1058,7 +1075,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu,
         new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn);
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
@@ -1379,7 +1396,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
      */
 
 out:
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
@@ -1480,7 +1497,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
         args[2] = 0;
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
@@ -1642,7 +1659,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
         return H_P3;
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
                                      flags & SPAPR_XIVE_ESB_STORE);
     } else {
@@ -1717,7 +1734,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
      * under KVM
      */
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_sync_source(xive, lisn, &local_err);
@@ -1761,7 +1778,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
 
     device_legacy_reset(DEVICE(xive));
 
-    if (kvm_irqchip_in_kernel()) {
+    if (spapr_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_reset(xive, &local_err);
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 561d746cd1da..a453e8f4dcbe 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -592,6 +592,17 @@ static const char * const xive_tctx_ring_names[] = {
     "USER", "OS", "POOL", "PHYS",
 };
 
+/*
+ * kvm_irqchip_in_kernel() will cause the compiler to turn this
+ * info a nop if CONFIG_KVM isn't defined.
+ */
+#define xive_in_kernel(xptr)                                            \
+    (kvm_irqchip_in_kernel() &&                                         \
+     ({                                                                 \
+         XivePresenterClass *xpc = XIVE_PRESENTER_GET_CLASS(xptr);      \
+         xpc->in_kernel ? xpc->in_kernel(xptr) : false;                 \
+     }))
+
 void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
 {
     int cpu_index;
@@ -606,7 +617,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
 
     cpu_index = tctx->cs ? tctx->cs->cpu_index : -1;
 
-    if (kvm_irqchip_in_kernel()) {
+    if (xive_in_kernel(tctx->xptr)) {
         Error *local_err = NULL;
 
         kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
@@ -671,7 +682,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
     }
 
     /* Connect the presenter to the VCPU (required for CPU hotplug) */
-    if (kvm_irqchip_in_kernel()) {
+    if (xive_in_kernel(tctx->xptr)) {
         kvmppc_xive_cpu_connect(tctx, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -682,10 +693,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
 
 static int vmstate_xive_tctx_pre_save(void *opaque)
 {
+    XiveTCTX *tctx = XIVE_TCTX(opaque);
     Error *local_err = NULL;
 
-    if (kvm_irqchip_in_kernel()) {
-        kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err);
+    if (xive_in_kernel(tctx->xptr)) {
+        kvmppc_xive_cpu_get_state(tctx, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -1;
@@ -697,14 +709,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque)
 
 static int vmstate_xive_tctx_post_load(void *opaque, int version_id)
 {
+    XiveTCTX *tctx = XIVE_TCTX(opaque);
     Error *local_err = NULL;
 
-    if (kvm_irqchip_in_kernel()) {
+    if (xive_in_kernel(tctx->xptr)) {
         /*
          * Required for hotplugged CPU, for which the state comes
          * after all states of the machine.
          */
-        kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err);
+        kvmppc_xive_cpu_set_state(tctx, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -1;
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 82a61eaca74f..2f3c5af810bb 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -402,6 +402,7 @@ typedef struct XivePresenterClass {
                      uint8_t nvt_blk, uint32_t nvt_idx,
                      bool cam_ignore, uint8_t priority,
                      uint32_t logic_serv, XiveTCTXMatch *match);
+    bool (*in_kernel)(const XivePresenter *xptr);
 } XivePresenterClass;
 
 int xive_presenter_tctx_match(XivePresenter *xptr, XiveTCTX *tctx,

[PATCH v3 for-5.2 3/3] spapr/xive: Convert KVM device fd checks to assert()

[Greg Kurz]

All callers guard these functions with an xive_in_kernel() helper. Make
it clear that they are only to be called when the KVM XIVE device exists.

Note that the check on xive is dropped in kvmppc_xive_disconnect(). It
really cannot be NULL since it comes from set_active_intc() which only
passes pointers to allocated objects.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
v2: Take the helper name change into account in the changelog
---
 hw/intc/spapr_xive_kvm.c |   35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 6130882be678..82a6f99f022d 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -79,10 +79,7 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
     uint64_t state[2];
     int ret;
 
-    /* The KVM XIVE device is not in use yet */
-    if (xive->fd == -1) {
-        return;
-    }
+    assert(xive->fd != -1);
 
     /* word0 and word1 of the OS ring. */
     state[0] = *((uint64_t *) &tctx->regs[TM_QW1_OS]);
@@ -101,10 +98,7 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
     uint64_t state[2] = { 0 };
     int ret;
 
-    /* The KVM XIVE device is not in use */
-    if (xive->fd == -1) {
-        return;
-    }
+    assert(xive->fd != -1);
 
     ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
     if (ret != 0) {
@@ -156,10 +150,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
     unsigned long vcpu_id;
     int ret;
 
-    /* The KVM XIVE device is not in use */
-    if (xive->fd == -1) {
-        return;
-    }
+    assert(xive->fd != -1);
 
     /* Check if CPU was hot unplugged and replugged. */
     if (kvm_cpu_is_enabled(tctx->cs)) {
@@ -245,10 +236,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
     SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
     uint64_t state = 0;
 
-    /* The KVM XIVE device is not in use */
-    if (xive->fd == -1) {
-        return -ENODEV;
-    }
+    assert(xive->fd != -1);
 
     if (xive_source_irq_is_lsi(xsrc, srcno)) {
         state |= KVM_XIVE_LEVEL_SENSITIVE;
@@ -592,10 +580,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, int running,
 
 void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
 {
-    /* The KVM XIVE device is not in use */
-    if (xive->fd == -1) {
-        return;
-    }
+    assert(xive->fd != -1);
 
     /*
      * When the VM is stopped, the sources are masked and the previous
@@ -622,10 +607,7 @@ int kvmppc_xive_pre_save(SpaprXive *xive)
 {
     Error *local_err = NULL;
 
-    /* The KVM XIVE device is not in use */
-    if (xive->fd == -1) {
-        return 0;
-    }
+    assert(xive->fd != -1);
 
     /* EAT: there is no extra state to query from KVM */
 
@@ -845,10 +827,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
     XiveSource *xsrc;
     size_t esb_len;
 
-    /* The KVM XIVE device is not in use */
-    if (!xive || xive->fd == -1) {
-        return;
-    }
+    assert(xive->fd != -1);
 
     /* Clear the KVM mapping */
     xsrc = &xive->source;

Re: [PATCH v3 for-5.2 1/3] ppc/xive: Rework setup of XiveSource::esb_mmio

[Cédric Le Goater]

On 8/7/20 1:32 PM, Greg Kurz wrote:
> Depending on whether XIVE is emultated or backed with a KVM XIVE device,
> the ESB MMIOs of a XIVE source point to an I/O memory region or a mapped
> memory region.
> 
> This is currently handled by checking kvm_irqchip_in_kernel() returns
> false in xive_source_realize(). This is a bit awkward as we usually
> need to do extra things when we're using the in-kernel backend, not
> less. But most important, we can do better: turn the existing "xive.esb"
> memory region into a plain container, introduce an "xive.esb-emulated"
> I/O subregion and rename the existing "xive.esb" subregion in the KVM
> code to "xive.esb-kvm". Since "xive.esb-kvm" is added with overlap
> and a higher priority, it prevails over "xive.esb-emulated" (ie.
> a guest using KVM XIVE will interact with "xive.esb-kvm" instead of
> the default "xive.esb-emulated" region.
> 
> While here, consolidate the computation of the MMIO region size in
> a common helper.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>

This is much better

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/spapr_xive_kvm.c |    4 ++--
>  hw/intc/xive.c           |   11 ++++++-----
>  include/hw/ppc/xive.h    |    6 ++++++
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 893a1ee77e70..6130882be678 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -742,7 +742,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      SpaprXive *xive = SPAPR_XIVE(intc);
>      XiveSource *xsrc = &xive->source;
>      Error *local_err = NULL;
> -    size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +    size_t esb_len = xive_source_esb_len(xsrc);
>      size_t tima_len = 4ull << TM_SHIFT;
>      CPUState *cs;
>      int fd;
> @@ -788,7 +788,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      }
>  
>      memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
> -                                      "xive.esb", esb_len, xsrc->esb_mmap);
> +                                      "xive.esb-kvm", esb_len, xsrc->esb_mmap);
>      memory_region_add_subregion_overlap(&xsrc->esb_mmio, 0,
>                                          &xsrc->esb_mmio_kvm, 1);
>  
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 9b55e0356c62..561d746cd1da 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1128,6 +1128,7 @@ static void xive_source_reset(void *dev)
>  static void xive_source_realize(DeviceState *dev, Error **errp)
>  {
>      XiveSource *xsrc = XIVE_SOURCE(dev);
> +    size_t esb_len = xive_source_esb_len(xsrc);
>  
>      assert(xsrc->xive);
>  
> @@ -1147,11 +1148,11 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
>      xsrc->status = g_malloc0(xsrc->nr_irqs);
>      xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
>  
> -    if (!kvm_irqchip_in_kernel()) {
> -        memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> -                              &xive_source_esb_ops, xsrc, "xive.esb",
> -                              (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
> -    }
> +    memory_region_init(&xsrc->esb_mmio, OBJECT(xsrc), "xive.esb", esb_len);
> +    memory_region_init_io(&xsrc->esb_mmio_emulated, OBJECT(xsrc),
> +                          &xive_source_esb_ops, xsrc, "xive.esb-emulated",
> +                          esb_len);
> +    memory_region_add_subregion(&xsrc->esb_mmio, 0, &xsrc->esb_mmio_emulated);
>  
>      qemu_register_reset(xive_source_reset, dev);
>  }
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 705cf48176fc..82a61eaca74f 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -191,6 +191,7 @@ typedef struct XiveSource {
>      uint64_t        esb_flags;
>      uint32_t        esb_shift;
>      MemoryRegion    esb_mmio;
> +    MemoryRegion    esb_mmio_emulated;
>  
>      /* KVM support */
>      void            *esb_mmap;
> @@ -215,6 +216,11 @@ static inline bool xive_source_esb_has_2page(XiveSource *xsrc)
>          xsrc->esb_shift == XIVE_ESB_4K_2PAGE;
>  }
>  
> +static inline size_t xive_source_esb_len(XiveSource *xsrc)
> +{
> +    return (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +}
> +
>  /* The trigger page is always the first/even page */
>  static inline hwaddr xive_source_esb_page(XiveSource *xsrc, uint32_t srcno)
>  {
> 
> 

Re: [PATCH v3 for-5.2 3/3] spapr/xive: Convert KVM device fd checks to assert()

[Cédric Le Goater]

On 8/7/20 1:32 PM, Greg Kurz wrote:
> All callers guard these functions with an xive_in_kernel() helper. Make
> it clear that they are only to be called when the KVM XIVE device exists.
> 
> Note that the check on xive is dropped in kvmppc_xive_disconnect(). It
> really cannot be NULL since it comes from set_active_intc() which only
> passes pointers to allocated objects.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
> v2: Take the helper name change into account in the changelog
> ---
>  hw/intc/spapr_xive_kvm.c |   35 +++++++----------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 6130882be678..82a6f99f022d 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -79,10 +79,7 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
>      uint64_t state[2];
>      int ret;
>  
> -    /* The KVM XIVE device is not in use yet */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* word0 and word1 of the OS ring. */
>      state[0] = *((uint64_t *) &tctx->regs[TM_QW1_OS]);
> @@ -101,10 +98,7 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
>      uint64_t state[2] = { 0 };
>      int ret;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
>      if (ret != 0) {
> @@ -156,10 +150,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>      unsigned long vcpu_id;
>      int ret;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* Check if CPU was hot unplugged and replugged. */
>      if (kvm_cpu_is_enabled(tctx->cs)) {
> @@ -245,10 +236,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
>      SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>      uint64_t state = 0;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return -ENODEV;
> -    }
> +    assert(xive->fd != -1);
>  
>      if (xive_source_irq_is_lsi(xsrc, srcno)) {
>          state |= KVM_XIVE_LEVEL_SENSITIVE;
> @@ -592,10 +580,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, int running,
>  
>  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
>  {
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /*
>       * When the VM is stopped, the sources are masked and the previous
> @@ -622,10 +607,7 @@ int kvmppc_xive_pre_save(SpaprXive *xive)
>  {
>      Error *local_err = NULL;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return 0;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* EAT: there is no extra state to query from KVM */
>  
> @@ -845,10 +827,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
>      XiveSource *xsrc;
>      size_t esb_len;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (!xive || xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* Clear the KVM mapping */
>      xsrc = &xive->source;
> 
> 

Re: [PATCH v3 for-5.2 2/3] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[Cédric Le Goater]

On 8/7/20 1:32 PM, Greg Kurz wrote:
> Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This
> ensures that QEMU won't try to use the device if KVM is disabled or if
> an in-kernel irqchip isn't required.
> 
> When using ic-mode=dual with the pseries machine, we have two possible
> interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper
> will return true as soon as any of the KVM device is created. It might
> lure QEMU to think that the other one is also around, while it is not.
> This is exactly what happens with ic-mode=dual at machine init when
> claiming IRQ numbers, which must be done on all possible IRQ backends,
> eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device
> is active but we end up calling kvmppc_xive_source_reset_one() anyway,
> which fails. This doesn't cause any trouble because of another bug :
> kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't
> see the failure.
> 
> Most of the other kvmppc_xive_* functions have similar xive->fd
> checks to filter out the case when KVM XIVE isn't active. It
> might look safer to have idempotent functions but it doesn't
> really help to understand what's going on when debugging.
> 
> Since we already have all the kvm_irqchip_in_kernel() in place,
> also have the callers to check xive->fd as well before calling
> KVM XIVE specific code. This is straight-forward for the spapr
> specific XIVE code. Some more care is needed for the platform
> agnostic XIVE code since it cannot access xive->fd directly.
> Introduce new in_kernel() methods in some base XIVE classes
> for this purpose and implement them only in spapr.
> 
> In all cases, we still need to call kvm_irqchip_in_kernel() so that
> compilers can optimize the kvmppc_xive_* calls away when CONFIG_KVM
> isn't defined, thus avoiding the need for stubs.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
> v3: Method for XiveNotifierClass no longer needed
> 
> v2: Introduce in_kernel() abstract methods in the base XIVE classes
> ---
>  hw/intc/spapr_xive.c  |   45 +++++++++++++++++++++++++++++++--------------
>  hw/intc/xive.c        |   25 +++++++++++++++++++------
>  include/hw/ppc/xive.h |    1 +
>  3 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 89c8cd96670b..3c84f64dc464 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end,
>      xive_end_queue_pic_print_info(end, 6, mon);
>  }
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define spapr_xive_in_kernel(xive) \
> +    (kvm_irqchip_in_kernel() && (xive)->fd != -1)
> +
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>      XiveSource *xsrc = &xive->source;
>      int i;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_synchronize_state(xive, &local_err);
> @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = {
>  
>  static int vmstate_spapr_xive_pre_save(void *opaque)
>  {
> -    if (kvm_irqchip_in_kernel()) {
> -        return kvmppc_xive_pre_save(SPAPR_XIVE(opaque));
> +    SpaprXive *xive = SPAPR_XIVE(opaque);
> +
> +    if (spapr_xive_in_kernel(xive)) {
> +        return kvmppc_xive_pre_save(xive);
>      }
>  
>      return 0;
> @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque)
>   */
>  static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id)
>  {
> -    if (kvm_irqchip_in_kernel()) {
> -        return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id);
> +    SpaprXive *xive = SPAPR_XIVE(intc);
> +
> +    if (spapr_xive_in_kernel(xive)) {
> +        return kvmppc_xive_post_load(xive, version_id);
>      }
>  
>      return 0;
> @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn,
>          xive_source_irq_set_lsi(xsrc, lisn);
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          return kvmppc_xive_source_reset_one(xsrc, lisn, errp);
>      }
>  
> @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          kvmppc_xive_source_set_irq(&xive->source, irq, val);
>      } else {
>          xive_source_set_irq(&xive->source, irq, val);
> @@ -749,11 +760,16 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
>  
>      spapr_xive_mmio_set_enabled(xive, false);
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          kvmppc_xive_disconnect(intc);
>      }
>  }
>  
> +static bool spapr_xive_in_kernel_xptr(const XivePresenter *xptr)
> +{
> +    return spapr_xive_in_kernel(SPAPR_XIVE(xptr));
> +}
> +
>  static void spapr_xive_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -788,6 +804,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>      sicc->post_load = spapr_xive_post_load;
>  
>      xpc->match_nvt  = spapr_xive_match_nvt;
> +    xpc->in_kernel  = spapr_xive_in_kernel_xptr;
>  }
>  
>  static const TypeInfo spapr_xive_info = {
> @@ -1058,7 +1075,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu,
>          new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn);
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> @@ -1379,7 +1396,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
>       */
>  
>  out:
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
> @@ -1480,7 +1497,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
>          args[2] = 0;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
> @@ -1642,7 +1659,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
>          return H_P3;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
>                                       flags & SPAPR_XIVE_ESB_STORE);
>      } else {
> @@ -1717,7 +1734,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
>       * under KVM
>       */
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_sync_source(xive, lisn, &local_err);
> @@ -1761,7 +1778,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
>  
>      device_legacy_reset(DEVICE(xive));
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_reset(xive, &local_err);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 561d746cd1da..a453e8f4dcbe 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -592,6 +592,17 @@ static const char * const xive_tctx_ring_names[] = {
>      "USER", "OS", "POOL", "PHYS",
>  };
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define xive_in_kernel(xptr)                                            \
> +    (kvm_irqchip_in_kernel() &&                                         \
> +     ({                                                                 \
> +         XivePresenterClass *xpc = XIVE_PRESENTER_GET_CLASS(xptr);      \
> +         xpc->in_kernel ? xpc->in_kernel(xptr) : false;                 \
> +     }))
> +
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>  {
>      int cpu_index;
> @@ -606,7 +617,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>  
>      cpu_index = tctx->cs ? tctx->cs->cpu_index : -1;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
> @@ -671,7 +682,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* Connect the presenter to the VCPU (required for CPU hotplug) */
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          kvmppc_xive_cpu_connect(tctx, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> @@ -682,10 +693,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
>  {
> +    XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
>  
> -    if (kvm_irqchip_in_kernel()) {
> -        kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err);
> +    if (xive_in_kernel(tctx->xptr)) {
> +        kvmppc_xive_cpu_get_state(tctx, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;
> @@ -697,14 +709,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque)
>  
>  static int vmstate_xive_tctx_post_load(void *opaque, int version_id)
>  {
> +    XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          /*
>           * Required for hotplugged CPU, for which the state comes
>           * after all states of the machine.
>           */
> -        kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err);
> +        kvmppc_xive_cpu_set_state(tctx, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 82a61eaca74f..2f3c5af810bb 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -402,6 +402,7 @@ typedef struct XivePresenterClass {
>                       uint8_t nvt_blk, uint32_t nvt_idx,
>                       bool cam_ignore, uint8_t priority,
>                       uint32_t logic_serv, XiveTCTXMatch *match);
> +    bool (*in_kernel)(const XivePresenter *xptr);
>  } XivePresenterClass;
>  
>  int xive_presenter_tctx_match(XivePresenter *xptr, XiveTCTX *tctx,
> 
> 

Re: [PATCH v3 for-5.2 2/3] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[Cédric Le Goater]

Some more comments, because I still think there are some shortcuts.

My feeling is that all the kvmppc_xive* could be part of a QOM interface
defining how to use a kernel device backend. When the kernel IRQ device 
is not available, under TCG or under an hypervisor not advertising 
support at the KVM level, the QOM interface kernel device backend 
would be a no-op, else it would implement what the kvmppc_xive_* do
today. So, it's something we would change like ->intc after an interrupt 
mode has been negotiated. 

It's an intuition regarding POWER10/XIVE2 and nested support which 
could need a different interface of the KVM XIVE device in the future. 
I don't think we need today but it would clarify some of the shortcuts. 
  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define spapr_xive_in_kernel(xive) \
> +    (kvm_irqchip_in_kernel() && (xive)->fd != -1)
> +

Here, we have a shortcut. kvm_irqchip_in_kernel() is a compilation 
trick but the real handler :

	{
		return SPAPR_XIVE(xrtr)->fd != -1;
	}

is a shortcut. We are using ->fd to know if QEMU is connected with 
a KVM device or not.


>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>      XiveSource *xsrc = &xive->source;
>      int i;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_synchronize_state(xive, &local_err);

With a QOM interface for a kernel device backend, this would become :

	XIVE_BACKEND_GET_CLASS(xive->backend)->synchronize_state(xive);

and we could drop all the 'if' statement.



Makes sense ? I think XICS behaves the same.

Thanks,

C. 

Re: [PATCH v3 for-5.2 1/3] ppc/xive: Rework setup of XiveSource::esb_mmio

[David Gibson]

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

On Fri, Aug 07, 2020 at 02:36:01PM +0200, Cédric Le Goater wrote:
> On 8/7/20 1:32 PM, Greg Kurz wrote:
> > Depending on whether XIVE is emultated or backed with a KVM XIVE device,
> > the ESB MMIOs of a XIVE source point to an I/O memory region or a mapped
> > memory region.
> > 
> > This is currently handled by checking kvm_irqchip_in_kernel() returns
> > false in xive_source_realize(). This is a bit awkward as we usually
> > need to do extra things when we're using the in-kernel backend, not
> > less. But most important, we can do better: turn the existing "xive.esb"
> > memory region into a plain container, introduce an "xive.esb-emulated"
> > I/O subregion and rename the existing "xive.esb" subregion in the KVM
> > code to "xive.esb-kvm". Since "xive.esb-kvm" is added with overlap
> > and a higher priority, it prevails over "xive.esb-emulated" (ie.
> > a guest using KVM XIVE will interact with "xive.esb-kvm" instead of
> > the default "xive.esb-emulated" region.
> > 
> > While here, consolidate the computation of the MMIO region size in
> > a common helper.
> > 
> > Suggested-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> This is much better
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Appled to ppc-for-5.2.

> 
> Thanks,
> 
> C.
> 
> > ---
> >  hw/intc/spapr_xive_kvm.c |    4 ++--
> >  hw/intc/xive.c           |   11 ++++++-----
> >  include/hw/ppc/xive.h    |    6 ++++++
> >  3 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 893a1ee77e70..6130882be678 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -742,7 +742,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> >      SpaprXive *xive = SPAPR_XIVE(intc);
> >      XiveSource *xsrc = &xive->source;
> >      Error *local_err = NULL;
> > -    size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> > +    size_t esb_len = xive_source_esb_len(xsrc);
> >      size_t tima_len = 4ull << TM_SHIFT;
> >      CPUState *cs;
> >      int fd;
> > @@ -788,7 +788,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
> >      }
> >  
> >      memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
> > -                                      "xive.esb", esb_len, xsrc->esb_mmap);
> > +                                      "xive.esb-kvm", esb_len, xsrc->esb_mmap);
> >      memory_region_add_subregion_overlap(&xsrc->esb_mmio, 0,
> >                                          &xsrc->esb_mmio_kvm, 1);
> >  
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 9b55e0356c62..561d746cd1da 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -1128,6 +1128,7 @@ static void xive_source_reset(void *dev)
> >  static void xive_source_realize(DeviceState *dev, Error **errp)
> >  {
> >      XiveSource *xsrc = XIVE_SOURCE(dev);
> > +    size_t esb_len = xive_source_esb_len(xsrc);
> >  
> >      assert(xsrc->xive);
> >  
> > @@ -1147,11 +1148,11 @@ static void xive_source_realize(DeviceState *dev, Error **errp)
> >      xsrc->status = g_malloc0(xsrc->nr_irqs);
> >      xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
> >  
> > -    if (!kvm_irqchip_in_kernel()) {
> > -        memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> > -                              &xive_source_esb_ops, xsrc, "xive.esb",
> > -                              (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
> > -    }
> > +    memory_region_init(&xsrc->esb_mmio, OBJECT(xsrc), "xive.esb", esb_len);
> > +    memory_region_init_io(&xsrc->esb_mmio_emulated, OBJECT(xsrc),
> > +                          &xive_source_esb_ops, xsrc, "xive.esb-emulated",
> > +                          esb_len);
> > +    memory_region_add_subregion(&xsrc->esb_mmio, 0, &xsrc->esb_mmio_emulated);
> >  
> >      qemu_register_reset(xive_source_reset, dev);
> >  }
> > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > index 705cf48176fc..82a61eaca74f 100644
> > --- a/include/hw/ppc/xive.h
> > +++ b/include/hw/ppc/xive.h
> > @@ -191,6 +191,7 @@ typedef struct XiveSource {
> >      uint64_t        esb_flags;
> >      uint32_t        esb_shift;
> >      MemoryRegion    esb_mmio;
> > +    MemoryRegion    esb_mmio_emulated;
> >  
> >      /* KVM support */
> >      void            *esb_mmap;
> > @@ -215,6 +216,11 @@ static inline bool xive_source_esb_has_2page(XiveSource *xsrc)
> >          xsrc->esb_shift == XIVE_ESB_4K_2PAGE;
> >  }
> >  
> > +static inline size_t xive_source_esb_len(XiveSource *xsrc)
> > +{
> > +    return (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> > +}
> > +
> >  /* The trigger page is always the first/even page */
> >  static inline hwaddr xive_source_esb_page(XiveSource *xsrc, uint32_t srcno)
> >  {
> > 
> > 
> 

-- 
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 v3 for-5.2 2/3] ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers

[David Gibson]

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

On Fri, Aug 07, 2020 at 01:32:14PM +0200, Greg Kurz wrote:
> Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This
> ensures that QEMU won't try to use the device if KVM is disabled or if
> an in-kernel irqchip isn't required.
> 
> When using ic-mode=dual with the pseries machine, we have two possible
> interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper
> will return true as soon as any of the KVM device is created. It might
> lure QEMU to think that the other one is also around, while it is not.
> This is exactly what happens with ic-mode=dual at machine init when
> claiming IRQ numbers, which must be done on all possible IRQ backends,
> eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device
> is active but we end up calling kvmppc_xive_source_reset_one() anyway,
> which fails. This doesn't cause any trouble because of another bug :
> kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't
> see the failure.
> 
> Most of the other kvmppc_xive_* functions have similar xive->fd
> checks to filter out the case when KVM XIVE isn't active. It
> might look safer to have idempotent functions but it doesn't
> really help to understand what's going on when debugging.
> 
> Since we already have all the kvm_irqchip_in_kernel() in place,
> also have the callers to check xive->fd as well before calling
> KVM XIVE specific code. This is straight-forward for the spapr
> specific XIVE code. Some more care is needed for the platform
> agnostic XIVE code since it cannot access xive->fd directly.
> Introduce new in_kernel() methods in some base XIVE classes
> for this purpose and implement them only in spapr.
> 
> In all cases, we still need to call kvm_irqchip_in_kernel() so that
> compilers can optimize the kvmppc_xive_* calls away when CONFIG_KVM
> isn't defined, thus avoiding the need for stubs.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
> v3: Method for XiveNotifierClass no longer needed
> 
> v2: Introduce in_kernel() abstract methods in the base XIVE classes
> ---
>  hw/intc/spapr_xive.c  |   45 +++++++++++++++++++++++++++++++--------------
>  hw/intc/xive.c        |   25 +++++++++++++++++++------
>  include/hw/ppc/xive.h |    1 +
>  3 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 89c8cd96670b..3c84f64dc464 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end,
>      xive_end_queue_pic_print_info(end, 6, mon);
>  }
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define spapr_xive_in_kernel(xive) \
> +    (kvm_irqchip_in_kernel() && (xive)->fd != -1)
> +
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>      XiveSource *xsrc = &xive->source;
>      int i;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_synchronize_state(xive, &local_err);
> @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = {
>  
>  static int vmstate_spapr_xive_pre_save(void *opaque)
>  {
> -    if (kvm_irqchip_in_kernel()) {
> -        return kvmppc_xive_pre_save(SPAPR_XIVE(opaque));
> +    SpaprXive *xive = SPAPR_XIVE(opaque);
> +
> +    if (spapr_xive_in_kernel(xive)) {
> +        return kvmppc_xive_pre_save(xive);
>      }
>  
>      return 0;
> @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque)
>   */
>  static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id)
>  {
> -    if (kvm_irqchip_in_kernel()) {
> -        return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id);
> +    SpaprXive *xive = SPAPR_XIVE(intc);
> +
> +    if (spapr_xive_in_kernel(xive)) {
> +        return kvmppc_xive_post_load(xive, version_id);
>      }
>  
>      return 0;
> @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn,
>          xive_source_irq_set_lsi(xsrc, lisn);
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          return kvmppc_xive_source_reset_one(xsrc, lisn, errp);
>      }
>  
> @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          kvmppc_xive_source_set_irq(&xive->source, irq, val);
>      } else {
>          xive_source_set_irq(&xive->source, irq, val);
> @@ -749,11 +760,16 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
>  
>      spapr_xive_mmio_set_enabled(xive, false);
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          kvmppc_xive_disconnect(intc);
>      }
>  }
>  
> +static bool spapr_xive_in_kernel_xptr(const XivePresenter *xptr)
> +{
> +    return spapr_xive_in_kernel(SPAPR_XIVE(xptr));
> +}
> +
>  static void spapr_xive_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -788,6 +804,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>      sicc->post_load = spapr_xive_post_load;
>  
>      xpc->match_nvt  = spapr_xive_match_nvt;
> +    xpc->in_kernel  = spapr_xive_in_kernel_xptr;
>  }
>  
>  static const TypeInfo spapr_xive_info = {
> @@ -1058,7 +1075,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu,
>          new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn);
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> @@ -1379,7 +1396,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
>       */
>  
>  out:
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
> @@ -1480,7 +1497,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
>          args[2] = 0;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
> @@ -1642,7 +1659,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
>          return H_P3;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
>                                       flags & SPAPR_XIVE_ESB_STORE);
>      } else {
> @@ -1717,7 +1734,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
>       * under KVM
>       */
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_sync_source(xive, lisn, &local_err);
> @@ -1761,7 +1778,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
>  
>      device_legacy_reset(DEVICE(xive));
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (spapr_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_reset(xive, &local_err);
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 561d746cd1da..a453e8f4dcbe 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -592,6 +592,17 @@ static const char * const xive_tctx_ring_names[] = {
>      "USER", "OS", "POOL", "PHYS",
>  };
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define xive_in_kernel(xptr)                                            \
> +    (kvm_irqchip_in_kernel() &&                                         \
> +     ({                                                                 \
> +         XivePresenterClass *xpc = XIVE_PRESENTER_GET_CLASS(xptr);      \
> +         xpc->in_kernel ? xpc->in_kernel(xptr) : false;                 \
> +     }))
> +
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>  {
>      int cpu_index;
> @@ -606,7 +617,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>  
>      cpu_index = tctx->cs ? tctx->cs->cpu_index : -1;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
> @@ -671,7 +682,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>      }
>  
>      /* Connect the presenter to the VCPU (required for CPU hotplug) */
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          kvmppc_xive_cpu_connect(tctx, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> @@ -682,10 +693,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
>  {
> +    XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
>  
> -    if (kvm_irqchip_in_kernel()) {
> -        kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err);
> +    if (xive_in_kernel(tctx->xptr)) {
> +        kvmppc_xive_cpu_get_state(tctx, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;
> @@ -697,14 +709,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque)
>  
>  static int vmstate_xive_tctx_post_load(void *opaque, int version_id)
>  {
> +    XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (xive_in_kernel(tctx->xptr)) {
>          /*
>           * Required for hotplugged CPU, for which the state comes
>           * after all states of the machine.
>           */
> -        kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err);
> +        kvmppc_xive_cpu_set_state(tctx, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 82a61eaca74f..2f3c5af810bb 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -402,6 +402,7 @@ typedef struct XivePresenterClass {
>                       uint8_t nvt_blk, uint32_t nvt_idx,
>                       bool cam_ignore, uint8_t priority,
>                       uint32_t logic_serv, XiveTCTXMatch *match);
> +    bool (*in_kernel)(const XivePresenter *xptr);
>  } XivePresenterClass;
>  
>  int xive_presenter_tctx_match(XivePresenter *xptr, XiveTCTX *tctx,
> 
> 

-- 
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 v3 for-5.2 3/3] spapr/xive: Convert KVM device fd checks to assert()

[David Gibson]

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

On Fri, Aug 07, 2020 at 01:32:21PM +0200, Greg Kurz wrote:
> All callers guard these functions with an xive_in_kernel() helper. Make
> it clear that they are only to be called when the KVM XIVE device exists.
> 
> Note that the check on xive is dropped in kvmppc_xive_disconnect(). It
> really cannot be NULL since it comes from set_active_intc() which only
> passes pointers to allocated objects.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Applied to ppc-for-5.2.

> ---
> v2: Take the helper name change into account in the changelog
> ---
>  hw/intc/spapr_xive_kvm.c |   35 +++++++----------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 6130882be678..82a6f99f022d 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -79,10 +79,7 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
>      uint64_t state[2];
>      int ret;
>  
> -    /* The KVM XIVE device is not in use yet */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* word0 and word1 of the OS ring. */
>      state[0] = *((uint64_t *) &tctx->regs[TM_QW1_OS]);
> @@ -101,10 +98,7 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
>      uint64_t state[2] = { 0 };
>      int ret;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
>      if (ret != 0) {
> @@ -156,10 +150,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>      unsigned long vcpu_id;
>      int ret;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* Check if CPU was hot unplugged and replugged. */
>      if (kvm_cpu_is_enabled(tctx->cs)) {
> @@ -245,10 +236,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
>      SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>      uint64_t state = 0;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return -ENODEV;
> -    }
> +    assert(xive->fd != -1);
>  
>      if (xive_source_irq_is_lsi(xsrc, srcno)) {
>          state |= KVM_XIVE_LEVEL_SENSITIVE;
> @@ -592,10 +580,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, int running,
>  
>  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
>  {
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /*
>       * When the VM is stopped, the sources are masked and the previous
> @@ -622,10 +607,7 @@ int kvmppc_xive_pre_save(SpaprXive *xive)
>  {
>      Error *local_err = NULL;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (xive->fd == -1) {
> -        return 0;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* EAT: there is no extra state to query from KVM */
>  
> @@ -845,10 +827,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
>      XiveSource *xsrc;
>      size_t esb_len;
>  
> -    /* The KVM XIVE device is not in use */
> -    if (!xive || xive->fd == -1) {
> -        return;
> -    }
> +    assert(xive->fd != -1);
>  
>      /* Clear the KVM mapping */
>      xsrc = &xive->source;
> 
> 

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