[Qemu-devel] [PATCH 0/3] spapr_pci and xics fixes

[Qemu-devel] [PATCH 0/3] spapr_pci and xics fixes

[Greg Kurz]

Hi,

This series fixes some issues when sPAPR allocates interrupts. Especially
a potential IRQ leak (patch 2) and bogus interrupt numbers stored in the
MSI table because of poor error reporting from the XICS layer (patch 3).

Please review. I'd like to have this fixes in 2.6.

---

Greg Kurz (3):
      spapr_pci: kill useless variable in rtas_ibm_change_msi()
      spapr_pci: fix irq leak in RTAS ibm,change-msi
      xics: report errors with the QEMU Error API


 hw/intc/xics.c        |   22 ++++++++++++++--------
 hw/ppc/spapr_events.c |    2 +-
 hw/ppc/spapr_pci.c    |   33 ++++++++++++++++++++++-----------
 hw/ppc/spapr_vio.c    |    8 +++++---
 include/hw/ppc/xics.h |    6 ++++--
 trace-events          |    2 --
 6 files changed, 46 insertions(+), 27 deletions(-)

--
Greg

[Qemu-devel] [PATCH 1/3] spapr_pci: kill useless variable in rtas_ibm_change_msi()

[Greg Kurz]

The num local variable is initialized to zero and has no writer.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cca9257fecc5..19dd6dbeb943 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -275,7 +275,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     unsigned int req_num = rtas_ld(args, 4); /* 0 == remove all */
     unsigned int seq_num = rtas_ld(args, 5);
     unsigned int ret_intr_type;
-    unsigned int irq, max_irqs = 0, num = 0;
+    unsigned int irq, max_irqs = 0;
     sPAPRPHBState *phb = NULL;
     PCIDevice *pdev = NULL;
     spapr_pci_msi *msi;
@@ -316,10 +316,10 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
         xics_free(spapr->icp, msi->first_irq, msi->num);
         if (msi_present(pdev)) {
-            spapr_msi_setmsg(pdev, 0, false, 0, num);
+            spapr_msi_setmsg(pdev, 0, false, 0, 0);
         }
         if (msix_present(pdev)) {
-            spapr_msi_setmsg(pdev, 0, true, 0, num);
+            spapr_msi_setmsg(pdev, 0, true, 0, 0);
         }
         g_hash_table_remove(phb->msi, &config_addr);
 

[Qemu-devel] [PATCH 2/3] spapr_pci: fix irq leak in RTAS ibm, change-msi

[Greg Kurz]

This RTAS call is used to request new interrupts or to free all interrupts.

If the driver has already allocated interrupts and asks again for a non-null
number of irqs, then the rtas_ibm_change_msi() function will silently leak
the previous interrupts.

It happens because xics_free() is only called when the driver releases all
interrupts (!req_num case). Note that the previously allocated spapr_pci_msi
is not leaked because the GHashTable is created with destroy functions and
g_hash_table_insert() hence frees the old value.

This patch makes sure any previously allocated MSIs are released when a
new allocation succeeds.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 19dd6dbeb943..9b2b546b541c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -305,9 +305,10 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
+    msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
+
     /* Releasing MSIs */
     if (!req_num) {
-        msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
         if (!msi) {
             trace_spapr_pci_msi("Releasing wrong config", config_addr);
             rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -360,6 +361,12 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
+    /* Release previous MSIs */
+    if (msi) {
+        xics_free(spapr->icp, msi->first_irq, msi->num);
+        g_hash_table_remove(phb->msi, &config_addr);
+    }
+
     /* Setup MSI/MSIX vectors in the device (via cfgspace or MSIX BAR) */
     spapr_msi_setmsg(pdev, SPAPR_PCI_MSI_WINDOW, ret_intr_type == RTAS_TYPE_MSIX,
                      irq, req_num);

[Qemu-devel] [PATCH 3/3] xics: report errors with the QEMU Error API

[Greg Kurz]

Using the return value to report errors is error prone:
- xics_alloc() returns -1 on error but spapr_vio_busdev_realize() errors
  on 0
- xics_alloc_block() returns the unclear value of ics->offset - 1 on error
  but both rtas_ibm_change_msi() and spapr_phb_realize() error on 0

This patch turns xics_alloc() and xics_alloc_block() into void functions
with an errp argument as only way to report errors, and an unsigned int *
argument to return the IRQ number in case of success.

The corresponding error traces get promotted to error messages.

This fixes the issues mentioned above.

Based on previous work from Brian W. Hart.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/intc/xics.c        |   22 ++++++++++++++--------
 hw/ppc/spapr_events.c |    2 +-
 hw/ppc/spapr_pci.c    |   18 +++++++++++-------
 hw/ppc/spapr_vio.c    |    8 +++++---
 include/hw/ppc/xics.h |    6 ++++--
 trace-events          |    2 --
 6 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e66ae328819a..32f28f1693fd 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -712,7 +712,8 @@ static int ics_find_free_block(ICSState *ics, int num, int alignnum)
     return -1;
 }
 
-int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
+void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
+                unsigned int *irqp, Error **errp)
 {
     ICSState *ics = &icp->ics[src];
     int irq;
@@ -720,15 +721,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
     if (irq_hint) {
         assert(src == xics_find_source(icp, irq_hint));
         if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
-            trace_xics_alloc_failed_hint(src, irq_hint);
-            return -1;
+            error_setg(errp, "IRQ %d is already in use", irq_hint);
+            return;
         }
         irq = irq_hint;
     } else {
         irq = ics_find_free_block(ics, 1, 1);
         if (irq < 0) {
-            trace_xics_alloc_failed_no_left(src);
-            return -1;
+            error_setg(errp, "no IRQ left");
+            return;
         }
         irq += ics->offset;
     }
@@ -736,14 +737,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
     ics_set_irq_type(ics, irq - ics->offset, lsi);
     trace_xics_alloc(src, irq);
 
-    return irq;
+    *irqp = irq;
 }
 
 /*
  * Allocate block of consecutive IRQs, and return the number of the first IRQ in the block.
  * If align==true, aligns the first IRQ number to num.
  */
-int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
+void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
+                      unsigned int *irqp, Error **errp)
 {
     int i, first = -1;
     ICSState *ics = &icp->ics[src];
@@ -763,6 +765,10 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
     } else {
         first = ics_find_free_block(ics, num, 1);
     }
+    if (first < 0) {
+        error_setg(errp, "cannot find a %d-IRQ block", num);
+        return;
+    }
 
     if (first >= 0) {
         for (i = first; i < first + num; ++i) {
@@ -773,7 +779,7 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
 
     trace_xics_alloc_block(src, first, num, lsi, align);
 
-    return first;
+    *irqp = first;
 }
 
 static void ics_free(ICSState *ics, int srcno, int num)
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index f5eac4b5441c..d6741d300952 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -588,7 +588,7 @@ out_no_events:
 void spapr_events_init(sPAPRMachineState *spapr)
 {
     QTAILQ_INIT(&spapr->pending_events);
-    spapr->check_exception_irq = xics_alloc(spapr->icp, 0, 0, false);
+    xics_alloc(spapr->icp, 0, 0, false, &spapr->check_exception_irq, NULL);
     spapr->epow_notifier.notify = spapr_powerdown_req;
     qemu_register_powerdown_notifier(&spapr->epow_notifier);
     spapr_rtas_register(RTAS_CHECK_EXCEPTION, "check-exception",
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 9b2b546b541c..9d1a2b7f7279 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -280,6 +280,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     PCIDevice *pdev = NULL;
     spapr_pci_msi *msi;
     int *config_addr_key;
+    Error *err = NULL;
 
     switch (func) {
     case RTAS_CHANGE_MSI_FN:
@@ -353,10 +354,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     /* Allocate MSIs */
-    irq = xics_alloc_block(spapr->icp, 0, req_num, false,
-                           ret_intr_type == RTAS_TYPE_MSI);
-    if (!irq) {
-        error_report("Cannot allocate MSIs for device %x", config_addr);
+    xics_alloc_block(spapr->icp, 0, req_num, false,
+                     ret_intr_type == RTAS_TYPE_MSI, &irq, &err);
+    if (err) {
+        error_reportf_err(err, "Can't allocate MSIs for device %x: ",
+                          config_addr);
         rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
         return;
     }
@@ -1367,10 +1369,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;
+        Error *local_err = NULL;
 
-        irq = xics_alloc_block(spapr->icp, 0, 1, true, false);
-        if (!irq) {
-            error_setg(errp, "spapr_allocate_lsi failed");
+        xics_alloc_block(spapr->icp, 0, 1, true, false, &irq, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            error_prepend(errp, "can't allocate LSIs: ");
             return;
         }
 
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index ac6666a90be7..91b1e8e75385 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -431,6 +431,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
     VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
     VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
     char *id;
+    Error *local_err = NULL;
 
     if (dev->reg != -1) {
         /*
@@ -463,9 +464,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
         dev->qdev.id = id;
     }
 
-    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false);
-    if (!dev->irq) {
-        error_setg(errp, "can't allocate IRQ");
+    xics_alloc(spapr->icp, 0, dev->irq, false, &dev->irq, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        error_prepend(errp, "can't allocate IRQ: ");
         return;
     }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 355a96623c70..3c10fbac8852 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,8 +161,10 @@ struct ICSIRQState {
 
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
 void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
-int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi);
-int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
+void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
+                unsigned int *irqp, Error **errp);
+void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
+                      unsigned int *irqp, Error **errp);
 void xics_free(XICSState *icp, int irq, int num);
 
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
diff --git a/trace-events b/trace-events
index f986c81dada5..3072b45d4081 100644
--- a/trace-events
+++ b/trace-events
@@ -1412,8 +1412,6 @@ xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_
 xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
 xics_ics_eoi(int nr) "ics_eoi: irq %#x"
 xics_alloc(int src, int irq) "source#%d, irq %d"
-xics_alloc_failed_hint(int src, int irq) "source#%d, irq %d is already in use"
-xics_alloc_failed_no_left(int src) "source#%d, no irq left"
 xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
 xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
 xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"

Re: [Qemu-devel] [PATCH 3/3] xics: report errors with the QEMU Error API

[David Gibson]

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

On Thu, Feb 25, 2016 at 07:02:25PM +0100, Greg Kurz wrote:
> Using the return value to report errors is error prone:
> - xics_alloc() returns -1 on error but spapr_vio_busdev_realize() errors
>   on 0
> - xics_alloc_block() returns the unclear value of ics->offset - 1 on error
>   but both rtas_ibm_change_msi() and spapr_phb_realize() error on 0
> 
> This patch turns xics_alloc() and xics_alloc_block() into void functions
> with an errp argument as only way to report errors, and an unsigned int *
> argument to return the IRQ number in case of success.
> 
> The corresponding error traces get promotted to error messages.
> 
> This fixes the issues mentioned above.
> 
> Based on previous work from Brian W. Hart.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c        |   22 ++++++++++++++--------
>  hw/ppc/spapr_events.c |    2 +-
>  hw/ppc/spapr_pci.c    |   18 +++++++++++-------
>  hw/ppc/spapr_vio.c    |    8 +++++---
>  include/hw/ppc/xics.h |    6 ++++--
>  trace-events          |    2 --
>  6 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e66ae328819a..32f28f1693fd 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -712,7 +712,8 @@ static int ics_find_free_block(ICSState *ics, int num, int alignnum)
>      return -1;
>  }
>  
> -int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
> +void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
> +                unsigned int *irqp, Error **errp)

So, I like adding the error API, but I don't really like the change to
a void function.  I think it would be better to keep returning the irq
number through the return value in the success case, and just return
the error values.

>  {
>      ICSState *ics = &icp->ics[src];
>      int irq;
> @@ -720,15 +721,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
>      if (irq_hint) {
>          assert(src == xics_find_source(icp, irq_hint));
>          if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
> -            trace_xics_alloc_failed_hint(src, irq_hint);
> -            return -1;
> +            error_setg(errp, "IRQ %d is already in use", irq_hint);
> +            return;

So, I know the callers add extra information, but I don't think you
should really count on that.  It would be better to have a more
complete error message right here, in case this appears without
context.

>          }
>          irq = irq_hint;
>      } else {
>          irq = ics_find_free_block(ics, 1, 1);
>          if (irq < 0) {
> -            trace_xics_alloc_failed_no_left(src);
> -            return -1;
> +            error_setg(errp, "no IRQ left");
> +            return;
>          }
>          irq += ics->offset;
>      }
> @@ -736,14 +737,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
>      ics_set_irq_type(ics, irq - ics->offset, lsi);
>      trace_xics_alloc(src, irq);
>  
> -    return irq;
> +    *irqp = irq;
>  }
>  
>  /*
>   * Allocate block of consecutive IRQs, and return the number of the first IRQ in the block.
>   * If align==true, aligns the first IRQ number to num.
>   */
> -int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
> +void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                      unsigned int *irqp, Error **errp)
>  {
>      int i, first = -1;
>      ICSState *ics = &icp->ics[src];
> @@ -763,6 +765,10 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
>      } else {
>          first = ics_find_free_block(ics, num, 1);
>      }
> +    if (first < 0) {
> +        error_setg(errp, "cannot find a %d-IRQ block", num);
> +        return;
> +    }
>  
>      if (first >= 0) {
>          for (i = first; i < first + num; ++i) {
> @@ -773,7 +779,7 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
>  
>      trace_xics_alloc_block(src, first, num, lsi, align);
>  
> -    return first;
> +    *irqp = first;
>  }
>  
>  static void ics_free(ICSState *ics, int srcno, int num)
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index f5eac4b5441c..d6741d300952 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -588,7 +588,7 @@ out_no_events:
>  void spapr_events_init(sPAPRMachineState *spapr)
>  {
>      QTAILQ_INIT(&spapr->pending_events);
> -    spapr->check_exception_irq = xics_alloc(spapr->icp, 0, 0, false);
> +    xics_alloc(spapr->icp, 0, 0, false, &spapr->check_exception_irq, NULL);

I don't think you want NULL for the errp, you probably want &error_fatal.

>      spapr->epow_notifier.notify = spapr_powerdown_req;
>      qemu_register_powerdown_notifier(&spapr->epow_notifier);
>      spapr_rtas_register(RTAS_CHECK_EXCEPTION, "check-exception",
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 9b2b546b541c..9d1a2b7f7279 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -280,6 +280,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      PCIDevice *pdev = NULL;
>      spapr_pci_msi *msi;
>      int *config_addr_key;
> +    Error *err = NULL;
>  
>      switch (func) {
>      case RTAS_CHANGE_MSI_FN:
> @@ -353,10 +354,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      /* Allocate MSIs */
> -    irq = xics_alloc_block(spapr->icp, 0, req_num, false,
> -                           ret_intr_type == RTAS_TYPE_MSI);
> -    if (!irq) {
> -        error_report("Cannot allocate MSIs for device %x", config_addr);
> +    xics_alloc_block(spapr->icp, 0, req_num, false,
> +                     ret_intr_type == RTAS_TYPE_MSI, &irq, &err);
> +    if (err) {
> +        error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> +                          config_addr);
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>          return;
>      }
> @@ -1367,10 +1369,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;
> +        Error *local_err = NULL;
>  
> -        irq = xics_alloc_block(spapr->icp, 0, 1, true, false);
> -        if (!irq) {
> -            error_setg(errp, "spapr_allocate_lsi failed");
> +        xics_alloc_block(spapr->icp, 0, 1, true, false, &irq, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "can't allocate LSIs: ");
>              return;
>          }
>  
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index ac6666a90be7..91b1e8e75385 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -431,6 +431,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>      VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>      char *id;
> +    Error *local_err = NULL;
>  
>      if (dev->reg != -1) {
>          /*
> @@ -463,9 +464,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          dev->qdev.id = id;
>      }
>  
> -    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false);
> -    if (!dev->irq) {
> -        error_setg(errp, "can't allocate IRQ");
> +    xics_alloc(spapr->icp, 0, dev->irq, false, &dev->irq, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "can't allocate IRQ: ");
>          return;
>      }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 355a96623c70..3c10fbac8852 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,8 +161,10 @@ struct ICSIRQState {
>  
>  qemu_irq xics_get_qirq(XICSState *icp, int irq);
>  void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
> -int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi);
> -int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
> +void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
> +                unsigned int *irqp, Error **errp);
> +void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                      unsigned int *irqp, Error **errp);
>  void xics_free(XICSState *icp, int irq, int num);
>  
>  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> diff --git a/trace-events b/trace-events
> index f986c81dada5..3072b45d4081 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1412,8 +1412,6 @@ xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_
>  xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>  xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>  xics_alloc(int src, int irq) "source#%d, irq %d"
> -xics_alloc_failed_hint(int src, int irq) "source#%d, irq %d is already in use"
> -xics_alloc_failed_no_left(int src) "source#%d, no irq left"
>  xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>  xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] spapr_pci and xics fixes

[David Gibson]

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

On Thu, Feb 25, 2016 at 07:02:06PM +0100, Greg Kurz wrote:
> Hi,
> 
> This series fixes some issues when sPAPR allocates interrupts. Especially
> a potential IRQ leak (patch 2) and bogus interrupt numbers stored in the
> MSI table because of poor error reporting from the XICS layer (patch 3).
> 
> Please review. I'd like to have this fixes in 2.6.

I've applied 1,2/3, but I have some comments on 3/3.

-- 
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: 819 bytes --]