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

["Cédric Le Goater" <clg@kaod.org>]

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);
>  
>