[Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling

[Qemu-devel] [PATCH 0/6] xics/kvm: Improve error handling

[Greg Kurz]

In case XICS KVM setup fails, depending on the in-kernel irqchip being
required or not, QEMU used to exit or fall back to emulated XICS, and
never try to setup XICS KVM again for the machine lifetime.

This is no longer the case with the "dual" interrupt controller mode of
the spapr machine: QEMU will try again to setup KVM XICS each time the
VM is reset. This calls for proper rollback on the XICS KVM setup error
path.

That's what that series does. The first patches are preparatory cleanup.

--
Greg

---

Greg Kurz (6):
      xics/spapr: Drop unused function declaration
      xics/spapr: Rename xics_kvm_init()
      xics/kvm: Skip rollback when KVM XICS is absent
      xics/kvm: Always use local_err in xics_kvm_init()
      xics/kvm: Add error propagation to ic*_set_kvm_state() functions
      xics/kvm: Add proper rollback to xics_kvm_init()


 hw/intc/xics.c              |   39 ++++++++++++++++++++++---
 hw/intc/xics_kvm.c          |   68 +++++++++++++++++++++++++------------------
 hw/ppc/spapr_irq.c          |    2 +
 include/hw/ppc/xics.h       |    6 ++--
 include/hw/ppc/xics_spapr.h |    3 +-
 5 files changed, 78 insertions(+), 40 deletions(-)

[Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration

[Greg Kurz]

Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
has no implementation and no users.

This is a leftover from a previous iteration of this patch. Drop it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/xics_spapr.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
index d968f2499ca7..330448126223 100644
--- a/include/hw/ppc/xics_spapr.h
+++ b/include/hw/ppc/xics_spapr.h
@@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
 void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
 bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
 void xics_spapr_init(SpaprMachineState *spapr);
-void xics_spapr_connect(SpaprMachineState *spapr);
 
 #endif /* XICS_SPAPR_H */

[Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()

[Greg Kurz]

Switch to using the connect/disconnect terminology like we already do for
XIVE.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_kvm.c          |    2 +-
 hw/ppc/spapr_irq.c          |    2 +-
 include/hw/ppc/xics_spapr.h |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c7f8f5edd257..534515143ea8 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
     }
 }
 
-int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
+int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 {
     int rc;
     CPUState *cs;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 84b9b32fe40f..ff3df0bbd8cf 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
 static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
 {
     if (kvm_enabled()) {
-        xics_kvm_init(spapr, errp);
+        xics_kvm_connect(spapr, errp);
     }
 }
 
diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
index 330448126223..5dabc9a1388f 100644
--- a/include/hw/ppc/xics_spapr.h
+++ b/include/hw/ppc/xics_spapr.h
@@ -33,7 +33,7 @@
 
 void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
                    uint32_t phandle);
-int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
+int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
 void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
 bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
 void xics_spapr_init(SpaprMachineState *spapr);

[Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent

[Greg Kurz]

There is no need to rollback anything at this point, so just return an
error.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_kvm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 534515143ea8..377ff88701c2 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
     if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
         error_setg(errp,
                    "KVM and IRQ_XICS capability must be present for in-kernel XICS");
-        goto fail;
+        return -1;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");

[Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

[Greg Kurz]

Passing both errp and &local_err to functions is a recipe for messing
things up.

Since we must use &local_err for icp_kvm_realize(), use &local_err
everywhere where rollback must happen and have a single call to
error_propagate() them all. While here, add errno to the error
message.

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

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 377ff88701c2..c9e25fb051bb 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
         goto fail;
     }
 
     rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
     if (rc < 0) {
-        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
+        error_setg_errno(&local_err, -rc,
+                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
         goto fail;
     }
 
     /* Create the KVM XICS device */
     rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
     if (rc < 0) {
-        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
+        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
         goto fail;
     }
 
@@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 
         icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto fail;
         }
     }
@@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
     return 0;
 
 fail:
+    error_propagate(errp, local_err);
     kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
     kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
     kvmppc_define_rtas_kernel_token(0, "ibm,int-on");

[Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions

[Greg Kurz]

This allows errors happening there to be propagated up to spapr_irq,
just like XIVE already does.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |   39 ++++++++++++++++++++++++++++++++++-----
 hw/intc/xics_kvm.c    |   37 ++++++++++++++++++++++---------------
 include/hw/ppc/xics.h |    6 +++---
 3 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 1dc3a0f12280..69152e3d9ff6 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
     ICPState *icp = opaque;
 
     if (kvm_irqchip_in_kernel()) {
-        return icp_set_kvm_state(icp);
+        Error *local_err = NULL;
+        int ret;
+
+        ret = icp_set_kvm_state(icp, &local_err);
+        if (ret < 0) {
+            error_report_err(local_err);
+            return ret;
+        }
     }
 
     return 0;
@@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
     qemu_set_irq(icp->output, 0);
 
     if (kvm_irqchip_in_kernel()) {
-        icp_set_kvm_state(ICP(dev));
+        Error *local_err = NULL;
+
+        icp_set_kvm_state(ICP(dev), &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 }
 
@@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
     icsc->parent_reset(dev);
 
     if (kvm_irqchip_in_kernel()) {
-        ics_set_kvm_state(ICS_BASE(dev));
+        Error *local_err = NULL;
+
+        ics_set_kvm_state(ICS_BASE(dev), &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 }
 
@@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
     ICSState *ics = opaque;
 
     if (kvm_irqchip_in_kernel()) {
-        return ics_set_kvm_state(ics);
+        Error *local_err = NULL;
+        int ret;
+
+        ret = ics_set_kvm_state(ics, &local_err);
+        if (ret < 0) {
+            error_report_err(local_err);
+            return ret;
+        }
     }
 
     return 0;
@@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
         lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
 
     if (kvm_irqchip_in_kernel()) {
+        Error *local_err = NULL;
+
         ics_reset_irq(ics->irqs + srcno);
-        ics_set_kvm_state_one(ics, srcno);
+        ics_set_kvm_state_one(ics, srcno, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        }
     }
 }
 
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c9e25fb051bb..4bfbe1a84092 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
     }
 }
 
-int icp_set_kvm_state(ICPState *icp)
+int icp_set_kvm_state(ICPState *icp, Error **errp)
 {
     uint64_t state;
     int ret;
@@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
         | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
 
     ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
-    if (ret != 0) {
-        error_report("Unable to restore KVM interrupt controller state (0x%"
-                PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
-                strerror(errno));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Unable to restore KVM interrupt controller state (0x%"
+                         PRIx64 ") for CPU %ld", state,
+                         kvm_arch_vcpu_id(icp->cs));
         return ret;
     }
 
@@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
     ics_get_kvm_state(ics);
 }
 
-int ics_set_kvm_state_one(ICSState *ics, int srcno)
+int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
 {
     uint64_t state;
-    Error *local_err = NULL;
     ICSIRQState *irq = &ics->irqs[srcno];
     int ret;
 
@@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
     }
 
     ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
-                            srcno + ics->offset, &state, true, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
+                            srcno + ics->offset, &state, true, errp);
+    if (ret < 0) {
         return ret;
     }
 
     return 0;
 }
 
-int ics_set_kvm_state(ICSState *ics)
+int ics_set_kvm_state(ICSState *ics, Error **errp)
 {
     int i;
 
@@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
     }
 
     for (i = 0; i < ics->nr_irqs; i++) {
+        Error *local_err = NULL;
         int ret;
 
-        ret = ics_set_kvm_state_one(ics, i);
-        if (ret) {
+        ret = ics_set_kvm_state_one(ics, i, &local_err);
+        if (ret < 0) {
+            error_propagate(errp, local_err);
             return ret;
         }
     }
@@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
     }
 
     /* Update the KVM sources */
-    ics_set_kvm_state(spapr->ics);
+    ics_set_kvm_state(spapr->ics, &local_err);
+    if (local_err) {
+        goto fail;
+    }
 
     /* Connect the presenters to the initial VCPUs of the machine */
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
-        icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
+        icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
+        if (local_err) {
+            goto fail;
+        }
     }
 
     return 0;
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index eb65ad7e43b7..1eb7b5cd6847 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
 
 /* KVM */
 void icp_get_kvm_state(ICPState *icp);
-int icp_set_kvm_state(ICPState *icp);
+int icp_set_kvm_state(ICPState *icp, Error **errp);
 void icp_synchronize_state(ICPState *icp);
 void icp_kvm_realize(DeviceState *dev, Error **errp);
 
 void ics_get_kvm_state(ICSState *ics);
-int ics_set_kvm_state_one(ICSState *ics, int srcno);
-int ics_set_kvm_state(ICSState *ics);
+int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
+int ics_set_kvm_state(ICSState *ics, Error **errp);
 void ics_synchronize_state(ICSState *ics);
 void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
 

[Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()

[Greg Kurz]

Make xics_kvm_disconnect() able to undo the changes of a partial execution
of xics_kvm_connect() and use it to perform rollback.

Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
RTAS call has been defined or not.

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

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 4bfbe1a84092..51433b19b076 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
 
 fail:
     error_propagate(errp, local_err);
-    kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
-    kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
-    kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
-    kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
+    xics_kvm_disconnect(spapr, NULL);
     return -1;
 }
 
@@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
      * removed from the list of devices of the VM. The VCPU presenters
      * are also detached from the device.
      */
-    close(kernel_xics_fd);
-    kernel_xics_fd = -1;
+    if (kernel_xics_fd != -1) {
+        close(kernel_xics_fd);
+        kernel_xics_fd = -1;
+    }
 
     kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
     kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");

Re: [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration

[Cédric Le Goater]

On 17/06/2019 15:46, Greg Kurz wrote:
> Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
> has no implementation and no users.
> 
> This is a leftover from a previous iteration of this patch. Drop it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



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

Thanks,

C.

> ---
>  include/hw/ppc/xics_spapr.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index d968f2499ca7..330448126223 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
>  void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
>  bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
>  void xics_spapr_init(SpaprMachineState *spapr);
> -void xics_spapr_connect(SpaprMachineState *spapr);
>  
>  #endif /* XICS_SPAPR_H */
> 

Re: [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()

[Cédric Le Goater]

On 17/06/2019 15:46, Greg Kurz wrote:
> Switch to using the connect/disconnect terminology like we already do for
> XIVE.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



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

Thanks,

C.


> ---
>  hw/intc/xics_kvm.c          |    2 +-
>  hw/ppc/spapr_irq.c          |    2 +-
>  include/hw/ppc/xics_spapr.h |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c7f8f5edd257..534515143ea8 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>      }
>  }
>  
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  {
>      int rc;
>      CPUState *cs;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 84b9b32fe40f..ff3df0bbd8cf 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
>  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> -        xics_kvm_init(spapr, errp);
> +        xics_kvm_connect(spapr, errp);
>      }
>  }
>  
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 330448126223..5dabc9a1388f 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -33,7 +33,7 @@
>  
>  void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
>  void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
>  bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
>  void xics_spapr_init(SpaprMachineState *spapr);
> 

Re: [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent

[Cédric Le Goater]

On 17/06/2019 15:46, Greg Kurz wrote:
> There is no need to rollback anything at this point, so just return an
> error.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



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

Thanks,

C.

> ---
>  hw/intc/xics_kvm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 534515143ea8..377ff88701c2 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
>          error_setg(errp,
>                     "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> -        goto fail;
> +        return -1;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
> 

Re: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

[Cédric Le Goater]

On 17/06/2019 15:46, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
> 
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



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

Thanks,

C.


> ---
>  hw/intc/xics_kvm.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
>          goto fail;
>      }
>  
>      /* Create the KVM XICS device */
>      rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
>      if (rc < 0) {
> -        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>          goto fail;
>      }
>  
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>          icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto fail;
>          }
>      }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      return 0;
>  
>  fail:
> +    error_propagate(errp, local_err);
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> 

Re: [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions

[Cédric Le Goater]

On 17/06/2019 15:46, Greg Kurz wrote:
> This allows errors happening there to be propagated up to spapr_irq,
> just like XIVE already does.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


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

Thanks,

C.

> ---
>  hw/intc/xics.c        |   39 ++++++++++++++++++++++++++++++++++-----
>  hw/intc/xics_kvm.c    |   37 ++++++++++++++++++++++---------------
>  include/hw/ppc/xics.h |    6 +++---
>  3 files changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 1dc3a0f12280..69152e3d9ff6 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
>      ICPState *icp = opaque;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        return icp_set_kvm_state(icp);
> +        Error *local_err = NULL;
> +        int ret;
> +
> +        ret = icp_set_kvm_state(icp, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
>      }
>  
>      return 0;
> @@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
>      qemu_set_irq(icp->output, 0);
>  
>      if (kvm_irqchip_in_kernel()) {
> -        icp_set_kvm_state(ICP(dev));
> +        Error *local_err = NULL;
> +
> +        icp_set_kvm_state(ICP(dev), &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> @@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
>      icsc->parent_reset(dev);
>  
>      if (kvm_irqchip_in_kernel()) {
> -        ics_set_kvm_state(ICS_BASE(dev));
> +        Error *local_err = NULL;
> +
> +        ics_set_kvm_state(ICS_BASE(dev), &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> @@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
>      ICSState *ics = opaque;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        return ics_set_kvm_state(ics);
> +        Error *local_err = NULL;
> +        int ret;
> +
> +        ret = ics_set_kvm_state(ics, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
>      }
>  
>      return 0;
> @@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>          lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
>  
>      if (kvm_irqchip_in_kernel()) {
> +        Error *local_err = NULL;
> +
>          ics_reset_irq(ics->irqs + srcno);
> -        ics_set_kvm_state_one(ics, srcno);
> +        ics_set_kvm_state_one(ics, srcno, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c9e25fb051bb..4bfbe1a84092 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
>      }
>  }
>  
> -int icp_set_kvm_state(ICPState *icp)
> +int icp_set_kvm_state(ICPState *icp, Error **errp)
>  {
>      uint64_t state;
>      int ret;
> @@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
>          | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
>  
>      ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
> -    if (ret != 0) {
> -        error_report("Unable to restore KVM interrupt controller state (0x%"
> -                PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
> -                strerror(errno));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Unable to restore KVM interrupt controller state (0x%"
> +                         PRIx64 ") for CPU %ld", state,
> +                         kvm_arch_vcpu_id(icp->cs));
>          return ret;
>      }
>  
> @@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
>      ics_get_kvm_state(ics);
>  }
>  
> -int ics_set_kvm_state_one(ICSState *ics, int srcno)
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
>  {
>      uint64_t state;
> -    Error *local_err = NULL;
>      ICSIRQState *irq = &ics->irqs[srcno];
>      int ret;
>  
> @@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
>      }
>  
>      ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> -                            srcno + ics->offset, &state, true, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> +                            srcno + ics->offset, &state, true, errp);
> +    if (ret < 0) {
>          return ret;
>      }
>  
>      return 0;
>  }
>  
> -int ics_set_kvm_state(ICSState *ics)
> +int ics_set_kvm_state(ICSState *ics, Error **errp)
>  {
>      int i;
>  
> @@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
>      }
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
> +        Error *local_err = NULL;
>          int ret;
>  
> -        ret = ics_set_kvm_state_one(ics, i);
> -        if (ret) {
> +        ret = ics_set_kvm_state_one(ics, i, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
>              return ret;
>          }
>      }
> @@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      }
>  
>      /* Update the KVM sources */
> -    ics_set_kvm_state(spapr->ics);
> +    ics_set_kvm_state(spapr->ics, &local_err);
> +    if (local_err) {
> +        goto fail;
> +    }
>  
>      /* Connect the presenters to the initial VCPUs of the machine */
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
> +        icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
> +        if (local_err) {
> +            goto fail;
> +        }
>      }
>  
>      return 0;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index eb65ad7e43b7..1eb7b5cd6847 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>  
>  /* KVM */
>  void icp_get_kvm_state(ICPState *icp);
> -int icp_set_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp, Error **errp);
>  void icp_synchronize_state(ICPState *icp);
>  void icp_kvm_realize(DeviceState *dev, Error **errp);
>  
>  void ics_get_kvm_state(ICSState *ics);
> -int ics_set_kvm_state_one(ICSState *ics, int srcno);
> -int ics_set_kvm_state(ICSState *ics);
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
> +int ics_set_kvm_state(ICSState *ics, Error **errp);
>  void ics_synchronize_state(ICSState *ics);
>  void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>  
> 

Re: [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()

[Cédric Le Goater]

On 17/06/2019 15:47, Greg Kurz wrote:
> Make xics_kvm_disconnect() able to undo the changes of a partial execution
> of xics_kvm_connect() and use it to perform rollback.
> 
> Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
> RTAS call has been defined or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/xics_kvm.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4bfbe1a84092..51433b19b076 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>  fail:
>      error_propagate(errp, local_err);
> -    kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
> +    xics_kvm_disconnect(spapr, NULL);
>      return -1;
>  }
>  
> @@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
>       * removed from the list of devices of the VM. The VCPU presenters
>       * are also detached from the device.
>       */
> -    close(kernel_xics_fd);
> -    kernel_xics_fd = -1;
> +    if (kernel_xics_fd != -1) {
> +        close(kernel_xics_fd);
> +        kernel_xics_fd = -1;
> +    }
>  
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> 

Re: [Qemu-devel] [PATCH 1/6] xics/spapr: Drop unused function declaration

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:46:35PM +0200, Greg Kurz wrote:
> Commit 9fb6eb7ca50c added the declaration of xics_spapr_connect(), which
> has no implementation and no users.
> 
> This is a leftover from a previous iteration of this patch. Drop it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  include/hw/ppc/xics_spapr.h |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index d968f2499ca7..330448126223 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -37,6 +37,5 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
>  void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
>  bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
>  void xics_spapr_init(SpaprMachineState *spapr);
> -void xics_spapr_connect(SpaprMachineState *spapr);
>  
>  #endif /* XICS_SPAPR_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: [Qemu-devel] [PATCH 2/6] xics/spapr: Rename xics_kvm_init()

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:46:41PM +0200, Greg Kurz wrote:
> Switch to using the connect/disconnect terminology like we already do for
> XIVE.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c          |    2 +-
>  hw/ppc/spapr_irq.c          |    2 +-
>  include/hw/ppc/xics_spapr.h |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c7f8f5edd257..534515143ea8 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -331,7 +331,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>      }
>  }
>  
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  {
>      int rc;
>      CPUState *cs;
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 84b9b32fe40f..ff3df0bbd8cf 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -237,7 +237,7 @@ static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
>  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> -        xics_kvm_init(spapr, errp);
> +        xics_kvm_connect(spapr, errp);
>      }
>  }
>  
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 330448126223..5dabc9a1388f 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -33,7 +33,7 @@
>  
>  void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> -int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
> +int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
>  void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
>  bool xics_kvm_has_broken_disconnect(SpaprMachineState *spapr);
>  void xics_spapr_init(SpaprMachineState *spapr);
> 

-- 
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: [Qemu-devel] [PATCH 3/6] xics/kvm: Skip rollback when KVM XICS is absent

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:46:46PM +0200, Greg Kurz wrote:
> There is no need to rollback anything at this point, so just return an
> error.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 534515143ea8..377ff88701c2 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -348,7 +348,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
>          error_setg(errp,
>                     "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> -        goto fail;
> +        return -1;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
> 

-- 
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: [Qemu-devel] [PATCH 4/6] xics/kvm: Always use local_err in xics_kvm_init()

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:46:52PM +0200, Greg Kurz wrote:
> Passing both errp and &local_err to functions is a recipe for messing
> things up.
> 
> Since we must use &local_err for icp_kvm_realize(), use &local_err
> everywhere where rollback must happen and have a single call to
> error_propagate() them all. While here, add errno to the error
> message.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 377ff88701c2..c9e25fb051bb 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -353,32 +353,36 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,set-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,get-xive");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-on");
>          goto fail;
>      }
>  
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off");
>      if (rc < 0) {
> -        error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off");
> +        error_setg_errno(&local_err, -rc,
> +                         "kvmppc_define_rtas_kernel_token: ibm,int-off");
>          goto fail;
>      }
>  
>      /* Create the KVM XICS device */
>      rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
>      if (rc < 0) {
> -        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        error_setg_errno(&local_err, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>          goto fail;
>      }
>  
> @@ -393,7 +397,6 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>          icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto fail;
>          }
>      }
> @@ -410,6 +413,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      return 0;
>  
>  fail:
> +    error_propagate(errp, local_err);
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> 

-- 
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: [Qemu-devel] [PATCH 5/6] xics/kvm: Add error propagation to ic*_set_kvm_state() functions

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:46:57PM +0200, Greg Kurz wrote:
> This allows errors happening there to be propagated up to spapr_irq,
> just like XIVE already does.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics.c        |   39 ++++++++++++++++++++++++++++++++++-----
>  hw/intc/xics_kvm.c    |   37 ++++++++++++++++++++++---------------
>  include/hw/ppc/xics.h |    6 +++---
>  3 files changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 1dc3a0f12280..69152e3d9ff6 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -267,7 +267,14 @@ static int icp_post_load(void *opaque, int version_id)
>      ICPState *icp = opaque;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        return icp_set_kvm_state(icp);
> +        Error *local_err = NULL;
> +        int ret;
> +
> +        ret = icp_set_kvm_state(icp, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
>      }
>  
>      return 0;
> @@ -300,7 +307,12 @@ static void icp_reset_handler(void *dev)
>      qemu_set_irq(icp->output, 0);
>  
>      if (kvm_irqchip_in_kernel()) {
> -        icp_set_kvm_state(ICP(dev));
> +        Error *local_err = NULL;
> +
> +        icp_set_kvm_state(ICP(dev), &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> @@ -555,7 +567,12 @@ static void ics_simple_reset(DeviceState *dev)
>      icsc->parent_reset(dev);
>  
>      if (kvm_irqchip_in_kernel()) {
> -        ics_set_kvm_state(ICS_BASE(dev));
> +        Error *local_err = NULL;
> +
> +        ics_set_kvm_state(ICS_BASE(dev), &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> @@ -671,7 +688,14 @@ static int ics_base_post_load(void *opaque, int version_id)
>      ICSState *ics = opaque;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        return ics_set_kvm_state(ics);
> +        Error *local_err = NULL;
> +        int ret;
> +
> +        ret = ics_set_kvm_state(ics, &local_err);
> +        if (ret < 0) {
> +            error_report_err(local_err);
> +            return ret;
> +        }
>      }
>  
>      return 0;
> @@ -757,8 +781,13 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>          lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
>  
>      if (kvm_irqchip_in_kernel()) {
> +        Error *local_err = NULL;
> +
>          ics_reset_irq(ics->irqs + srcno);
> -        ics_set_kvm_state_one(ics, srcno);
> +        ics_set_kvm_state_one(ics, srcno, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
>      }
>  }
>  
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index c9e25fb051bb..4bfbe1a84092 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -106,7 +106,7 @@ void icp_synchronize_state(ICPState *icp)
>      }
>  }
>  
> -int icp_set_kvm_state(ICPState *icp)
> +int icp_set_kvm_state(ICPState *icp, Error **errp)
>  {
>      uint64_t state;
>      int ret;
> @@ -126,10 +126,11 @@ int icp_set_kvm_state(ICPState *icp)
>          | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
>  
>      ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
> -    if (ret != 0) {
> -        error_report("Unable to restore KVM interrupt controller state (0x%"
> -                PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(icp->cs),
> -                strerror(errno));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Unable to restore KVM interrupt controller state (0x%"
> +                         PRIx64 ") for CPU %ld", state,
> +                         kvm_arch_vcpu_id(icp->cs));
>          return ret;
>      }
>  
> @@ -240,10 +241,9 @@ void ics_synchronize_state(ICSState *ics)
>      ics_get_kvm_state(ics);
>  }
>  
> -int ics_set_kvm_state_one(ICSState *ics, int srcno)
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp)
>  {
>      uint64_t state;
> -    Error *local_err = NULL;
>      ICSIRQState *irq = &ics->irqs[srcno];
>      int ret;
>  
> @@ -278,16 +278,15 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno)
>      }
>  
>      ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> -                            srcno + ics->offset, &state, true, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> +                            srcno + ics->offset, &state, true, errp);
> +    if (ret < 0) {
>          return ret;
>      }
>  
>      return 0;
>  }
>  
> -int ics_set_kvm_state(ICSState *ics)
> +int ics_set_kvm_state(ICSState *ics, Error **errp)
>  {
>      int i;
>  
> @@ -297,10 +296,12 @@ int ics_set_kvm_state(ICSState *ics)
>      }
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
> +        Error *local_err = NULL;
>          int ret;
>  
> -        ret = ics_set_kvm_state_one(ics, i);
> -        if (ret) {
> +        ret = ics_set_kvm_state_one(ics, i, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
>              return ret;
>          }
>      }
> @@ -402,12 +403,18 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>      }
>  
>      /* Update the KVM sources */
> -    ics_set_kvm_state(spapr->ics);
> +    ics_set_kvm_state(spapr->ics, &local_err);
> +    if (local_err) {
> +        goto fail;
> +    }
>  
>      /* Connect the presenters to the initial VCPUs of the machine */
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
> +        icp_set_kvm_state(spapr_cpu_state(cpu)->icp, &local_err);
> +        if (local_err) {
> +            goto fail;
> +        }
>      }
>  
>      return 0;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index eb65ad7e43b7..1eb7b5cd6847 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -190,13 +190,13 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>  
>  /* KVM */
>  void icp_get_kvm_state(ICPState *icp);
> -int icp_set_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp, Error **errp);
>  void icp_synchronize_state(ICPState *icp);
>  void icp_kvm_realize(DeviceState *dev, Error **errp);
>  
>  void ics_get_kvm_state(ICSState *ics);
> -int ics_set_kvm_state_one(ICSState *ics, int srcno);
> -int ics_set_kvm_state(ICSState *ics);
> +int ics_set_kvm_state_one(ICSState *ics, int srcno, Error **errp);
> +int ics_set_kvm_state(ICSState *ics, Error **errp);
>  void ics_synchronize_state(ICSState *ics);
>  void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>  
> 

-- 
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: [Qemu-devel] [PATCH 6/6] xics/kvm: Add proper rollback to xics_kvm_init()

[David Gibson]

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

On Mon, Jun 17, 2019 at 03:47:03PM +0200, Greg Kurz wrote:
> Make xics_kvm_disconnect() able to undo the changes of a partial execution
> of xics_kvm_connect() and use it to perform rollback.
> 
> Note that kvmppc_define_rtas_kernel_token(0) never fails, no matter the
> RTAS call has been defined or not.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4bfbe1a84092..51433b19b076 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -421,10 +421,7 @@ int xics_kvm_connect(SpaprMachineState *spapr, Error **errp)
>  
>  fail:
>      error_propagate(errp, local_err);
> -    kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> -    kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
> +    xics_kvm_disconnect(spapr, NULL);
>      return -1;
>  }
>  
> @@ -448,8 +445,10 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp)
>       * removed from the list of devices of the VM. The VCPU presenters
>       * are also detached from the device.
>       */
> -    close(kernel_xics_fd);
> -    kernel_xics_fd = -1;
> +    if (kernel_xics_fd != -1) {
> +        close(kernel_xics_fd);
> +        kernel_xics_fd = -1;
> +    }
>  
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
> 

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