Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] spapr: introduce a fixed IRQ number space

[Greg Kurz <groug@kaod.org>]

On Mon, 18 Jun 2018 10:56:55 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> [ ... ]
> 
> >>> This is 4 irqs per PHB, hence up to 32 PHBs. Cool, we're currently
> >>> limited to 31 PHBs.
> >>>     
> >>>> +#define SPAPR_IRQ_MSI        0x1100  /* Offset of the dynamic range covered    
> >>>
> >>> We only support dynamic MSIs with PCI, maybe rename to SPAPR_IRQ_PCI_MSI ?    
> >>
> >> hmm, no. We could have CAPI devices there. remember ? ;)
> >>  
> > 
> > Well... OpenCAPI devices are exposed to the OS as PCI devices, so I'm not
> > sure we need a CAPI specific range.  
> 
> yes. so this range is common to all devices doing dynamic allocation
> of IRQs. How should we call it ? 
> 
> >>>> +                                      * by the bitmap allocator */    
> >>>
> >>> The range size is hence 1k (XICS_IRQS_SPAPR) for the time being.    
> >>
> >> in fact we could this bogus limit and use spapr->irq_map_nr now.
> >>  
> > 
> > "we could *missing verb* this bogus limit"... so I'm not sure to
> > understand...  
> 
> oups. We could use spapr->irq_map_nr instead of XICS_IRQS_SPAPR when
> defining : 
> 
>     _FDT(fdt_setprop_cell(fdt, bus_off, "ibm,pe-total-#msi", XICS_IRQS_SPAPR));
> 
> in spapr_pci.c
> 

Ah... yeah, I've always found weird that all PHBs advertise the same number
of available MSIs as the total number of irqs for the whole machine. And
putting spapr->irq_map_nr looks weird all the same if all PHBs rely on the
same bitmap actually.

I'm thinking of doing the other way around: keep XICS_IRQS_SPAPR in
"ibm,pe-total-#msi" and have one XICS_IRQS_SPAPR sized bitmap per PHB.

> [ ... ]
> 
> >>>> +        if (spapr->xics_legacy) {
> >>>> +            dev->irq = spapr_irq_findone(spapr, &local_err);
> >>>> +            if (local_err) {
> >>>> +                error_propagate(errp, local_err);
> >>>> +                return;
> >>>> +            }
> >>>> +        } else {
> >>>> +            dev->irq = SPAPR_IRQ_VIO + vio_index++;    
> >>>
> >>> This can overlap the next range if we have more than 64 VIO devices...    
> >>
> >> yes. but claim() should fail.
> >>  
> > 
> > Hmm... I have the impression claim() only fails if:
> > - irq < ics->offset (ie, XICS_IRQ_BASE == 4096)
> > - or irq >= ics->offset + ics->nr_irqs (ie, XICS_IRQS_SPAPR == 1024)
> > - or irq is already in use
> > 
> > I can't find code that would prevent dev->irq to reach SPAPR_IRQ_MSI.  
> 
> Ah yes. It can overlap. 
> 
> My previous proposal took care of overlaps but something simpler was 
> requested. That's how I understand it at least. We can introduce 
> a maximum for the VIO range or live with it. 

Hmm... I'm not very comfortable with the VIO range silently stealing an
irq number from the bitmap allocator. This would cause spapr_irq_msi_alloc()
to return a value that causes spapr_irq_claim() to fail, which looks buggy.

> 
> C.