Re: [Qemu-devel] [Qemu-ppc] [PATCH] hw/intc: fix failure return for xics_alloc_block()

[Greg Kurz]

On Wed, 10 Feb 2016 10:41:16 +0100
Greg Kurz <address@hidden> wrote:

> On Mon, 8 Feb 2016 09:31:49 +0100
> Greg Kurz <address@hidden> wrote:
> 
> > On Mon, 8 Feb 2016 11:45:19 +1000
> > David Gibson <address@hidden> wrote:
> >   
> > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote:    
> > > > From: Brian W. Hart <address@hidden>
> > > > 
> > > > 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 <address@hidden>
> > > > [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 <address@hidden>      
> > > 
> > > 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 <address@hidden>
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"
> > > >  
> > > >       
> > >     
> >   
> 
>