[PATCH 00/14] ppc/spapr: Error handling fixes and cleanups

[PATCH 00/14] ppc/spapr: Error handling fixes and cleanups

[Greg Kurz]

This series aims to be a follow up to Markus Ambruster's massive
work to sanitize error handling. It focuses on the following:
- check return values rather than a pointer to a local Error object
- get rid of the local_err + error_propagate() boilerplate when
  possible
- propage negative errnos to vmstate instead of -1

---

Greg Kurz (14):
      spapr: Simplify error handling in spapr_phb_realize()
      spapr/xive: Rework error handling of kvmppc_xive_cpu_connect()
      spapr/xive: Rework error handling of kvmppc_xive_source_reset()
      spapr/xive: Rework error handling of kvmppc_xive_mmap()
      spapr/xive: Rework error handling of kvmppc_xive_cpu_[gs]et_state()
      spapr/xive: Rework error handling of kvmppc_xive_[gs]et_queue_config()
      spapr/xive: Rework error handling in kvmppc_xive_get_queues()
      spapr/xive: Rework error handling of kvmppc_xive_set_source_config()
      spapr/kvm: Fix error handling in kvmppc_xive_pre_save()
      spapr/xive: Fix error handling in kvmppc_xive_post_load()
      ppc/xive: Fix error handling in vmstate_xive_tctx_*() callbacks
      spapr/xive: Simplify error handling in kvmppc_xive_connect()
      ppc/xive: Simplify error handling in xive_tctx_realize()
      spapr/xive: Simplify error handling of kvmppc_xive_cpu_synchronize_state()


 hw/intc/spapr_xive_kvm.c    |  202 ++++++++++++++++++++++---------------------
 hw/intc/xive.c              |   19 ++--
 hw/ppc/spapr_pci.c          |   16 +--
 include/hw/ppc/spapr_xive.h |    8 +-
 include/hw/ppc/xive.h       |    8 +-
 5 files changed, 125 insertions(+), 128 deletions(-)

--
Greg

[PATCH 01/14] 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 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;
         }
 

[PATCH 02/14] spapr/xive: Rework error handling of kvmppc_xive_cpu_connect()

[Greg Kurz]

Use error_setg_errno() instead of error_setg(strerror()). While here,
use -ret instead of errno since kvm_vcpu_enable_cap() returns a negative
errno on failure.

Use ERRP_GUARD() to ensure that errp can be passed to error_append_hint(),
and get rid of the local_err boilerplate.

Propagate the return value so that callers may use it as well to check
failures.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |   21 ++++++++++-----------
 include/hw/ppc/xive.h    |    2 +-
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 82a6f99f022d..aa1a2f915363 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -144,8 +144,9 @@ void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
     }
 }
 
-void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
+int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
 {
+    ERRP_GUARD();
     SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
     unsigned long vcpu_id;
     int ret;
@@ -154,7 +155,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
 
     /* Check if CPU was hot unplugged and replugged. */
     if (kvm_cpu_is_enabled(tctx->cs)) {
-        return;
+        return 0;
     }
 
     vcpu_id = kvm_arch_vcpu_id(tctx->cs);
@@ -162,20 +163,18 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
     ret = kvm_vcpu_enable_cap(tctx->cs, KVM_CAP_PPC_IRQ_XIVE, 0, xive->fd,
                               vcpu_id, 0);
     if (ret < 0) {
-        Error *local_err = NULL;
-
-        error_setg(&local_err,
-                   "XIVE: unable to connect CPU%ld to KVM device: %s",
-                   vcpu_id, strerror(errno));
-        if (errno == ENOSPC) {
-            error_append_hint(&local_err, "Try -smp maxcpus=N with N < %u\n",
+        error_setg_errno(errp, -ret,
+                         "XIVE: unable to connect CPU%ld to KVM device",
+                         vcpu_id);
+        if (ret == -ENOSPC) {
+            error_append_hint(errp, "Try -smp maxcpus=N with N < %u\n",
                               MACHINE(qdev_get_machine())->smp.max_cpus);
         }
-        error_propagate(errp, local_err);
-        return;
+        return ret;
     }
 
     kvm_cpu_enable(tctx->cs);
+    return 0;
 }
 
 /*
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 2f3c5af810bb..2d87ed43728a 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -487,7 +487,7 @@ void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb);
 
 int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
 void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
-void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
+int 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);

[PATCH 03/14] spapr/xive: Rework error handling of kvmppc_xive_source_reset()

[Greg Kurz]

Since kvmppc_xive_source_reset_one() has a return value, convert
kvmppc_xive_source_reset() to use it for error checking. This
allows to get rid of the local_err boiler plate.

Propagate the return value so that callers may use it as well to check
failures.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index aa1a2f915363..d801bf5cd11c 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -248,24 +248,25 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
                              true, errp);
 }
 
-static void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
+static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
 {
     SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
     int i;
 
     for (i = 0; i < xsrc->nr_irqs; i++) {
-        Error *local_err = NULL;
+        int ret;
 
         if (!xive_eas_is_valid(&xive->eat[i])) {
             continue;
         }
 
-        kvmppc_xive_source_reset_one(xsrc, i, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+        ret = kvmppc_xive_source_reset_one(xsrc, i, errp);
+        if (ret < 0) {
+            return ret;
         }
     }
+
+    return 0;
 }
 
 /*

[PATCH 04/14] spapr/xive: Rework error handling of kvmppc_xive_mmap()

[Greg Kurz]

Callers currently check failures of kvmppc_xive_mmap() through the
@errp argument, which isn't a recommanded practice. It is preferred
to use a return value when possible.

Since NULL isn't an invalid address in theory, it seems better to
return MAP_FAILED and to teach callers to handle it.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index d801bf5cd11c..b2a36fd59dae 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -698,6 +698,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
     return 0;
 }
 
+/* Returns MAP_FAILED on error and sets errno */
 static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
                               Error **errp)
 {
@@ -708,7 +709,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
                 pgoff << page_shift);
     if (addr == MAP_FAILED) {
         error_setg_errno(errp, errno, "XIVE: unable to set memory mapping");
-        return NULL;
     }
 
     return addr;
@@ -728,6 +728,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     size_t tima_len = 4ull << TM_SHIFT;
     CPUState *cs;
     int fd;
+    void *addr;
 
     /*
      * The KVM XIVE device already in use. This is the case when
@@ -763,11 +764,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     /*
      * 1. Source ESB pages - KVM mapping
      */
-    xsrc->esb_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
-                                      &local_err);
-    if (local_err) {
+    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
+                            &local_err);
+    if (addr == MAP_FAILED) {
         goto fail;
     }
+    xsrc->esb_mmap = addr;
 
     memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
                                       "xive.esb-kvm", esb_len, xsrc->esb_mmap);
@@ -781,11 +783,13 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     /*
      * 3. TIMA pages - KVM mapping
      */
-    xive->tm_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
-                                     &local_err);
-    if (local_err) {
+    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
+                            &local_err);
+    if (addr == MAP_FAILED) {
         goto fail;
     }
+    xive->tm_mmap = addr;
+
     memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive),
                                       "xive.tima", tima_len, xive->tm_mmap);
     memory_region_add_subregion_overlap(&xive->tm_mmio, 0,

[PATCH 05/14] spapr/xive: Rework error handling of kvmppc_xive_cpu_[gs]et_state()

[Greg Kurz]

kvm_set_one_reg() returns a negative errno on failure, use that instead
of errno. Also propagate it to callers so they can use it to check
for failures and hopefully get rid of their local_err boilerplate.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |   15 ++++++++++-----
 include/hw/ppc/xive.h    |    4 ++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index b2a36fd59dae..5e088ccbf885 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -73,7 +73,7 @@ static void kvm_cpu_disable_all(void)
  * XIVE Thread Interrupt Management context (KVM)
  */
 
-void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
+int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
 {
     SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
     uint64_t state[2];
@@ -86,13 +86,16 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
 
     ret = kvm_set_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
     if (ret != 0) {
-        error_setg_errno(errp, errno,
+        error_setg_errno(errp, -ret,
                          "XIVE: could not restore KVM state of CPU %ld",
                          kvm_arch_vcpu_id(tctx->cs));
+        return ret;
     }
+
+    return 0;
 }
 
-void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
+int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
 {
     SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
     uint64_t state[2] = { 0 };
@@ -102,14 +105,16 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
 
     ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
     if (ret != 0) {
-        error_setg_errno(errp, errno,
+        error_setg_errno(errp, -ret,
                          "XIVE: could not capture KVM state of CPU %ld",
                          kvm_arch_vcpu_id(tctx->cs));
-        return;
+        return ret;
     }
 
     /* word0 and word1 of the OS ring. */
     *((uint64_t *) &tctx->regs[TM_QW1_OS]) = state[0];
+
+    return 0;
 }
 
 typedef struct {
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 2d87ed43728a..785c905357dc 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -489,7 +489,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
 void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
 int 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);
+int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
+int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
 
 #endif /* PPC_XIVE_H */

[PATCH 06/14] spapr/xive: Rework error handling of kvmppc_xive_[gs]et_queue_config()

[Greg Kurz]

Since kvm_device_access() returns a negative errno on failure, convert
kvmppc_xive_get_queue_config() and kvmppc_xive_set_queue_config() to
use it for error checking. This allows to get rid of the local_err
boilerplate.

Propagate the return value so that callers may use it as well to check
failures.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c    |   35 ++++++++++++++++-------------------
 include/hw/ppc/spapr_xive.h |    4 ++--
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 5e088ccbf885..696623f717b7 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -374,15 +374,15 @@ void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
 /*
  * sPAPR XIVE interrupt controller (KVM)
  */
-void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
-                                  uint32_t end_idx, XiveEND *end,
-                                  Error **errp)
+int kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
+                                 uint32_t end_idx, XiveEND *end,
+                                 Error **errp)
 {
     struct kvm_ppc_xive_eq kvm_eq = { 0 };
     uint64_t kvm_eq_idx;
     uint8_t priority;
     uint32_t server;
-    Error *local_err = NULL;
+    int ret;
 
     assert(xive_end_is_valid(end));
 
@@ -394,11 +394,10 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
     kvm_eq_idx |= server << KVM_XIVE_EQ_SERVER_SHIFT &
         KVM_XIVE_EQ_SERVER_MASK;
 
-    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
-                      &kvm_eq, false, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
+                            &kvm_eq, false, errp);
+    if (ret < 0) {
+        return ret;
     }
 
     /*
@@ -408,17 +407,18 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
      */
     end->w1 = xive_set_field32(END_W1_GENERATION, 0ul, kvm_eq.qtoggle) |
         xive_set_field32(END_W1_PAGE_OFF, 0ul, kvm_eq.qindex);
+
+    return 0;
 }
 
-void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
-                                  uint32_t end_idx, XiveEND *end,
-                                  Error **errp)
+int kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
+                                 uint32_t end_idx, XiveEND *end,
+                                 Error **errp)
 {
     struct kvm_ppc_xive_eq kvm_eq = { 0 };
     uint64_t kvm_eq_idx;
     uint8_t priority;
     uint32_t server;
-    Error *local_err = NULL;
 
     /*
      * Build the KVM state from the local END structure.
@@ -456,12 +456,9 @@ void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
     kvm_eq_idx |= server << KVM_XIVE_EQ_SERVER_SHIFT &
         KVM_XIVE_EQ_SERVER_MASK;
 
-    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
-                      &kvm_eq, true, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    return
+        kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
+                          &kvm_eq, true, errp);
 }
 
 void kvmppc_xive_reset(SpaprXive *xive, Error **errp)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 93d09d68deb7..d0a08b618f79 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -85,10 +85,10 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
 void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
 uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
                             uint64_t data, bool write);
-void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
+int kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
                                  uint32_t end_idx, XiveEND *end,
                                  Error **errp);
-void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
+int kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
                                  uint32_t end_idx, XiveEND *end,
                                  Error **errp);
 void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);

[PATCH 07/14] spapr/xive: Rework error handling in kvmppc_xive_get_queues()

[Greg Kurz]

Since kvmppc_xive_get_queue_config() has a return value, convert
kvmppc_xive_get_queues() to use it for error checking. This allows
to get rid of the local_err boiler plate.

Propagate the return value so that callers may use it as well to check
failures.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 696623f717b7..4142aaffff47 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -467,23 +467,24 @@ void kvmppc_xive_reset(SpaprXive *xive, Error **errp)
                       NULL, true, errp);
 }
 
-static void kvmppc_xive_get_queues(SpaprXive *xive, Error **errp)
+static int kvmppc_xive_get_queues(SpaprXive *xive, Error **errp)
 {
-    Error *local_err = NULL;
     int i;
+    int ret;
 
     for (i = 0; i < xive->nr_ends; i++) {
         if (!xive_end_is_valid(&xive->endt[i])) {
             continue;
         }
 
-        kvmppc_xive_get_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
-                                     &xive->endt[i], &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+        ret = kvmppc_xive_get_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
+                                           &xive->endt[i], errp);
+        if (ret < 0) {
+            return ret;
         }
     }
+
+    return 0;
 }
 
 /*

[PATCH 08/14] spapr/xive: Rework error handling of kvmppc_xive_set_source_config()

[Greg Kurz]

Since kvm_device_access() returns a negative errno on failure, convert
kvmppc_xive_set_source_config() to use it for error checking. This allows
to get rid of the local_err boilerplate.

Propagate the return value so that callers may use it as well to check
failures.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c    |   13 ++++---------
 include/hw/ppc/spapr_xive.h |    4 ++--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 4142aaffff47..f2dda692183b 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -186,8 +186,8 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
  * XIVE Interrupt Source (KVM)
  */
 
-void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
-                                   Error **errp)
+int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
+                                  Error **errp)
 {
     uint32_t end_idx;
     uint32_t end_blk;
@@ -196,7 +196,6 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
     bool masked;
     uint32_t eisn;
     uint64_t kvm_src;
-    Error *local_err = NULL;
 
     assert(xive_eas_is_valid(eas));
 
@@ -216,12 +215,8 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
     kvm_src |= ((uint64_t)eisn << KVM_XIVE_SOURCE_EISN_SHIFT) &
         KVM_XIVE_SOURCE_EISN_MASK;
 
-    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_CONFIG, lisn,
-                      &kvm_src, true, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    return kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_CONFIG, lisn,
+                             &kvm_src, true, errp);
 }
 
 void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index d0a08b618f79..0ffbe0be0280 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -80,8 +80,8 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
                         Error **errp);
 void kvmppc_xive_disconnect(SpaprInterruptController *intc);
 void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
-void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
-                                   Error **errp);
+int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
+                                  Error **errp);
 void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
 uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
                             uint64_t data, bool write);

[PATCH 09/14] spapr/kvm: Fix error handling in kvmppc_xive_pre_save()

[Greg Kurz]

Now that kvmppc_xive_get_queues() returns a negative errno on failure, check
with that because it is preferred to local_err. And most of all, propagate
it because vmstate expects negative errnos.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index f2dda692183b..1686b036eb2d 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -604,16 +604,17 @@ void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
 int kvmppc_xive_pre_save(SpaprXive *xive)
 {
     Error *local_err = NULL;
+    int ret;
 
     assert(xive->fd != -1);
 
     /* EAT: there is no extra state to query from KVM */
 
     /* ENDT */
-    kvmppc_xive_get_queues(xive, &local_err);
-    if (local_err) {
+    ret = kvmppc_xive_get_queues(xive, &local_err);
+    if (ret < 0) {
         error_report_err(local_err);
-        return -1;
+        return ret;
     }
 
     return 0;

[PATCH 10/14] spapr/xive: Fix error handling in kvmppc_xive_post_load()

[Greg Kurz]

Now that all these functions return a negative errno on failure, check
that because it is preferred to local_err. And most of all, propagate it
because vmstate expects negative errnos.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 1686b036eb2d..005729ebffed 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -631,6 +631,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
     Error *local_err = NULL;
     CPUState *cs;
     int i;
+    int ret;
 
     /* The KVM XIVE device should be in use */
     assert(xive->fd != -1);
@@ -641,11 +642,10 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
             continue;
         }
 
-        kvmppc_xive_set_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
-                                     &xive->endt[i], &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            return -1;
+        ret = kvmppc_xive_set_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
+                                           &xive->endt[i], &local_err);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
@@ -660,16 +660,14 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
          * previously set in KVM. Since we don't do that for all interrupts
          * at reset time anymore, let's do it now.
          */
-        kvmppc_xive_source_reset_one(&xive->source, i, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            return -1;
+        ret = kvmppc_xive_source_reset_one(&xive->source, i, &local_err);
+        if (ret < 0) {
+            goto fail;
         }
 
-        kvmppc_xive_set_source_config(xive, i, &xive->eat[i], &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            return -1;
+        ret = kvmppc_xive_set_source_config(xive, i, &xive->eat[i], &local_err);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
@@ -686,15 +684,18 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        kvmppc_xive_cpu_set_state(spapr_cpu_state(cpu)->tctx, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            return -1;
+        ret = kvmppc_xive_cpu_set_state(spapr_cpu_state(cpu)->tctx, &local_err);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
     /* The source states will be restored when the machine starts running */
     return 0;
+
+fail:
+    error_report_err(local_err);
+    return ret;
 }
 
 /* Returns MAP_FAILED on error and sets errno */

[PATCH 11/14] ppc/xive: Fix error handling in vmstate_xive_tctx_*() callbacks

[Greg Kurz]

Now that kvmppc_xive_cpu_get_state() and kvmppc_xive_cpu_set_state()
return negative errnos on failures, use that instead local_err because
it is the recommended practice. Also return that instead of -1 since
vmstate expects negative errnos.

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

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index a453e8f4dcbe..17ca5a1916b4 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -695,12 +695,13 @@ static int vmstate_xive_tctx_pre_save(void *opaque)
 {
     XiveTCTX *tctx = XIVE_TCTX(opaque);
     Error *local_err = NULL;
+    int ret;
 
     if (xive_in_kernel(tctx->xptr)) {
-        kvmppc_xive_cpu_get_state(tctx, &local_err);
-        if (local_err) {
+        ret = kvmppc_xive_cpu_get_state(tctx, &local_err);
+        if (ret < 0) {
             error_report_err(local_err);
-            return -1;
+            return ret;
         }
     }
 
@@ -711,16 +712,17 @@ static int vmstate_xive_tctx_post_load(void *opaque, int version_id)
 {
     XiveTCTX *tctx = XIVE_TCTX(opaque);
     Error *local_err = NULL;
+    int ret;
 
     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(tctx, &local_err);
-        if (local_err) {
+        ret = kvmppc_xive_cpu_set_state(tctx, &local_err);
+        if (ret < 0) {
             error_report_err(local_err);
-            return -1;
+            return ret;
         }
     }
 

[PATCH 12/14] spapr/xive: Simplify error handling in kvmppc_xive_connect()

[Greg Kurz]

Now that all these functions return a negative errno on failure, check
that and get rid of the local_err boilerplate.

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

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 005729ebffed..e9a36115bed6 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -723,12 +723,12 @@ 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 = xive_source_esb_len(xsrc);
     size_t tima_len = 4ull << TM_SHIFT;
     CPUState *cs;
     int fd;
     void *addr;
+    int ret;
 
     /*
      * The KVM XIVE device already in use. This is the case when
@@ -754,9 +754,10 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     /* Tell KVM about the # of VCPUs we may have */
     if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
                               KVM_DEV_XIVE_NR_SERVERS)) {
-        if (kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
-                              KVM_DEV_XIVE_NR_SERVERS, &nr_servers, true,
-                              &local_err)) {
+        ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
+                                KVM_DEV_XIVE_NR_SERVERS, &nr_servers, true,
+                                errp);
+        if (ret < 0) {
             goto fail;
         }
     }
@@ -764,8 +765,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     /*
      * 1. Source ESB pages - KVM mapping
      */
-    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
-                            &local_err);
+    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, errp);
     if (addr == MAP_FAILED) {
         goto fail;
     }
@@ -783,8 +783,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     /*
      * 3. TIMA pages - KVM mapping
      */
-    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
-                            &local_err);
+    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, errp);
     if (addr == MAP_FAILED) {
         goto fail;
     }
@@ -802,15 +801,15 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, &local_err);
-        if (local_err) {
+        ret = kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, errp);
+        if (ret < 0) {
             goto fail;
         }
     }
 
     /* Update the KVM sources */
-    kvmppc_xive_source_reset(xsrc, &local_err);
-    if (local_err) {
+    ret = kvmppc_xive_source_reset(xsrc, errp);
+    if (ret < 0) {
         goto fail;
     }
 
@@ -820,7 +819,6 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
     return 0;
 
 fail:
-    error_propagate(errp, local_err);
     kvmppc_xive_disconnect(intc);
     return -1;
 }

[PATCH 13/14] ppc/xive: Simplify error handling in xive_tctx_realize()

[Greg Kurz]

Now that kvmppc_xive_cpu_connect() returns a negative errno on failure,
use that and get rid of the local_err boilerplate.

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

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 17ca5a1916b4..489e6256ef70 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -662,7 +662,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
     XiveTCTX *tctx = XIVE_TCTX(dev);
     PowerPCCPU *cpu;
     CPUPPCState *env;
-    Error *local_err = NULL;
 
     assert(tctx->cs);
     assert(tctx->xptr);
@@ -683,9 +682,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
 
     /* Connect the presenter to the VCPU (required for CPU hotplug) */
     if (xive_in_kernel(tctx->xptr)) {
-        kvmppc_xive_cpu_connect(tctx, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (kvmppc_xive_cpu_connect(tctx, errp) < 0) {
             return;
         }
     }

[PATCH 14/14] spapr/xive: Simplify error handling of kvmppc_xive_cpu_synchronize_state()

[Greg Kurz]

Now that kvmppc_xive_cpu_get_state() returns negative on error, use that
and get rid of the temporary Error object and error_propagate().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |   14 ++++++--------
 include/hw/ppc/xive.h    |    2 +-
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index e9a36115bed6..d871bb1a0016 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -119,7 +119,8 @@ int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
 
 typedef struct {
     XiveTCTX *tctx;
-    Error *err;
+    Error **errp;
+    int ret;
 } XiveCpuGetState;
 
 static void kvmppc_xive_cpu_do_synchronize_state(CPUState *cpu,
@@ -127,14 +128,14 @@ static void kvmppc_xive_cpu_do_synchronize_state(CPUState *cpu,
 {
     XiveCpuGetState *s = arg.host_ptr;
 
-    kvmppc_xive_cpu_get_state(s->tctx, &s->err);
+    s->ret = kvmppc_xive_cpu_get_state(s->tctx, s->errp);
 }
 
-void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
+int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
 {
     XiveCpuGetState s = {
         .tctx = tctx,
-        .err = NULL,
+        .errp = errp,
     };
 
     /*
@@ -143,10 +144,7 @@ void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
     run_on_cpu(tctx->cs, kvmppc_xive_cpu_do_synchronize_state,
                RUN_ON_CPU_HOST_PTR(&s));
 
-    if (s.err) {
-        error_propagate(errp, s.err);
-        return;
-    }
+    return s.ret;
 }
 
 int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 785c905357dc..2c42ae92d287 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -488,7 +488,7 @@ void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb);
 int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
 void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
 int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
-void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
+int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
 int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
 int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
 

Re: [PATCH 01/14] spapr: Simplify error handling in spapr_phb_realize()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:53:58PM +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>

Applied to ppc-for-5.2.

> ---
>  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 02/14] spapr/xive: Rework error handling of kvmppc_xive_cpu_connect()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:05PM +0200, Greg Kurz wrote:
> Use error_setg_errno() instead of error_setg(strerror()). While here,
> use -ret instead of errno since kvm_vcpu_enable_cap() returns a negative
> errno on failure.
> 
> Use ERRP_GUARD() to ensure that errp can be passed to error_append_hint(),
> and get rid of the local_err boilerplate.
> 
> Propagate the return value so that callers may use it as well to check
> failures.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   21 ++++++++++-----------
>  include/hw/ppc/xive.h    |    2 +-
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 82a6f99f022d..aa1a2f915363 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -144,8 +144,9 @@ void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
>      }
>  }
>  
> -void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
> +int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>  {
> +    ERRP_GUARD();
>      SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
>      unsigned long vcpu_id;
>      int ret;
> @@ -154,7 +155,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>  
>      /* Check if CPU was hot unplugged and replugged. */
>      if (kvm_cpu_is_enabled(tctx->cs)) {
> -        return;
> +        return 0;
>      }
>  
>      vcpu_id = kvm_arch_vcpu_id(tctx->cs);
> @@ -162,20 +163,18 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>      ret = kvm_vcpu_enable_cap(tctx->cs, KVM_CAP_PPC_IRQ_XIVE, 0, xive->fd,
>                                vcpu_id, 0);
>      if (ret < 0) {
> -        Error *local_err = NULL;
> -
> -        error_setg(&local_err,
> -                   "XIVE: unable to connect CPU%ld to KVM device: %s",
> -                   vcpu_id, strerror(errno));
> -        if (errno == ENOSPC) {
> -            error_append_hint(&local_err, "Try -smp maxcpus=N with N < %u\n",
> +        error_setg_errno(errp, -ret,
> +                         "XIVE: unable to connect CPU%ld to KVM device",
> +                         vcpu_id);
> +        if (ret == -ENOSPC) {
> +            error_append_hint(errp, "Try -smp maxcpus=N with N < %u\n",
>                                MACHINE(qdev_get_machine())->smp.max_cpus);
>          }
> -        error_propagate(errp, local_err);
> -        return;
> +        return ret;
>      }
>  
>      kvm_cpu_enable(tctx->cs);
> +    return 0;
>  }
>  
>  /*
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 2f3c5af810bb..2d87ed43728a 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -487,7 +487,7 @@ void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb);
>  
>  int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
> -void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
> +int 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);
> 
> 

-- 
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 03/14] spapr/xive: Rework error handling of kvmppc_xive_source_reset()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:12PM +0200, Greg Kurz wrote:
> Since kvmppc_xive_source_reset_one() has a return value, convert
> kvmppc_xive_source_reset() to use it for error checking. This
> allows to get rid of the local_err boiler plate.
> 
> Propagate the return value so that callers may use it as well to check
> failures.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index aa1a2f915363..d801bf5cd11c 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -248,24 +248,25 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
>                               true, errp);
>  }
>  
> -static void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
> +static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>      int i;
>  
>      for (i = 0; i < xsrc->nr_irqs; i++) {
> -        Error *local_err = NULL;
> +        int ret;
>  
>          if (!xive_eas_is_valid(&xive->eat[i])) {
>              continue;
>          }
>  
> -        kvmppc_xive_source_reset_one(xsrc, i, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +        ret = kvmppc_xive_source_reset_one(xsrc, i, errp);
> +        if (ret < 0) {
> +            return ret;
>          }
>      }
> +
> +    return 0;
>  }
>  
>  /*
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 04/14] spapr/xive: Rework error handling of kvmppc_xive_mmap()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:19PM +0200, Greg Kurz wrote:
> Callers currently check failures of kvmppc_xive_mmap() through the
> @errp argument, which isn't a recommanded practice. It is preferred
> to use a return value when possible.
> 
> Since NULL isn't an invalid address in theory, it seems better to
> return MAP_FAILED and to teach callers to handle it.

Heh... there's a world of C and Unix subtleties buried in that
assertion.  But, given that it's a function named ..._mmap() using
MAP_FAILED isn't a bad option.

Applied to ppc-for-5.2.

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/spapr_xive_kvm.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index d801bf5cd11c..b2a36fd59dae 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -698,6 +698,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>      return 0;
>  }
>  
> +/* Returns MAP_FAILED on error and sets errno */
>  static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
>                                Error **errp)
>  {
> @@ -708,7 +709,6 @@ static void *kvmppc_xive_mmap(SpaprXive *xive, int pgoff, size_t len,
>                  pgoff << page_shift);
>      if (addr == MAP_FAILED) {
>          error_setg_errno(errp, errno, "XIVE: unable to set memory mapping");
> -        return NULL;
>      }
>  
>      return addr;
> @@ -728,6 +728,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      size_t tima_len = 4ull << TM_SHIFT;
>      CPUState *cs;
>      int fd;
> +    void *addr;
>  
>      /*
>       * The KVM XIVE device already in use. This is the case when
> @@ -763,11 +764,12 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      /*
>       * 1. Source ESB pages - KVM mapping
>       */
> -    xsrc->esb_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
> -                                      &local_err);
> -    if (local_err) {
> +    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
> +                            &local_err);
> +    if (addr == MAP_FAILED) {
>          goto fail;
>      }
> +    xsrc->esb_mmap = addr;
>  
>      memory_region_init_ram_device_ptr(&xsrc->esb_mmio_kvm, OBJECT(xsrc),
>                                        "xive.esb-kvm", esb_len, xsrc->esb_mmap);
> @@ -781,11 +783,13 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      /*
>       * 3. TIMA pages - KVM mapping
>       */
> -    xive->tm_mmap = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
> -                                     &local_err);
> -    if (local_err) {
> +    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
> +                            &local_err);
> +    if (addr == MAP_FAILED) {
>          goto fail;
>      }
> +    xive->tm_mmap = addr;
> +
>      memory_region_init_ram_device_ptr(&xive->tm_mmio_kvm, OBJECT(xive),
>                                        "xive.tima", tima_len, xive->tm_mmap);
>      memory_region_add_subregion_overlap(&xive->tm_mmio, 0,
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 05/14] spapr/xive: Rework error handling of kvmppc_xive_cpu_[gs]et_state()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:26PM +0200, Greg Kurz wrote:
> kvm_set_one_reg() returns a negative errno on failure, use that instead
> of errno. Also propagate it to callers so they can use it to check
> for failures and hopefully get rid of their local_err boilerplate.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   15 ++++++++++-----
>  include/hw/ppc/xive.h    |    4 ++--
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index b2a36fd59dae..5e088ccbf885 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -73,7 +73,7 @@ static void kvm_cpu_disable_all(void)
>   * XIVE Thread Interrupt Management context (KVM)
>   */
>  
> -void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
> +int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
>      uint64_t state[2];
> @@ -86,13 +86,16 @@ void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp)
>  
>      ret = kvm_set_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
>      if (ret != 0) {
> -        error_setg_errno(errp, errno,
> +        error_setg_errno(errp, -ret,
>                           "XIVE: could not restore KVM state of CPU %ld",
>                           kvm_arch_vcpu_id(tctx->cs));
> +        return ret;
>      }
> +
> +    return 0;
>  }
>  
> -void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
> +int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(tctx->xptr);
>      uint64_t state[2] = { 0 };
> @@ -102,14 +105,16 @@ void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
>  
>      ret = kvm_get_one_reg(tctx->cs, KVM_REG_PPC_VP_STATE, state);
>      if (ret != 0) {
> -        error_setg_errno(errp, errno,
> +        error_setg_errno(errp, -ret,
>                           "XIVE: could not capture KVM state of CPU %ld",
>                           kvm_arch_vcpu_id(tctx->cs));
> -        return;
> +        return ret;
>      }
>  
>      /* word0 and word1 of the OS ring. */
>      *((uint64_t *) &tctx->regs[TM_QW1_OS]) = state[0];
> +
> +    return 0;
>  }
>  
>  typedef struct {
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 2d87ed43728a..785c905357dc 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -489,7 +489,7 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
>  int 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);
> +int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
> +int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
>  
>  #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 06/14] spapr/xive: Rework error handling of kvmppc_xive_[gs]et_queue_config()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:33PM +0200, Greg Kurz wrote:
> Since kvm_device_access() returns a negative errno on failure, convert
> kvmppc_xive_get_queue_config() and kvmppc_xive_set_queue_config() to
> use it for error checking. This allows to get rid of the local_err
> boilerplate.
> 
> Propagate the return value so that callers may use it as well to check
> failures.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c    |   35 ++++++++++++++++-------------------
>  include/hw/ppc/spapr_xive.h |    4 ++--
>  2 files changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 5e088ccbf885..696623f717b7 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -374,15 +374,15 @@ void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
>  /*
>   * sPAPR XIVE interrupt controller (KVM)
>   */
> -void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
> -                                  uint32_t end_idx, XiveEND *end,
> -                                  Error **errp)
> +int kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
> +                                 uint32_t end_idx, XiveEND *end,
> +                                 Error **errp)
>  {
>      struct kvm_ppc_xive_eq kvm_eq = { 0 };
>      uint64_t kvm_eq_idx;
>      uint8_t priority;
>      uint32_t server;
> -    Error *local_err = NULL;
> +    int ret;
>  
>      assert(xive_end_is_valid(end));
>  
> @@ -394,11 +394,10 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
>      kvm_eq_idx |= server << KVM_XIVE_EQ_SERVER_SHIFT &
>          KVM_XIVE_EQ_SERVER_MASK;
>  
> -    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> -                      &kvm_eq, false, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> +    ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> +                            &kvm_eq, false, errp);
> +    if (ret < 0) {
> +        return ret;
>      }
>  
>      /*
> @@ -408,17 +407,18 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
>       */
>      end->w1 = xive_set_field32(END_W1_GENERATION, 0ul, kvm_eq.qtoggle) |
>          xive_set_field32(END_W1_PAGE_OFF, 0ul, kvm_eq.qindex);
> +
> +    return 0;
>  }
>  
> -void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
> -                                  uint32_t end_idx, XiveEND *end,
> -                                  Error **errp)
> +int kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
> +                                 uint32_t end_idx, XiveEND *end,
> +                                 Error **errp)
>  {
>      struct kvm_ppc_xive_eq kvm_eq = { 0 };
>      uint64_t kvm_eq_idx;
>      uint8_t priority;
>      uint32_t server;
> -    Error *local_err = NULL;
>  
>      /*
>       * Build the KVM state from the local END structure.
> @@ -456,12 +456,9 @@ void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
>      kvm_eq_idx |= server << KVM_XIVE_EQ_SERVER_SHIFT &
>          KVM_XIVE_EQ_SERVER_MASK;
>  
> -    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> -                      &kvm_eq, true, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    return
> +        kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_EQ_CONFIG, kvm_eq_idx,
> +                          &kvm_eq, true, errp);
>  }
>  
>  void kvmppc_xive_reset(SpaprXive *xive, Error **errp)
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 93d09d68deb7..d0a08b618f79 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -85,10 +85,10 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
>  uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
>                              uint64_t data, bool write);
> -void kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
> +int kvmppc_xive_set_queue_config(SpaprXive *xive, uint8_t end_blk,
>                                   uint32_t end_idx, XiveEND *end,
>                                   Error **errp);
> -void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
> +int kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk,
>                                   uint32_t end_idx, XiveEND *end,
>                                   Error **errp);
>  void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp);
> 
> 

-- 
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 07/14] spapr/xive: Rework error handling in kvmppc_xive_get_queues()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:40PM +0200, Greg Kurz wrote:
> Since kvmppc_xive_get_queue_config() has a return value, convert
> kvmppc_xive_get_queues() to use it for error checking. This allows
> to get rid of the local_err boiler plate.
> 
> Propagate the return value so that callers may use it as well to check
> failures.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 696623f717b7..4142aaffff47 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -467,23 +467,24 @@ void kvmppc_xive_reset(SpaprXive *xive, Error **errp)
>                        NULL, true, errp);
>  }
>  
> -static void kvmppc_xive_get_queues(SpaprXive *xive, Error **errp)
> +static int kvmppc_xive_get_queues(SpaprXive *xive, Error **errp)
>  {
> -    Error *local_err = NULL;
>      int i;
> +    int ret;
>  
>      for (i = 0; i < xive->nr_ends; i++) {
>          if (!xive_end_is_valid(&xive->endt[i])) {
>              continue;
>          }
>  
> -        kvmppc_xive_get_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
> -                                     &xive->endt[i], &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +        ret = kvmppc_xive_get_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
> +                                           &xive->endt[i], errp);
> +        if (ret < 0) {
> +            return ret;
>          }
>      }
> +
> +    return 0;
>  }
>  
>  /*
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 08/14] spapr/xive: Rework error handling of kvmppc_xive_set_source_config()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:47PM +0200, Greg Kurz wrote:
> Since kvm_device_access() returns a negative errno on failure, convert
> kvmppc_xive_set_source_config() to use it for error checking. This allows
> to get rid of the local_err boilerplate.
> 
> Propagate the return value so that callers may use it as well to check
> failures.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c    |   13 ++++---------
>  include/hw/ppc/spapr_xive.h |    4 ++--
>  2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 4142aaffff47..f2dda692183b 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -186,8 +186,8 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
>   * XIVE Interrupt Source (KVM)
>   */
>  
> -void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
> -                                   Error **errp)
> +int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
> +                                  Error **errp)
>  {
>      uint32_t end_idx;
>      uint32_t end_blk;
> @@ -196,7 +196,6 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>      bool masked;
>      uint32_t eisn;
>      uint64_t kvm_src;
> -    Error *local_err = NULL;
>  
>      assert(xive_eas_is_valid(eas));
>  
> @@ -216,12 +215,8 @@ void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
>      kvm_src |= ((uint64_t)eisn << KVM_XIVE_SOURCE_EISN_SHIFT) &
>          KVM_XIVE_SOURCE_EISN_MASK;
>  
> -    kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_CONFIG, lisn,
> -                      &kvm_src, true, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    return kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE_CONFIG, lisn,
> +                             &kvm_src, true, errp);
>  }
>  
>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp)
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index d0a08b618f79..0ffbe0be0280 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -80,8 +80,8 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>                          Error **errp);
>  void kvmppc_xive_disconnect(SpaprInterruptController *intc);
>  void kvmppc_xive_reset(SpaprXive *xive, Error **errp);
> -void kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
> -                                   Error **errp);
> +int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas,
> +                                  Error **errp);
>  void kvmppc_xive_sync_source(SpaprXive *xive, uint32_t lisn, Error **errp);
>  uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
>                              uint64_t data, bool write);
> 
> 

-- 
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 09/14] spapr/kvm: Fix error handling in kvmppc_xive_pre_save()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:54:54PM +0200, Greg Kurz wrote:
> Now that kvmppc_xive_get_queues() returns a negative errno on failure, check
> with that because it is preferred to local_err. And most of all, propagate
> it because vmstate expects negative errnos.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Fixed a conflict and applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index f2dda692183b..1686b036eb2d 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -604,16 +604,17 @@ void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp)
>  int kvmppc_xive_pre_save(SpaprXive *xive)
>  {
>      Error *local_err = NULL;
> +    int ret;
>  
>      assert(xive->fd != -1);
>  
>      /* EAT: there is no extra state to query from KVM */
>  
>      /* ENDT */
> -    kvmppc_xive_get_queues(xive, &local_err);
> -    if (local_err) {
> +    ret = kvmppc_xive_get_queues(xive, &local_err);
> +    if (ret < 0) {
>          error_report_err(local_err);
> -        return -1;
> +        return ret;
>      }
>  
>      return 0;
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 10/14] spapr/xive: Fix error handling in kvmppc_xive_post_load()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:55:01PM +0200, Greg Kurz wrote:
> Now that all these functions return a negative errno on failure, check
> that because it is preferred to local_err. And most of all, propagate it
> because vmstate expects negative errnos.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 1686b036eb2d..005729ebffed 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -631,6 +631,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>      Error *local_err = NULL;
>      CPUState *cs;
>      int i;
> +    int ret;
>  
>      /* The KVM XIVE device should be in use */
>      assert(xive->fd != -1);
> @@ -641,11 +642,10 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>              continue;
>          }
>  
> -        kvmppc_xive_set_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
> -                                     &xive->endt[i], &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -            return -1;
> +        ret = kvmppc_xive_set_queue_config(xive, SPAPR_XIVE_BLOCK_ID, i,
> +                                           &xive->endt[i], &local_err);
> +        if (ret < 0) {
> +            goto fail;
>          }
>      }
>  
> @@ -660,16 +660,14 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>           * previously set in KVM. Since we don't do that for all interrupts
>           * at reset time anymore, let's do it now.
>           */
> -        kvmppc_xive_source_reset_one(&xive->source, i, &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -            return -1;
> +        ret = kvmppc_xive_source_reset_one(&xive->source, i, &local_err);
> +        if (ret < 0) {
> +            goto fail;
>          }
>  
> -        kvmppc_xive_set_source_config(xive, i, &xive->eat[i], &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -            return -1;
> +        ret = kvmppc_xive_set_source_config(xive, i, &xive->eat[i], &local_err);
> +        if (ret < 0) {
> +            goto fail;
>          }
>      }
>  
> @@ -686,15 +684,18 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        kvmppc_xive_cpu_set_state(spapr_cpu_state(cpu)->tctx, &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -            return -1;
> +        ret = kvmppc_xive_cpu_set_state(spapr_cpu_state(cpu)->tctx, &local_err);
> +        if (ret < 0) {
> +            goto fail;
>          }
>      }
>  
>      /* The source states will be restored when the machine starts running */
>      return 0;
> +
> +fail:
> +    error_report_err(local_err);
> +    return ret;
>  }
>  
>  /* Returns MAP_FAILED on error and sets errno */
> 
> 

-- 
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 11/14] ppc/xive: Fix error handling in vmstate_xive_tctx_*() callbacks

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:55:08PM +0200, Greg Kurz wrote:
> Now that kvmppc_xive_cpu_get_state() and kvmppc_xive_cpu_set_state()
> return negative errnos on failures, use that instead local_err because
> it is the recommended practice. Also return that instead of -1 since
> vmstate expects negative errnos.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2

> ---
>  hw/intc/xive.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index a453e8f4dcbe..17ca5a1916b4 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -695,12 +695,13 @@ static int vmstate_xive_tctx_pre_save(void *opaque)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
> +    int ret;
>  
>      if (xive_in_kernel(tctx->xptr)) {
> -        kvmppc_xive_cpu_get_state(tctx, &local_err);
> -        if (local_err) {
> +        ret = kvmppc_xive_cpu_get_state(tctx, &local_err);
> +        if (ret < 0) {
>              error_report_err(local_err);
> -            return -1;
> +            return ret;
>          }
>      }
>  
> @@ -711,16 +712,17 @@ static int vmstate_xive_tctx_post_load(void *opaque, int version_id)
>  {
>      XiveTCTX *tctx = XIVE_TCTX(opaque);
>      Error *local_err = NULL;
> +    int ret;
>  
>      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(tctx, &local_err);
> -        if (local_err) {
> +        ret = kvmppc_xive_cpu_set_state(tctx, &local_err);
> +        if (ret < 0) {
>              error_report_err(local_err);
> -            return -1;
> +            return ret;
>          }
>      }
>  
> 
> 

-- 
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 12/14] spapr/xive: Simplify error handling in kvmppc_xive_connect()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:55:15PM +0200, Greg Kurz wrote:
> Now that all these functions return a negative errno on failure, check
> that and get rid of the local_err boilerplate.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 005729ebffed..e9a36115bed6 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -723,12 +723,12 @@ 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 = xive_source_esb_len(xsrc);
>      size_t tima_len = 4ull << TM_SHIFT;
>      CPUState *cs;
>      int fd;
>      void *addr;
> +    int ret;
>  
>      /*
>       * The KVM XIVE device already in use. This is the case when
> @@ -754,9 +754,10 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      /* Tell KVM about the # of VCPUs we may have */
>      if (kvm_device_check_attr(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
>                                KVM_DEV_XIVE_NR_SERVERS)) {
> -        if (kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> -                              KVM_DEV_XIVE_NR_SERVERS, &nr_servers, true,
> -                              &local_err)) {
> +        ret = kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_CTRL,
> +                                KVM_DEV_XIVE_NR_SERVERS, &nr_servers, true,
> +                                errp);
> +        if (ret < 0) {
>              goto fail;
>          }
>      }
> @@ -764,8 +765,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      /*
>       * 1. Source ESB pages - KVM mapping
>       */
> -    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len,
> -                            &local_err);
> +    addr = kvmppc_xive_mmap(xive, KVM_XIVE_ESB_PAGE_OFFSET, esb_len, errp);
>      if (addr == MAP_FAILED) {
>          goto fail;
>      }
> @@ -783,8 +783,7 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      /*
>       * 3. TIMA pages - KVM mapping
>       */
> -    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len,
> -                            &local_err);
> +    addr = kvmppc_xive_mmap(xive, KVM_XIVE_TIMA_PAGE_OFFSET, tima_len, errp);
>      if (addr == MAP_FAILED) {
>          goto fail;
>      }
> @@ -802,15 +801,15 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, &local_err);
> -        if (local_err) {
> +        ret = kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, errp);
> +        if (ret < 0) {
>              goto fail;
>          }
>      }
>  
>      /* Update the KVM sources */
> -    kvmppc_xive_source_reset(xsrc, &local_err);
> -    if (local_err) {
> +    ret = kvmppc_xive_source_reset(xsrc, errp);
> +    if (ret < 0) {
>          goto fail;
>      }
>  
> @@ -820,7 +819,6 @@ int kvmppc_xive_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>      return 0;
>  
>  fail:
> -    error_propagate(errp, local_err);
>      kvmppc_xive_disconnect(intc);
>      return -1;
>  }
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 13/14] ppc/xive: Simplify error handling in xive_tctx_realize()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:55:22PM +0200, Greg Kurz wrote:
> Now that kvmppc_xive_cpu_connect() returns a negative errno on failure,
> use that and get rid of the local_err boilerplate.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2

> ---
>  hw/intc/xive.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 17ca5a1916b4..489e6256ef70 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -662,7 +662,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>      XiveTCTX *tctx = XIVE_TCTX(dev);
>      PowerPCCPU *cpu;
>      CPUPPCState *env;
> -    Error *local_err = NULL;
>  
>      assert(tctx->cs);
>      assert(tctx->xptr);
> @@ -683,9 +682,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>  
>      /* Connect the presenter to the VCPU (required for CPU hotplug) */
>      if (xive_in_kernel(tctx->xptr)) {
> -        kvmppc_xive_cpu_connect(tctx, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        if (kvmppc_xive_cpu_connect(tctx, errp) < 0) {
>              return;
>          }
>      }
> 
> 

-- 
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 14/14] spapr/xive: Simplify error handling of kvmppc_xive_cpu_synchronize_state()

[David Gibson]

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

On Mon, Aug 10, 2020 at 06:55:29PM +0200, Greg Kurz wrote:
> Now that kvmppc_xive_cpu_get_state() returns negative on error, use that
> and get rid of the temporary Error object and error_propagate().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.2.

> ---
>  hw/intc/spapr_xive_kvm.c |   14 ++++++--------
>  include/hw/ppc/xive.h    |    2 +-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index e9a36115bed6..d871bb1a0016 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -119,7 +119,8 @@ int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp)
>  
>  typedef struct {
>      XiveTCTX *tctx;
> -    Error *err;
> +    Error **errp;
> +    int ret;
>  } XiveCpuGetState;
>  
>  static void kvmppc_xive_cpu_do_synchronize_state(CPUState *cpu,
> @@ -127,14 +128,14 @@ static void kvmppc_xive_cpu_do_synchronize_state(CPUState *cpu,
>  {
>      XiveCpuGetState *s = arg.host_ptr;
>  
> -    kvmppc_xive_cpu_get_state(s->tctx, &s->err);
> +    s->ret = kvmppc_xive_cpu_get_state(s->tctx, s->errp);
>  }
>  
> -void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
> +int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
>  {
>      XiveCpuGetState s = {
>          .tctx = tctx,
> -        .err = NULL,
> +        .errp = errp,
>      };
>  
>      /*
> @@ -143,10 +144,7 @@ void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp)
>      run_on_cpu(tctx->cs, kvmppc_xive_cpu_do_synchronize_state,
>                 RUN_ON_CPU_HOST_PTR(&s));
>  
> -    if (s.err) {
> -        error_propagate(errp, s.err);
> -        return;
> -    }
> +    return s.ret;
>  }
>  
>  int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp)
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 785c905357dc..2c42ae92d287 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -488,7 +488,7 @@ void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb);
>  int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
>  int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
> -void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
> +int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
>  int kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp);
>  int kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp);
>  
> 
> 

-- 
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 01/14] spapr: Simplify error handling in spapr_phb_realize()

[Daniel Henrique Barboza]

Greg,

This patch is breaking guest startup in ppc-for-5.2 for me. The process
gives an almost instant segfault. Here's what I'm doing:

$ sudo ./qemu-system-ppc64 -machine pseries-5.1,accel=kvm,usb=off,dump-guest-core=off -m 65536\
-overcommit mem-lock=off -smp 4,sockets=4,cores=1,threads=1 -rtc base=utc -display none -vga none -nographic -boot menu=on \
-device spapr-pci-host-bridge,index=1,id=pci.1 -device spapr-pci-host-bridge,index=2,id=pci.2 \
-device spapr-pci-host-bridge,index=3,id=pci.3 -device spapr-pci-host-bridge,index=4,id=pci.4 \
-device qemu-xhci,id=usb,bus=pci.0,addr=0x2 \
-drive file=/home/danielhb/f32.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
-device usb-kbd,id=input0,bus=usb.0,port=1 -device usb-mouse,id=input1,bus=usb.0,port=2 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 -msg timestamp=on \
-machine cap-ccf-assist=off
Segmentation fault
$


GDB points this backtrace:

Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
error_vprepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ", ap=0x7fffffffe290 "\030") at /home/danielhb/qemu/util/error.c:134
134         g_string_append(newmsg, (*errp)->msg);
Missing separate debuginfos, use: dnf debuginfo-install glib2-2.64.4-1.fc32.ppc64le libblkid-2.35.2-1.fc32.ppc64le libffi-3.1-24.fc32.ppc64le libgcrypt-1.8.5-3.fc32.ppc64le libgpg-error-1.36-3.fc32.ppc64le libmount-2.35.2-1.fc32.ppc64le libselinux-3.0-5.fc32.ppc64le libxml2-2.9.10-3.fc32.ppc64le ncurses-libs-6.1-15.20191109.fc32.ppc64le numactl-libs-2.0.12-4.fc32.ppc64le pcre-8.44-1.fc32.ppc64le pcre2-10.35-4.fc32.ppc64le pixman-0.40.0-1.fc32.ppc64le xz-libs-5.2.5-1.fc32.ppc64le zlib-1.2.11-21.fc32.ppc64le
(gdb) bt
#0  error_vprepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ", ap=0x7fffffffe290 "\030") at /home/danielhb/qemu/util/error.c:134
#1  0x0000000100c1e9cc in error_prepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ") at /home/danielhb/qemu/util/error.c:144
#2  0x00000001004cdad4 in spapr_phb_realize (dev=0x101d6cb90, errp=0x7fffffffe400) at /home/danielhb/qemu/hw/ppc/spapr_pci.c:1982
#3  0x0000000100735f70 in device_set_realized (obj=0x101d6cb90, value=true, errp=0x7fffffffe568) at /home/danielhb/qemu/hw/core/qdev.c:864
#4  0x0000000100a5aae4 in property_set_bool (obj=0x101d6cb90, v=0x101d6daa0, name=0x100f13df8 "realized", opaque=0x1016d2430, errp=0x7fffffffe568) at /home/danielhb/qemu/qom/object.c:2202
#5  0x0000000100a57d64 in object_property_set (obj=0x101d6cb90, name=0x100f13df8 "realized", v=0x101d6daa0, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/qom/object.c:1349
#6  0x0000000100a5cb38 in object_property_set_qobject (obj=0x101d6cb90, name=0x100f13df8 "realized", value=0x101d6c950, errp=0x1016156c0 <error_fatal>)
     at /home/danielhb/qemu/qom/qom-qobject.c:28
#7  0x0000000100a581fc in object_property_set_bool (obj=0x101d6cb90, name=0x100f13df8 "realized", value=true, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/qom/object.c:1416
#8  0x0000000100734178 in qdev_realize (dev=0x101d6cb90, bus=0x10198e250, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/qdev.c:379
#9  0x00000001007341dc in qdev_realize_and_unref (dev=0x101d6cb90, bus=0x10198e250, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/qdev.c:386
#10 0x00000001007463c0 in sysbus_realize_and_unref (dev=0x101d6cb90, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/sysbus.c:260
#11 0x00000001004a6960 in spapr_create_default_phb () at /home/danielhb/qemu/hw/ppc/spapr.c:2652
#12 0x00000001004a7428 in spapr_machine_init (machine=0x101965800) at /home/danielhb/qemu/hw/ppc/spapr.c:2940
#13 0x000000010074a3b0 in machine_run_board_init (machine=0x101965800) at /home/danielhb/qemu/hw/core/machine.c:1135
#14 0x000000010054f390 in qemu_init (argc=42, argv=0x7ffffffff0a8, envp=0x7ffffffff200) at /home/danielhb/qemu/softmmu/vl.c:4355
#15 0x0000000100b8ee00 in main (argc=42, argv=0x7ffffffff0a8, envp=0x7ffffffff200) at /home/danielhb/qemu/softmmu/main.c:48
(gdb)


Removing this patch (i.e. resetting HEAD at "target/ppc: Integrate icount
to purr, vtb, and tbu40") allows me to get the guest rolling.



Thanks,


Daniel



On 8/10/20 1:53 PM, 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>
> ---
>   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 01/14] spapr: Simplify error handling in spapr_phb_realize()

[Greg Kurz]

On Thu, 13 Aug 2020 16:57:04 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> Greg,
> 
> This patch is breaking guest startup in ppc-for-5.2 for me. The process
> gives an almost instant segfault. Here's what I'm doing:
> 

Ok, this is because this patch should have been applied after
the "spapr: Cleanups for XIVE" series:

http://patchwork.ozlabs.org/project/qemu-devel/cover/159679991916.876294.8967140647442842745.stgit@bahia.lan/

specifically this patch:

http://patchwork.ozlabs.org/project/qemu-devel/patch/159679993438.876294.7285654331498605426.stgit@bahia.lan/

which prevents of a bogus error path in kvmppc_xive_source_reset_one() to
be taken:

/* The KVM XIVE device is not in use */
if (xive->fd == -1) {
    return -ENODEV; <== this should return 0 to avoid the segfault, but
                        the real issue is that kvmppc_xive_*() calls
                        shouldn't be called at all when we don't have
                        a KVM XIVE device. This is the purpose of the
                        "spapr: Cleanups for XIVE" series.
}

I should maybe have added some Based-on: tag to make it clearer...

David,

Can you apply the series the other way around ?

First :

"spapr: Cleanups for XIVE"

http://patchwork.ozlabs.org/project/qemu-devel/cover/159679991916.876294.8967140647442842745.stgit@bahia.lan/

Then :

"ppc/spapr: Error handling fixes and cleanups"

http://patchwork.ozlabs.org/project/qemu-devel/cover/159707843034.1489912.1082061742626355958.stgit@bahia.lan/

Sorry everyone for the inconvenience.

Cheers,

--
Greg

> $ sudo ./qemu-system-ppc64 -machine pseries-5.1,accel=kvm,usb=off,dump-guest-core=off -m 65536\
> -overcommit mem-lock=off -smp 4,sockets=4,cores=1,threads=1 -rtc base=utc -display none -vga none -nographic -boot menu=on \
> -device spapr-pci-host-bridge,index=1,id=pci.1 -device spapr-pci-host-bridge,index=2,id=pci.2 \
> -device spapr-pci-host-bridge,index=3,id=pci.3 -device spapr-pci-host-bridge,index=4,id=pci.4 \
> -device qemu-xhci,id=usb,bus=pci.0,addr=0x2 \
> -drive file=/home/danielhb/f32.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
> -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
> -device usb-kbd,id=input0,bus=usb.0,port=1 -device usb-mouse,id=input1,bus=usb.0,port=2 \
> -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 -msg timestamp=on \
> -machine cap-ccf-assist=off
> Segmentation fault
> $
> 
> 
> GDB points this backtrace:
> 
> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
> error_vprepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ", ap=0x7fffffffe290 "\030") at /home/danielhb/qemu/util/error.c:134
> 134         g_string_append(newmsg, (*errp)->msg);
> Missing separate debuginfos, use: dnf debuginfo-install glib2-2.64.4-1.fc32.ppc64le libblkid-2.35.2-1.fc32.ppc64le libffi-3.1-24.fc32.ppc64le libgcrypt-1.8.5-3.fc32.ppc64le libgpg-error-1.36-3.fc32.ppc64le libmount-2.35.2-1.fc32.ppc64le libselinux-3.0-5.fc32.ppc64le libxml2-2.9.10-3.fc32.ppc64le ncurses-libs-6.1-15.20191109.fc32.ppc64le numactl-libs-2.0.12-4.fc32.ppc64le pcre-8.44-1.fc32.ppc64le pcre2-10.35-4.fc32.ppc64le pixman-0.40.0-1.fc32.ppc64le xz-libs-5.2.5-1.fc32.ppc64le zlib-1.2.11-21.fc32.ppc64le
> (gdb) bt
> #0  error_vprepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ", ap=0x7fffffffe290 "\030") at /home/danielhb/qemu/util/error.c:134
> #1  0x0000000100c1e9cc in error_prepend (errp=0x7fffffffe400, fmt=0x100ec2698 "can't allocate LSIs: ") at /home/danielhb/qemu/util/error.c:144
> #2  0x00000001004cdad4 in spapr_phb_realize (dev=0x101d6cb90, errp=0x7fffffffe400) at /home/danielhb/qemu/hw/ppc/spapr_pci.c:1982
> #3  0x0000000100735f70 in device_set_realized (obj=0x101d6cb90, value=true, errp=0x7fffffffe568) at /home/danielhb/qemu/hw/core/qdev.c:864
> #4  0x0000000100a5aae4 in property_set_bool (obj=0x101d6cb90, v=0x101d6daa0, name=0x100f13df8 "realized", opaque=0x1016d2430, errp=0x7fffffffe568) at /home/danielhb/qemu/qom/object.c:2202
> #5  0x0000000100a57d64 in object_property_set (obj=0x101d6cb90, name=0x100f13df8 "realized", v=0x101d6daa0, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/qom/object.c:1349
> #6  0x0000000100a5cb38 in object_property_set_qobject (obj=0x101d6cb90, name=0x100f13df8 "realized", value=0x101d6c950, errp=0x1016156c0 <error_fatal>)
>      at /home/danielhb/qemu/qom/qom-qobject.c:28
> #7  0x0000000100a581fc in object_property_set_bool (obj=0x101d6cb90, name=0x100f13df8 "realized", value=true, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/qom/object.c:1416
> #8  0x0000000100734178 in qdev_realize (dev=0x101d6cb90, bus=0x10198e250, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/qdev.c:379
> #9  0x00000001007341dc in qdev_realize_and_unref (dev=0x101d6cb90, bus=0x10198e250, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/qdev.c:386
> #10 0x00000001007463c0 in sysbus_realize_and_unref (dev=0x101d6cb90, errp=0x1016156c0 <error_fatal>) at /home/danielhb/qemu/hw/core/sysbus.c:260
> #11 0x00000001004a6960 in spapr_create_default_phb () at /home/danielhb/qemu/hw/ppc/spapr.c:2652
> #12 0x00000001004a7428 in spapr_machine_init (machine=0x101965800) at /home/danielhb/qemu/hw/ppc/spapr.c:2940
> #13 0x000000010074a3b0 in machine_run_board_init (machine=0x101965800) at /home/danielhb/qemu/hw/core/machine.c:1135
> #14 0x000000010054f390 in qemu_init (argc=42, argv=0x7ffffffff0a8, envp=0x7ffffffff200) at /home/danielhb/qemu/softmmu/vl.c:4355
> #15 0x0000000100b8ee00 in main (argc=42, argv=0x7ffffffff0a8, envp=0x7ffffffff200) at /home/danielhb/qemu/softmmu/main.c:48
> (gdb)
> 
> 
> Removing this patch (i.e. resetting HEAD at "target/ppc: Integrate icount
> to purr, vtb, and tbu40") allows me to get the guest rolling.
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> On 8/10/20 1:53 PM, 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>
> > ---
> >   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 01/14] spapr: Simplify error handling in spapr_phb_realize()

[David Gibson]

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

On Thu, Aug 13, 2020 at 11:39:27PM +0200, Greg Kurz wrote:
> On Thu, 13 Aug 2020 16:57:04 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
> > Greg,
> > 
> > This patch is breaking guest startup in ppc-for-5.2 for me. The process
> > gives an almost instant segfault. Here's what I'm doing:
> > 
> 
> Ok, this is because this patch should have been applied after
> the "spapr: Cleanups for XIVE" series:
> 
> http://patchwork.ozlabs.org/project/qemu-devel/cover/159679991916.876294.8967140647442842745.stgit@bahia.lan/
> 
> specifically this patch:
> 
> http://patchwork.ozlabs.org/project/qemu-devel/patch/159679993438.876294.7285654331498605426.stgit@bahia.lan/
> 
> which prevents of a bogus error path in kvmppc_xive_source_reset_one() to
> be taken:

Oops, yeah,  I realized this halfway through and fixed it up, but
forgot to push out the updated version.

> 
> /* The KVM XIVE device is not in use */
> if (xive->fd == -1) {
>     return -ENODEV; <== this should return 0 to avoid the segfault, but
>                         the real issue is that kvmppc_xive_*() calls
>                         shouldn't be called at all when we don't have
>                         a KVM XIVE device. This is the purpose of the
>                         "spapr: Cleanups for XIVE" series.
> }
> 
> I should maybe have added some Based-on: tag to make it clearer...

That would have helped, yes.

> 
> David,
> 
> Can you apply the series the other way around ?
> 
> First :
> 
> "spapr: Cleanups for XIVE"
> 
> http://patchwork.ozlabs.org/project/qemu-devel/cover/159679991916.876294.8967140647442842745.stgit@bahia.lan/
> 
> Then :
> 
> "ppc/spapr: Error handling fixes and cleanups"
> 
> http://patchwork.ozlabs.org/project/qemu-devel/cover/159707843034.1489912.1082061742626355958.stgit@bahia.lan/
> 
> Sorry everyone for the inconvenience.
> 

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