| Message ID | 20160205084340.4178.52171.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > xics_alloc_block() does not return a clear error code when it > fails to allocate a block of interrupts. Instead it returns the > base interrupt number minus 1. This change updates it to return a > clear -1 in case of failure (following the example of xics_alloc()). > > The two callers of xics_alloc_block() are updated to check for > a negative return as an error. They had previously checked for > a 0 return as an error, which wrongly treated most failures as > successes. > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > [only pass src and num to trace_xics_alloc_block_failed_no_left, > added trace_xics_alloc_block_failed_no_left definition to trace-events] > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> Hrm, it would probably be better to give xics_alloc_block() an Error ** argument so it can report errors using the new API. TBH the whole xics_alloc_block() interface is kind of dubious, or at least the ics_find_free_block() part of it. Dynamically allocating irqs to devices is basically awful for migration, so it's better to have fixed allocations of all interrupts at the machine level. > --- > hw/intc/xics.c | 10 ++++++---- > hw/ppc/spapr_pci.c | 9 +++++---- > trace-events | 1 + > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index cd91ddc4d1d9..3bb77ff96e7b 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -763,11 +763,13 @@ 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) { > + trace_xics_alloc_block_failed_no_left(src, num); > + return -1; > + } > > - if (first >= 0) { > - for (i = first; i < first + num; ++i) { > - ics_set_irq_type(ics, i, lsi); > - } > + for (i = first; i < first + num; ++i) { > + ics_set_irq_type(ics, i, lsi); > } > first += ics->offset; > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index cca9257fecc5..ba33cee2a465 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; > + int irq; > sPAPRPHBState *phb = NULL; > PCIDevice *pdev = NULL; > spapr_pci_msi *msi; > @@ -354,7 +355,7 @@ 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) { > + if (irq < 0) { > error_report("Cannot allocate MSIs for device %x", config_addr); > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > return; > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > /* Initialize the LSI table */ > for (i = 0; i < PCI_NUM_PINS; i++) { > - uint32_t irq; > + int32_t irq; > > irq = xics_alloc_block(spapr->icp, 0, 1, true, false); > - if (!irq) { > + if (irq < 0) { > error_setg(errp, "spapr_allocate_lsi failed"); > return; > } > diff --git a/trace-events b/trace-events > index c9ac144ceee4..07b0250aaf11 100644 > --- a/trace-events > +++ b/trace-events > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" > 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" > >
On Mon, 8 Feb 2016 11:45:19 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > xics_alloc_block() does not return a clear error code when it > > fails to allocate a block of interrupts. Instead it returns the > > base interrupt number minus 1. This change updates it to return a > > clear -1 in case of failure (following the example of xics_alloc()). > > > > The two callers of xics_alloc_block() are updated to check for > > a negative return as an error. They had previously checked for > > a 0 return as an error, which wrongly treated most failures as > > successes. > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Hrm, it would probably be better to give xics_alloc_block() an Error > ** argument so it can report errors using the new API. > Sure. I can rework the patch to do so. > TBH the whole xics_alloc_block() interface is kind of dubious, or at > least the ics_find_free_block() part of it. Dynamically allocating > irqs to devices is basically awful for migration, so it's better to > have fixed allocations of all interrupts at the machine level. > I agree about the extra complexity, but isn't it the purpose of the ibm,change-msi RTAS call ? I'm not sure to understand what you are suggesting... > > > --- > > hw/intc/xics.c | 10 ++++++---- > > hw/ppc/spapr_pci.c | 9 +++++---- > > trace-events | 1 + > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > index cd91ddc4d1d9..3bb77ff96e7b 100644 > > --- a/hw/intc/xics.c > > +++ b/hw/intc/xics.c > > @@ -763,11 +763,13 @@ 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) { > > + trace_xics_alloc_block_failed_no_left(src, num); > > + return -1; > > + } > > > > - if (first >= 0) { > > - for (i = first; i < first + num; ++i) { > > - ics_set_irq_type(ics, i, lsi); > > - } > > + for (i = first; i < first + num; ++i) { > > + ics_set_irq_type(ics, i, lsi); > > } > > first += ics->offset; > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index cca9257fecc5..ba33cee2a465 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; > > + int irq; > > sPAPRPHBState *phb = NULL; > > PCIDevice *pdev = NULL; > > spapr_pci_msi *msi; > > @@ -354,7 +355,7 @@ 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) { > > + if (irq < 0) { > > error_report("Cannot allocate MSIs for device %x", config_addr); > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > > return; > > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > > > /* Initialize the LSI table */ > > for (i = 0; i < PCI_NUM_PINS; i++) { > > - uint32_t irq; > > + int32_t irq; > > > > irq = xics_alloc_block(spapr->icp, 0, 1, true, false); > > - if (!irq) { > > + if (irq < 0) { > > error_setg(errp, "spapr_allocate_lsi failed"); > > return; > > } > > diff --git a/trace-events b/trace-events > > index c9ac144ceee4..07b0250aaf11 100644 > > --- a/trace-events > > +++ b/trace-events > > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" > > 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" > > > > >
On Mon, 8 Feb 2016 09:31:49 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Mon, 8 Feb 2016 11:45:19 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > xics_alloc_block() does not return a clear error code when it > > > fails to allocate a block of interrupts. Instead it returns the > > > base interrupt number minus 1. This change updates it to return a > > > clear -1 in case of failure (following the example of xics_alloc()). > > > > > > The two callers of xics_alloc_block() are updated to check for > > > a negative return as an error. They had previously checked for > > > a 0 return as an error, which wrongly treated most failures as > > > successes. > > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > ** argument so it can report errors using the new API. > > > > Sure. I can rework the patch to do so. > The trace_xics_alloc_block_failed_no_left trace is more a debugging thing than an error to be reported to the user. Also, rtas_ibm_change_msi() already has a meaningful error message: error_report("Cannot allocate MSIs for device %x", config_addr); So in the end, I'm not sure about the benefit of passing an Error ** down to xics_alloc_block(). > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > least the ics_find_free_block() part of it. Dynamically allocating > > irqs to devices is basically awful for migration, so it's better to > > have fixed allocations of all interrupts at the machine level. > > > > I agree about the extra complexity, but isn't it the purpose of > the ibm,change-msi RTAS call ? I'm not sure to understand what you > are suggesting... > And anyway, even if the decision is made one day to have fixed allocations, shouldn't we consider fixing this bug first ? Cheers. -- Greg > > > > > --- > > > hw/intc/xics.c | 10 ++++++---- > > > hw/ppc/spapr_pci.c | 9 +++++---- > > > trace-events | 1 + > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > > index cd91ddc4d1d9..3bb77ff96e7b 100644 > > > --- a/hw/intc/xics.c > > > +++ b/hw/intc/xics.c > > > @@ -763,11 +763,13 @@ 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) { > > > + trace_xics_alloc_block_failed_no_left(src, num); > > > + return -1; > > > + } > > > > > > - if (first >= 0) { > > > - for (i = first; i < first + num; ++i) { > > > - ics_set_irq_type(ics, i, lsi); > > > - } > > > + for (i = first; i < first + num; ++i) { > > > + ics_set_irq_type(ics, i, lsi); > > > } > > > first += ics->offset; > > > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > > index cca9257fecc5..ba33cee2a465 100644 > > > --- a/hw/ppc/spapr_pci.c > > > +++ b/hw/ppc/spapr_pci.c > > > @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; > > > + int irq; > > > sPAPRPHBState *phb = NULL; > > > PCIDevice *pdev = NULL; > > > spapr_pci_msi *msi; > > > @@ -354,7 +355,7 @@ 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) { > > > + if (irq < 0) { > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > > > return; > > > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > > > > > /* Initialize the LSI table */ > > > for (i = 0; i < PCI_NUM_PINS; i++) { > > > - uint32_t irq; > > > + int32_t irq; > > > > > > irq = xics_alloc_block(spapr->icp, 0, 1, true, false); > > > - if (!irq) { > > > + if (irq < 0) { > > > error_setg(errp, "spapr_allocate_lsi failed"); > > > return; > > > } > > > diff --git a/trace-events b/trace-events > > > index c9ac144ceee4..07b0250aaf11 100644 > > > --- a/trace-events > > > +++ b/trace-events > > > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" > > > 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" > > > > > > > > >
On Wed, 10 Feb 2016 10:41:16 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Mon, 8 Feb 2016 09:31:49 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Mon, 8 Feb 2016 11:45:19 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > > > xics_alloc_block() does not return a clear error code when it > > > > fails to allocate a block of interrupts. Instead it returns the > > > > base interrupt number minus 1. This change updates it to return a > > > > clear -1 in case of failure (following the example of xics_alloc()). > > > > > > > > The two callers of xics_alloc_block() are updated to check for > > > > a negative return as an error. They had previously checked for > > > > a 0 return as an error, which wrongly treated most failures as > > > > successes. > > > > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > > ** argument so it can report errors using the new API. > > > > > > > Sure. I can rework the patch to do so. > > > > The trace_xics_alloc_block_failed_no_left trace is more a debugging thing > than an error to be reported to the user. Also, rtas_ibm_change_msi() > already has a meaningful error message: > > error_report("Cannot allocate MSIs for device %x", config_addr); > > So in the end, I'm not sure about the benefit of passing an Error ** > down to xics_alloc_block(). > Hi David ! Given the remarks above, do you still think we should pass Error ** ? > > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > > least the ics_find_free_block() part of it. Dynamically allocating > > > irqs to devices is basically awful for migration, so it's better to > > > have fixed allocations of all interrupts at the machine level. > > > > > > > I agree about the extra complexity, but isn't it the purpose of > > the ibm,change-msi RTAS call ? I'm not sure to understand what you > > are suggesting... > > > > And anyway, even if the decision is made one day to have fixed > allocations, shouldn't we consider fixing this bug first ? > According to the following commit changelog, the dynamic allocation was introduced to support PCI hot(un)plug. Maybe Alexey may explain why it got coded this way. commit bee763dbfb8cfceea112131970da07f215f293a6 Author: Alexey Kardashevskiy <aik@ozlabs.ru> Date: Fri May 30 19:34:15 2014 +1000 spapr: Move interrupt allocator to xics I'm not sure of the amount of reflexion and work needed to address your concern... Given the time frame, what about deferring xics rework to 2.7 and fix the bug we currently have in 2.6 ? Thanks ! -- Greg > Cheers. > > -- > Greg > > > > > > > > --- > > > > hw/intc/xics.c | 10 ++++++---- > > > > hw/ppc/spapr_pci.c | 9 +++++---- > > > > trace-events | 1 + > > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > > > index cd91ddc4d1d9..3bb77ff96e7b 100644 > > > > --- a/hw/intc/xics.c > > > > +++ b/hw/intc/xics.c > > > > @@ -763,11 +763,13 @@ 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) { > > > > + trace_xics_alloc_block_failed_no_left(src, num); > > > > + return -1; > > > > + } > > > > > > > > - if (first >= 0) { > > > > - for (i = first; i < first + num; ++i) { > > > > - ics_set_irq_type(ics, i, lsi); > > > > - } > > > > + for (i = first; i < first + num; ++i) { > > > > + ics_set_irq_type(ics, i, lsi); > > > > } > > > > first += ics->offset; > > > > > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > > > index cca9257fecc5..ba33cee2a465 100644 > > > > --- a/hw/ppc/spapr_pci.c > > > > +++ b/hw/ppc/spapr_pci.c > > > > @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; > > > > + int irq; > > > > sPAPRPHBState *phb = NULL; > > > > PCIDevice *pdev = NULL; > > > > spapr_pci_msi *msi; > > > > @@ -354,7 +355,7 @@ 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) { > > > > + if (irq < 0) { > > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > > > > return; > > > > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > > > > > > > /* Initialize the LSI table */ > > > > for (i = 0; i < PCI_NUM_PINS; i++) { > > > > - uint32_t irq; > > > > + int32_t irq; > > > > > > > > irq = xics_alloc_block(spapr->icp, 0, 1, true, false); > > > > - if (!irq) { > > > > + if (irq < 0) { > > > > error_setg(errp, "spapr_allocate_lsi failed"); > > > > return; > > > > } > > > > diff --git a/trace-events b/trace-events > > > > index c9ac144ceee4..07b0250aaf11 100644 > > > > --- a/trace-events > > > > +++ b/trace-events > > > > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" > > > > 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" > > > > > > > > > > > > > > >
Just pinging Alexey as well. :) On Tue, 23 Feb 2016 18:46:44 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Wed, 10 Feb 2016 10:41:16 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Mon, 8 Feb 2016 09:31:49 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > On Mon, 8 Feb 2016 11:45:19 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > > > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > > > > > xics_alloc_block() does not return a clear error code when it > > > > > fails to allocate a block of interrupts. Instead it returns the > > > > > base interrupt number minus 1. This change updates it to return a > > > > > clear -1 in case of failure (following the example of xics_alloc()). > > > > > > > > > > The two callers of xics_alloc_block() are updated to check for > > > > > a negative return as an error. They had previously checked for > > > > > a 0 return as an error, which wrongly treated most failures as > > > > > successes. > > > > > > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > > > ** argument so it can report errors using the new API. > > > > > > > > > > Sure. I can rework the patch to do so. > > > > > > > The trace_xics_alloc_block_failed_no_left trace is more a debugging thing > > than an error to be reported to the user. Also, rtas_ibm_change_msi() > > already has a meaningful error message: > > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > > So in the end, I'm not sure about the benefit of passing an Error ** > > down to xics_alloc_block(). > > > > Hi David ! > > Given the remarks above, do you still think we should pass Error ** ? > > > > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > > > least the ics_find_free_block() part of it. Dynamically allocating > > > > irqs to devices is basically awful for migration, so it's better to > > > > have fixed allocations of all interrupts at the machine level. > > > > > > > > > > I agree about the extra complexity, but isn't it the purpose of > > > the ibm,change-msi RTAS call ? I'm not sure to understand what you > > > are suggesting... > > > > > > > And anyway, even if the decision is made one day to have fixed > > allocations, shouldn't we consider fixing this bug first ? > > > > According to the following commit changelog, the dynamic allocation was > introduced to support PCI hot(un)plug. Maybe Alexey may explain why it > got coded this way. > > commit bee763dbfb8cfceea112131970da07f215f293a6 > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > Date: Fri May 30 19:34:15 2014 +1000 > > spapr: Move interrupt allocator to xics > > I'm not sure of the amount of reflexion and work needed to address your > concern... Given the time frame, what about deferring xics rework to 2.7 > and fix the bug we currently have in 2.6 ? > > Thanks ! > > -- > Greg > > > Cheers. > > > > -- > > Greg > > > > > > > > > > > --- > > > > > hw/intc/xics.c | 10 ++++++---- > > > > > hw/ppc/spapr_pci.c | 9 +++++---- > > > > > trace-events | 1 + > > > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > > > > index cd91ddc4d1d9..3bb77ff96e7b 100644 > > > > > --- a/hw/intc/xics.c > > > > > +++ b/hw/intc/xics.c > > > > > @@ -763,11 +763,13 @@ 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) { > > > > > + trace_xics_alloc_block_failed_no_left(src, num); > > > > > + return -1; > > > > > + } > > > > > > > > > > - if (first >= 0) { > > > > > - for (i = first; i < first + num; ++i) { > > > > > - ics_set_irq_type(ics, i, lsi); > > > > > - } > > > > > + for (i = first; i < first + num; ++i) { > > > > > + ics_set_irq_type(ics, i, lsi); > > > > > } > > > > > first += ics->offset; > > > > > > > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > > > > index cca9257fecc5..ba33cee2a465 100644 > > > > > --- a/hw/ppc/spapr_pci.c > > > > > +++ b/hw/ppc/spapr_pci.c > > > > > @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; > > > > > + int irq; > > > > > sPAPRPHBState *phb = NULL; > > > > > PCIDevice *pdev = NULL; > > > > > spapr_pci_msi *msi; > > > > > @@ -354,7 +355,7 @@ 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) { > > > > > + if (irq < 0) { > > > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > > > > > return; > > > > > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > > > > > > > > > > /* Initialize the LSI table */ > > > > > for (i = 0; i < PCI_NUM_PINS; i++) { > > > > > - uint32_t irq; > > > > > + int32_t irq; > > > > > > > > > > irq = xics_alloc_block(spapr->icp, 0, 1, true, false); > > > > > - if (!irq) { > > > > > + if (irq < 0) { > > > > > error_setg(errp, "spapr_allocate_lsi failed"); > > > > > return; > > > > > } > > > > > diff --git a/trace-events b/trace-events > > > > > index c9ac144ceee4..07b0250aaf11 100644 > > > > > --- a/trace-events > > > > > +++ b/trace-events > > > > > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" > > > > > 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" > > > > > > > > > > > > > > > > > > > > > > >
On Tue, Feb 23, 2016 at 06:46:44PM +0100, Greg Kurz wrote: > On Wed, 10 Feb 2016 10:41:16 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Mon, 8 Feb 2016 09:31:49 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > On Mon, 8 Feb 2016 11:45:19 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > > > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > > > > > xics_alloc_block() does not return a clear error code when it > > > > > fails to allocate a block of interrupts. Instead it returns the > > > > > base interrupt number minus 1. This change updates it to return a > > > > > clear -1 in case of failure (following the example of xics_alloc()). > > > > > > > > > > The two callers of xics_alloc_block() are updated to check for > > > > > a negative return as an error. They had previously checked for > > > > > a 0 return as an error, which wrongly treated most failures as > > > > > successes. > > > > > > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > > > ** argument so it can report errors using the new API. > > > > > > > > > > Sure. I can rework the patch to do so. > > > > > > > The trace_xics_alloc_block_failed_no_left trace is more a debugging thing > > than an error to be reported to the user. Also, rtas_ibm_change_msi() > > already has a meaningful error message: > > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > > So in the end, I'm not sure about the benefit of passing an Error ** > > down to xics_alloc_block(). > > > > Hi David ! > > Given the remarks above, do you still think we should pass Error ** ? I still think using the Error API would be preferable, but it doesn't make a huge difference. > > > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > > > least the ics_find_free_block() part of it. Dynamically allocating > > > > irqs to devices is basically awful for migration, so it's better to > > > > have fixed allocations of all interrupts at the machine level. > > > > > > > > > > I agree about the extra complexity, but isn't it the purpose of > > > the ibm,change-msi RTAS call ? I'm not sure to understand what you > > > are suggesting... > > > > > > > And anyway, even if the decision is made one day to have fixed > > allocations, shouldn't we consider fixing this bug first ? > > > > According to the following commit changelog, the dynamic allocation was > introduced to support PCI hot(un)plug. Maybe Alexey may explain why it > got coded this way. > > commit bee763dbfb8cfceea112131970da07f215f293a6 > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > Date: Fri May 30 19:34:15 2014 +1000 > > spapr: Move interrupt allocator to xics > > I'm not sure of the amount of reflexion and work needed to address your > concern... Given the time frame, what about deferring xics rework to 2.7 > and fix the bug we currently have in 2.6 ? Yeah, sorry, I realised after I sent that the allocation does actually make sense in this case - these are guest triggered allocations that we really do need an allocator for, not host setup allocations which we have used in the past but turned out to be dubious.
On Wed, 24 Feb 2016 11:36:49 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Feb 23, 2016 at 06:46:44PM +0100, Greg Kurz wrote: > > On Wed, 10 Feb 2016 10:41:16 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > On Mon, 8 Feb 2016 09:31:49 +0100 > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > > > On Mon, 8 Feb 2016 11:45:19 +1000 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > > > > > > From: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > > > > > > > xics_alloc_block() does not return a clear error code when it > > > > > > fails to allocate a block of interrupts. Instead it returns the > > > > > > base interrupt number minus 1. This change updates it to return a > > > > > > clear -1 in case of failure (following the example of xics_alloc()). > > > > > > > > > > > > The two callers of xics_alloc_block() are updated to check for > > > > > > a negative return as an error. They had previously checked for > > > > > > a 0 return as an error, which wrongly treated most failures as > > > > > > successes. > > > > > > > > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > > > > Signed-off-by: Brian W. Hart <hartb@linux.vnet.ibm.com> > > > > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > > > > added trace_xics_alloc_block_failed_no_left definition to trace-events] > > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > > > > > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > > > > ** argument so it can report errors using the new API. > > > > > > > > > > > > > Sure. I can rework the patch to do so. > > > > > > > > > > The trace_xics_alloc_block_failed_no_left trace is more a debugging thing > > > than an error to be reported to the user. Also, rtas_ibm_change_msi() > > > already has a meaningful error message: > > > > > > error_report("Cannot allocate MSIs for device %x", config_addr); > > > > > > So in the end, I'm not sure about the benefit of passing an Error ** > > > down to xics_alloc_block(). > > > > > > > Hi David ! > > > > Given the remarks above, do you still think we should pass Error ** ? > > I still think using the Error API would be preferable, but it doesn't > make a huge difference. > > > > > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > > > > least the ics_find_free_block() part of it. Dynamically allocating > > > > > irqs to devices is basically awful for migration, so it's better to > > > > > have fixed allocations of all interrupts at the machine level. > > > > > > > > > > > > > I agree about the extra complexity, but isn't it the purpose of > > > > the ibm,change-msi RTAS call ? I'm not sure to understand what you > > > > are suggesting... > > > > > > > > > > And anyway, even if the decision is made one day to have fixed > > > allocations, shouldn't we consider fixing this bug first ? > > > > > > > According to the following commit changelog, the dynamic allocation was > > introduced to support PCI hot(un)plug. Maybe Alexey may explain why it > > got coded this way. > > > > commit bee763dbfb8cfceea112131970da07f215f293a6 > > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > > Date: Fri May 30 19:34:15 2014 +1000 > > > > spapr: Move interrupt allocator to xics > > > > I'm not sure of the amount of reflexion and work needed to address your > > concern... Given the time frame, what about deferring xics rework to 2.7 > > and fix the bug we currently have in 2.6 ? > > Yeah, sorry, I realised after I sent that the allocation does > actually make sense in this case - these are guest triggered > allocations that we really do need an allocator for, not host setup > allocations which we have used in the past but turned out to be > dubious. > There are other issues related to xics actually. I'll post a series with all the fixes. Cheers. -- Greg
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index cd91ddc4d1d9..3bb77ff96e7b 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -763,11 +763,13 @@ 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) { + trace_xics_alloc_block_failed_no_left(src, num); + return -1; + } - if (first >= 0) { - for (i = first; i < first + num; ++i) { - ics_set_irq_type(ics, i, lsi); - } + for (i = first; i < first + num; ++i) { + ics_set_irq_type(ics, i, lsi); } first += ics->offset; diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index cca9257fecc5..ba33cee2a465 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -275,7 +275,8 @@ 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 max_irqs = 0, num = 0; + int irq; sPAPRPHBState *phb = NULL; PCIDevice *pdev = NULL; spapr_pci_msi *msi; @@ -354,7 +355,7 @@ 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) { + if (irq < 0) { error_report("Cannot allocate MSIs for device %x", config_addr); rtas_st(rets, 0, RTAS_OUT_HW_ERROR); return; @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) /* Initialize the LSI table */ for (i = 0; i < PCI_NUM_PINS; i++) { - uint32_t irq; + int32_t irq; irq = xics_alloc_block(spapr->icp, 0, 1, true, false); - if (!irq) { + if (irq < 0) { error_setg(errp, "spapr_allocate_lsi failed"); return; } diff --git a/trace-events b/trace-events index c9ac144ceee4..07b0250aaf11 100644 --- a/trace-events +++ b/trace-events @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot find %d consecutive irqs" 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"