[PATCH for-5.2 0/5] spapr: Cleanups for XIVE and PHB

[PATCH for-5.2 0/5] spapr: Cleanups for XIVE and PHB

[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 isn't supported on Bostons) without calling
error_setg(), which the caller doesn't expect when the patch above 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.

---

Greg Kurz (5):
      spapr/xive: Fix xive->fd if kvm_create_device() fails
      spapr/xive: Simplify kvmppc_xive_disconnect()
      ppc/xive: Introduce dedicated kvm_irqchip_in_kernel() wrappers
      spapr/xive: Convert KVM device fd checks to assert()
      spapr: Simplify error handling in spapr_phb_realize()


 hw/intc/spapr_xive.c        |   39 +++++++++++++++++---------
 hw/intc/spapr_xive_kvm.c    |   64 +++++++++++++++++++------------------------
 hw/intc/xive.c              |   30 +++++++++++++++-----
 hw/ppc/spapr_pci.c          |   16 +++++------
 include/hw/ppc/spapr_xive.h |    1 +
 include/hw/ppc/xive.h       |    2 +
 6 files changed, 87 insertions(+), 65 deletions(-)

--
Greg

[PATCH for-5.2 1/5] spapr/xive: Fix xive->fd if kvm_create_device() fails

[Greg Kurz]

If the creation of the KVM XIVE device fails for some reasons, the
negative errno ends up in xive->fd, but the rest of the code assumes
that xive->fd either contains an open fd, ie. positive value, or -1.

This doesn't cause any misbehavior except kvmppc_xive_disconnect()
that will try to close(xive->fd) during rollback and likely be
rewarded with an EBADF.

Only set xive->fd with a open fd.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index edb7ee0e74f1..d55ea4670e0e 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -745,6 +745,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
     size_t tima_len = 4ull << TM_SHIFT;
     CPUState *cs;
+    int fd;
 
     /*
      * The KVM XIVE device already in use. This is the case when
@@ -760,11 +761,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     }
 
     /* First, create the KVM XIVE device */
-    xive->fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
-    if (xive->fd < 0) {
-        error_setg_errno(errp, -xive->fd, "XIVE: error creating KVM device");
+    fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
+    if (fd < 0) {
+        error_setg_errno(errp, -fd, "XIVE: error creating KVM device");
         return -1;
     }
+    xive->fd = fd;
 
     /* Tell KVM about the # of VCPUs we may have */
     if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,

Re: [PATCH for-5.2 1/5] spapr/xive: Fix xive->fd if kvm_create_device() fails

[David Gibson]

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

On Wed, Aug 05, 2020 at 07:35:22PM +0200, Greg Kurz wrote:
> If the creation of the KVM XIVE device fails for some reasons, the
> negative errno ends up in xive->fd, but the rest of the code assumes
> that xive->fd either contains an open fd, ie. positive value, or -1.
> 
> This doesn't cause any misbehavior except kvmppc_xive_disconnect()
> that will try to close(xive->fd) during rollback and likely be
> rewarded with an EBADF.
> 
> Only set xive->fd with a open fd.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index edb7ee0e74f1..d55ea4670e0e 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -745,6 +745,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>      size_t tima_len = 4ull << TM_SHIFT;
>      CPUState *cs;
> +    int fd;
>  
>      /*
>       * The KVM XIVE device already in use. This is the case when
> @@ -760,11 +761,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      }
>  
>      /* First, create the KVM XIVE device */
> -    xive->fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
> -    if (xive->fd < 0) {
> -        error_setg_errno(errp, -xive->fd, "XIVE: error creating KVM device");
> +    fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_XIVE, false);
> +    if (fd < 0) {
> +        error_setg_errno(errp, -fd, "XIVE: error creating KVM device");
>          return -1;
>      }
> +    xive->fd = fd;
>  
>      /* Tell KVM about the # of VCPUs we may have */
>      if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> 
> 

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

[PATCH for-5.2 2/5] spapr/xive: Simplify kvmppc_xive_disconnect()

[Greg Kurz]

Since this function begins with:

    /* The KVM XIVE device is not in use */
    if (!xive || xive->fd == -1) {
        return;
    }

we obviously don't need to check xive->fd again.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index d55ea4670e0e..893a1ee77e70 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -873,10 +873,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
      * and removed from the list of devices of the VM. The VCPU
      * presenters are also detached from the device.
      */
-    if (xive->fd != -1) {
-        close(xive->fd);
-        xive->fd = -1;
-    }
+    close(xive->fd);
+    xive->fd = -1;
 
     kvm_kernel_irqchip = false;
     kvm_msi_via_irqfd_allowed = false;

Re: [PATCH for-5.2 2/5] spapr/xive: Simplify kvmppc_xive_disconnect()

[David Gibson]

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

On Wed, Aug 05, 2020 at 07:35:29PM +0200, Greg Kurz wrote:
> Since this function begins with:
> 
>     /* The KVM XIVE device is not in use */
>     if (!xive || xive->fd == -1) {
>         return;
>     }
> 
> we obviously don't need to check xive->fd again.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index d55ea4670e0e..893a1ee77e70 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -873,10 +873,8 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
>       * and removed from the list of devices of the VM. The VCPU
>       * presenters are also detached from the device.
>       */
> -    if (xive->fd != -1) {
> -        close(xive->fd);
> -        xive->fd = -1;
> -    }
> +    close(xive->fd);
> +    xive->fd = -1;
>  
>      kvm_kernel_irqchip = false;
>      kvm_msi_via_irqfd_allowed = false;
> 
> 

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

[PATCH for-5.2 3/5] 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. Introduce helpers to access the underlying
fd for various XIVE types and call them with a kvm_irqchip_in_kernel()
guard : this allows the compiler to optimize the kvmppc_xive_* calls
out if CONFIG_KVM isn't defined, thus avoiding the need for stubs.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive.c        |   39 +++++++++++++++++++++++++--------------
 hw/intc/spapr_xive_kvm.c    |   15 +++++++++++++++
 hw/intc/xive.c              |   30 +++++++++++++++++++++++-------
 include/hw/ppc/spapr_xive.h |    1 +
 include/hw/ppc/xive.h       |    2 ++
 5 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 89c8cd96670b..98e6489fb16d 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 kvmppc_xive_in_kernel(xive) \
+    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive))
+
 void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
 {
     XiveSource *xsrc = &xive->source;
     int i;
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_xive_in_kernel(xive)) {
         kvmppc_xive_source_set_irq(&xive->source, irq, val);
     } else {
         xive_source_set_irq(&xive->source, irq, val);
@@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
 
     spapr_xive_mmio_set_enabled(xive, false);
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         kvmppc_xive_disconnect(intc);
     }
 }
@@ -1058,7 +1069,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 (kvmppc_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
@@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
      */
 
 out:
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
@@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
         args[2] = 0;
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
@@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
         return H_P3;
     }
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
                                      flags & SPAPR_XIVE_ESB_STORE);
     } else {
@@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
      * under KVM
      */
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_sync_source(xive, lisn, &local_err);
@@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
 
     device_legacy_reset(DEVICE(xive));
 
-    if (kvm_irqchip_in_kernel()) {
+    if (kvmppc_xive_in_kernel(xive)) {
         Error *local_err = NULL;
 
         kvmppc_xive_reset(xive, &local_err);
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 893a1ee77e70..a9657e2b0cda 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
         xive->change = NULL;
     }
 }
+
+bool kvmppc_xive_kernel_irqchip(SpaprXive *xive)
+{
+    return xive->fd != -1;
+}
+
+bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx)
+{
+    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr));
+}
+
+bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc)
+{
+    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive));
+}
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 9b55e0356c62..2e1af4530dc5 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -592,6 +592,13 @@ 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 kvmppc_xive_in_kernel_tctx(tctx) \
+    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx))
+
 void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
 {
     int cpu_index;
@@ -606,7 +613,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
         Error *local_err = NULL;
 
         kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
@@ -671,7 +678,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
         kvmppc_xive_cpu_connect(tctx, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -682,10 +689,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
+        kvmppc_xive_cpu_get_state(tctx, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -1;
@@ -697,14 +705,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
         /*
          * 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;
@@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev)
     memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs);
 }
 
+/*
+ * kvm_irqchip_in_kernel() will cause the compiler to turn this
+ * info a nop if CONFIG_KVM isn't defined.
+ */
+#define kvmppc_xive_in_kernel_xsrc(xsrc) \
+    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc))
+
 static void xive_source_realize(DeviceState *dev, Error **errp)
 {
     XiveSource *xsrc = XIVE_SOURCE(dev);
@@ -1147,7 +1163,7 @@ 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()) {
+    if (!kvmppc_xive_in_kernel_xsrc(xsrc)) {
         memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
                               &xive_source_esb_ops, xsrc, "xive.esb",
                               (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 93d09d68deb7..9fd00378ac1f 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
 void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);
 int kvmppc_xive_pre_save(SpaprXive *xive);
 int kvmppc_xive_post_load(SpaprXive *xive, int version_id);
+bool kvmppc_xive_kernel_irqchip(SpaprXive *xive);
 
 #endif /* PPC_SPAPR_XIVE_H */
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 705cf48176fc..ea66e9ea9c7b 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
 void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
 void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
 void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
+bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx);
+bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc);
 
 #endif /* PPC_XIVE_H */

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

[David Gibson]

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

On Wed, Aug 05, 2020 at 07:35:37PM +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. Introduce helpers to access the underlying
> fd for various XIVE types and call them with a kvm_irqchip_in_kernel()
> guard : this allows the compiler to optimize the kvmppc_xive_* calls
> out if CONFIG_KVM isn't defined, thus avoiding the need for stubs.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/spapr_xive.c        |   39 +++++++++++++++++++++++++--------------
>  hw/intc/spapr_xive_kvm.c    |   15 +++++++++++++++
>  hw/intc/xive.c              |   30 +++++++++++++++++++++++-------
>  include/hw/ppc/spapr_xive.h |    1 +
>  include/hw/ppc/xive.h       |    2 ++
>  5 files changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 89c8cd96670b..98e6489fb16d 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 kvmppc_xive_in_kernel(xive) \
> +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive))

This all seems like a good idea, my only concern is that the semantic
difference between kvmppc_xive_in_kernel() and
kvmppc_xive_kernel_irqchip() is pretty subtle and non-obvious (and
likewise for the tctx and xsrc variations).

We're right in the XIVE implementation, so I suggest you eliminate the
kvmppc_xive_kernel_irqchip() etc. wrappers and just open code a check
on the fd in kvmppc_xive_in_kernel() etc.

> +
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
>  {
>      XiveSource *xsrc = &xive->source;
>      int i;
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_xive_in_kernel(xive)) {
>          kvmppc_xive_source_set_irq(&xive->source, irq, val);
>      } else {
>          xive_source_set_irq(&xive->source, irq, val);
> @@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
>  
>      spapr_xive_mmio_set_enabled(xive, false);
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          kvmppc_xive_disconnect(intc);
>      }
>  }
> @@ -1058,7 +1069,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 (kvmppc_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> @@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
>       */
>  
>  out:
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
> @@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
>          args[2] = 0;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
> @@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
>          return H_P3;
>      }
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
>                                       flags & SPAPR_XIVE_ESB_STORE);
>      } else {
> @@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
>       * under KVM
>       */
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_sync_source(xive, lisn, &local_err);
> @@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
>  
>      device_legacy_reset(DEVICE(xive));
>  
> -    if (kvm_irqchip_in_kernel()) {
> +    if (kvmppc_xive_in_kernel(xive)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_reset(xive, &local_err);
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 893a1ee77e70..a9657e2b0cda 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
>          xive->change = NULL;
>      }
>  }
> +
> +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive)
> +{
> +    return xive->fd != -1;
> +}
> +
> +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx)
> +{
> +    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr));
> +}
> +
> +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc)
> +{
> +    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive));
> +}
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 9b55e0356c62..2e1af4530dc5 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -592,6 +592,13 @@ 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 kvmppc_xive_in_kernel_tctx(tctx) \
> +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx))
> +
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>  {
>      int cpu_index;
> @@ -606,7 +613,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
>          Error *local_err = NULL;
>  
>          kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
> @@ -671,7 +678,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
>          kvmppc_xive_cpu_connect(tctx, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> @@ -682,10 +689,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
> +        kvmppc_xive_cpu_get_state(tctx, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return -1;
> @@ -697,14 +705,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
>          /*
>           * 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;
> @@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev)
>      memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs);
>  }
>  
> +/*
> + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> + * info a nop if CONFIG_KVM isn't defined.
> + */
> +#define kvmppc_xive_in_kernel_xsrc(xsrc) \
> +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc))
> +
>  static void xive_source_realize(DeviceState *dev, Error **errp)
>  {
>      XiveSource *xsrc = XIVE_SOURCE(dev);
> @@ -1147,7 +1163,7 @@ 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()) {
> +    if (!kvmppc_xive_in_kernel_xsrc(xsrc)) {
>          memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
>                                &xive_source_esb_ops, xsrc, "xive.esb",
>                                (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 93d09d68deb7..9fd00378ac1f 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
>  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);
>  int kvmppc_xive_pre_save(SpaprXive *xive);
>  int kvmppc_xive_post_load(SpaprXive *xive, int version_id);
> +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive);
>  
>  #endif /* PPC_SPAPR_XIVE_H */
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 705cf48176fc..ea66e9ea9c7b 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
>  void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
>  void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
>  void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
> +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx);
> +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc);
>  
>  #endif /* PPC_XIVE_H */
> 
> 

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

[Greg Kurz]

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

On Thu, 6 Aug 2020 15:08:35 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 05, 2020 at 07:35:37PM +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. Introduce helpers to access the underlying
> > fd for various XIVE types and call them with a kvm_irqchip_in_kernel()
> > guard : this allows the compiler to optimize the kvmppc_xive_* calls
> > out if CONFIG_KVM isn't defined, thus avoiding the need for stubs.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/spapr_xive.c        |   39 +++++++++++++++++++++++++--------------
> >  hw/intc/spapr_xive_kvm.c    |   15 +++++++++++++++
> >  hw/intc/xive.c              |   30 +++++++++++++++++++++++-------
> >  include/hw/ppc/spapr_xive.h |    1 +
> >  include/hw/ppc/xive.h       |    2 ++
> >  5 files changed, 66 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 89c8cd96670b..98e6489fb16d 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 kvmppc_xive_in_kernel(xive) \
> > +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive))
> 
> This all seems like a good idea, my only concern is that the semantic
> difference between kvmppc_xive_in_kernel() and
> kvmppc_xive_kernel_irqchip() is pretty subtle and non-obvious (and
> likewise for the tctx and xsrc variations).
> 

Yeah, kvmppc_xive_in_kernel() and friends are essentially versions
that also work when CONFIG_KVM is not set (needed to build
qemu-system-ppc64 for non-POWER hosts).

> We're right in the XIVE implementation, so I suggest you eliminate the
> kvmppc_xive_kernel_irqchip() etc. wrappers and just open code a check
> on the fd in kvmppc_xive_in_kernel() etc.
> 

This is true for the kvmppc_xive_in_kernel() variant that takes a
SpaprXive * arg. Things are different for the tctx and xsrc
variants, because xive.c is platform agnostic (shared between
spapr and powernv). Exposing the SpaprXive type to xive.c
would look like a layering violation to me.

Maybe introducing abstract class methods at the base XIVE level (eg.
XivePresenterClass) and implement them in platform-specific code
would make things clearer ?

> > +
> >  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon)
> >  {
> >      XiveSource *xsrc = &xive->source;
> >      int i;
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_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 (kvmppc_xive_in_kernel(xive)) {
> >          kvmppc_xive_source_set_irq(&xive->source, irq, val);
> >      } else {
> >          xive_source_set_irq(&xive->source, irq, val);
> > @@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc)
> >  
> >      spapr_xive_mmio_set_enabled(xive, false);
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          kvmppc_xive_disconnect(intc);
> >      }
> >  }
> > @@ -1058,7 +1069,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 (kvmppc_xive_in_kernel(xive)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> > @@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu,
> >       */
> >  
> >  out:
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err);
> > @@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu,
> >          args[2] = 0;
> >      }
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err);
> > @@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
> >          return H_P3;
> >      }
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data,
> >                                       flags & SPAPR_XIVE_ESB_STORE);
> >      } else {
> > @@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu,
> >       * under KVM
> >       */
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_sync_source(xive, lisn, &local_err);
> > @@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
> >  
> >      device_legacy_reset(DEVICE(xive));
> >  
> > -    if (kvm_irqchip_in_kernel()) {
> > +    if (kvmppc_xive_in_kernel(xive)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_reset(xive, &local_err);
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 893a1ee77e70..a9657e2b0cda 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc)
> >          xive->change = NULL;
> >      }
> >  }
> > +
> > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive)
> > +{
> > +    return xive->fd != -1;
> > +}
> > +
> > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx)
> > +{
> > +    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr));
> > +}
> > +
> > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc)
> > +{
> > +    return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive));
> > +}
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > index 9b55e0356c62..2e1af4530dc5 100644
> > --- a/hw/intc/xive.c
> > +++ b/hw/intc/xive.c
> > @@ -592,6 +592,13 @@ 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 kvmppc_xive_in_kernel_tctx(tctx) \
> > +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx))
> > +
> >  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
> >  {
> >      int cpu_index;
> > @@ -606,7 +613,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
> >          Error *local_err = NULL;
> >  
> >          kvmppc_xive_cpu_synchronize_state(tctx, &local_err);
> > @@ -671,7 +678,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
> >          kvmppc_xive_cpu_connect(tctx, &local_err);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> > @@ -682,10 +689,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
> > +        kvmppc_xive_cpu_get_state(tctx, &local_err);
> >          if (local_err) {
> >              error_report_err(local_err);
> >              return -1;
> > @@ -697,14 +705,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 (kvmppc_xive_in_kernel_tctx(tctx)) {
> >          /*
> >           * 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;
> > @@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev)
> >      memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs);
> >  }
> >  
> > +/*
> > + * kvm_irqchip_in_kernel() will cause the compiler to turn this
> > + * info a nop if CONFIG_KVM isn't defined.
> > + */
> > +#define kvmppc_xive_in_kernel_xsrc(xsrc) \
> > +    (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc))
> > +
> >  static void xive_source_realize(DeviceState *dev, Error **errp)
> >  {
> >      XiveSource *xsrc = XIVE_SOURCE(dev);
> > @@ -1147,7 +1163,7 @@ 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()) {
> > +    if (!kvmppc_xive_in_kernel_xsrc(xsrc)) {
> >          memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> >                                &xive_source_esb_ops, xsrc, "xive.esb",
> >                                (1ull << xsrc->esb_shift) * xsrc->nr_irqs);
> > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> > index 93d09d68deb7..9fd00378ac1f 100644
> > --- a/include/hw/ppc/spapr_xive.h
> > +++ b/include/hw/ppc/spapr_xive.h
> > @@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
> >  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);
> >  int kvmppc_xive_pre_save(SpaprXive *xive);
> >  int kvmppc_xive_post_load(SpaprXive *xive, int version_id);
> > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive);
> >  
> >  #endif /* PPC_SPAPR_XIVE_H */
> > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > index 705cf48176fc..ea66e9ea9c7b 100644
> > --- a/include/hw/ppc/xive.h
> > +++ b/include/hw/ppc/xive.h
> > @@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
> >  void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
> >  void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
> >  void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
> > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx);
> > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc);
> >  
> >  #endif /* PPC_XIVE_H */
> > 
> > 
> 


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

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

[Greg Kurz]

All callers guard these functions with kvmppc_xive_in_kernel() or one
of its variants. Make it clear that these functions are only to be
called when the KVM XIVE device is active.

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>
---
 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 a9657e2b0cda..3364832de83a 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 for-5.2 4/5] spapr/xive: Convert KVM device fd checks to assert()

[David Gibson]

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

On Wed, Aug 05, 2020 at 07:35:44PM +0200, Greg Kurz wrote:
> All callers guard these functions with kvmppc_xive_in_kernel() or one
> of its variants. Make it clear that these functions are only to be
> called when the KVM XIVE device is active.
> 
> 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>

> ---
>  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 a9657e2b0cda..3364832de83a 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 --]

[PATCH for-5.2 5/5] spapr: Simplify error handling in spapr_phb_realize()

[Greg Kurz]

The spapr_phb_realize() function has a local_err variable which
is used to:

1) check failures of spapr_irq_findone() and spapr_irq_claim()

2) prepend extra information to the error message

Recent work from Markus Armbruster highlighted we get better
code when testing the return value of a function, rather than
setting up all the local_err boiler plate. For similar reasons,
it is now preferred to use ERRP_GUARD() and error_prepend()
rather than error_propagate_prepend().

Since spapr_irq_findone() and spapr_irq_claim() return negative
values in case of failure, do both changes.

This is just cleanup, no functional impact.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
Note that the int32_t=>int followup change suggested by Markus was squashed
into this patch.
---
 hw/ppc/spapr_pci.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 363cdb3f7b8d..0a418f1e6711 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
      * tries to add a sPAPR PHB to a non-pseries machine.
      */
@@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     uint64_t msi_window_size = 4096;
     SpaprTceTable *tcet;
     const unsigned windows_supported = spapr_phb_windows_supported(sphb);
-    Error *local_err = NULL;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
@@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
 
     /* Initialize the LSI table */
     for (i = 0; i < PCI_NUM_PINS; i++) {
-        uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
+        int irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
 
         if (smc->legacy_irq_allocation) {
-            irq = spapr_irq_findone(spapr, &local_err);
-            if (local_err) {
-                error_propagate_prepend(errp, local_err,
-                                        "can't allocate LSIs: ");
+            irq = spapr_irq_findone(spapr, errp);
+            if (irq < 0) {
+                error_prepend(errp, "can't allocate LSIs: ");
                 /*
                  * Older machines will never support PHB hotplug, ie, this is an
                  * init only path and QEMU will terminate. No need to rollback.
@@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
             }
         }
 
-        spapr_irq_claim(spapr, irq, true, &local_err);
-        if (local_err) {
-            error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
+        if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
+            error_prepend(errp, "can't allocate LSIs: ");
             goto unrealize;
         }
 

Re: [PATCH for-5.2 5/5] spapr: Simplify error handling in spapr_phb_realize()

[David Gibson]

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

On Wed, Aug 05, 2020 at 07:35:51PM +0200, Greg Kurz wrote:
> The spapr_phb_realize() function has a local_err variable which
> is used to:
> 
> 1) check failures of spapr_irq_findone() and spapr_irq_claim()
> 
> 2) prepend extra information to the error message
> 
> Recent work from Markus Armbruster highlighted we get better
> code when testing the return value of a function, rather than
> setting up all the local_err boiler plate. For similar reasons,
> it is now preferred to use ERRP_GUARD() and error_prepend()
> rather than error_propagate_prepend().
> 
> Since spapr_irq_findone() and spapr_irq_claim() return negative
> values in case of failure, do both changes.
> 
> This is just cleanup, no functional impact.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> Note that the int32_t=>int followup change suggested by Markus was squashed
> into this patch.
> ---
>  hw/ppc/spapr_pci.c |   16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 363cdb3f7b8d..0a418f1e6711 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1796,6 +1796,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
>  
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
>       * tries to add a sPAPR PHB to a non-pseries machine.
>       */
> @@ -1813,7 +1814,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      uint64_t msi_window_size = 4096;
>      SpaprTceTable *tcet;
>      const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> -    Error *local_err = NULL;
>  
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> @@ -1964,13 +1964,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  
>      /* Initialize the LSI table */
>      for (i = 0; i < PCI_NUM_PINS; i++) {
> -        uint32_t irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
> +        int irq = SPAPR_IRQ_PCI_LSI + sphb->index * PCI_NUM_PINS + i;
>  
>          if (smc->legacy_irq_allocation) {
> -            irq = spapr_irq_findone(spapr, &local_err);
> -            if (local_err) {
> -                error_propagate_prepend(errp, local_err,
> -                                        "can't allocate LSIs: ");
> +            irq = spapr_irq_findone(spapr, errp);
> +            if (irq < 0) {
> +                error_prepend(errp, "can't allocate LSIs: ");
>                  /*
>                   * Older machines will never support PHB hotplug, ie, this is an
>                   * init only path and QEMU will terminate. No need to rollback.
> @@ -1979,9 +1978,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              }
>          }
>  
> -        spapr_irq_claim(spapr, irq, true, &local_err);
> -        if (local_err) {
> -            error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
> +        if (spapr_irq_claim(spapr, irq, true, errp) < 0) {
> +            error_prepend(errp, "can't allocate LSIs: ");
>              goto unrealize;
>          }
>  
> 
> 

-- 
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 for-5.2 0/5] spapr: Cleanups for XIVE and PHB

[no-reply]

Patchew URL: https://patchew.org/QEMU/159664891296.638781.18417631893299150932.stgit@bahia.lan/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/159664891296.638781.18417631893299150932.stgit@bahia.lan/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com