On 06/05/2018 05:44 AM, David Gibson wrote: > On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote: >> On Fri, 18 May 2018 18:44:03 +0200 >> Cédric Le Goater <c...@kaod.org> wrote: >> >>> PCI LSIs are today allocated one by one using the IRQ alloc_block >>> routine. Change the code sequence to first allocate a PCI_NUM_PINS >>> block. It will help us providing a generic IRQ framework to the >>> machine. >>> >>> Signed-off-by: Cédric Le Goater <c...@kaod.org> >>> --- >>> hw/ppc/spapr_pci.c | 21 ++++++++++----------- >>> 1 file changed, 10 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>> index 39a14980d397..4fd97ffe4c6e 100644 >>> --- a/hw/ppc/spapr_pci.c >>> +++ b/hw/ppc/spapr_pci.c >>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error >>> **errp) >>> sPAPRTCETable *tcet; >>> const unsigned windows_supported = >>> sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; >>> + uint32_t irq; >>> + Error *local_err = NULL; >>> >>> if (!spapr) { >>> error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries >>> machine"); >>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, >>> Error **errp) >>> QLIST_INSERT_HEAD(&spapr->phbs, sphb, list); >>> >>> /* Initialize the LSI table */ >>> - for (i = 0; i < PCI_NUM_PINS; i++) { >>> - uint32_t irq; >>> - Error *local_err = NULL; >>> - >>> - irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err); >>> - if (local_err) { >>> - error_propagate(errp, local_err); >>> - error_prepend(errp, "can't allocate LSIs: "); >>> - return; >>> - } >>> + irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, >>> &local_err); >>> + if (local_err) { >>> + error_propagate(errp, local_err); >>> + error_prepend(errp, "can't allocate LSIs: "); >>> + return; >>> + } >>> >> >> It isn't strictly equivalent. The current code would be happy with >> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this >> cannot happen since we don't have PHB hotplug. > > This makes me pretty nervous, because it's not obvious it will come up > with the same numbers in all circumstances, which we have to do for > existing machine types.
Given that : - irq_hint is "unused" - all IRQs are allocated sequentially at machine init, - spapr_pci is the only model using the block allocation for MSIs, potentially fragmenting more the IRQ number space but done at guest runtime. - the PHB LSI are the allocated at realize time doing the loop above, - we don't support PHB hotplug - we do support PHB coldplug but then the IRQ allocation is done at machine time, it seems highly improbable that the IRQ number space is fragmented to a point which would not allow the loop above to return four contiguous IRQ numbers, always. That is why I felt confident changing the loop to a single block allocation. > It's also not obvious to me why it's useful > to go via this step before going straight to static allocation of the > irq numbers. It pollutes the new sPAPR IRQ interface API with an extra parameter to support both underlying backend and it complexifies the code to handle block allocation of a single IRQ (like above) within an IRQ range (the PCI LSIs). So you end up having a family, a device index, a count, an alignment, and an index within the range. pffut. Also, could we kill the alignment ? C. > If you can convince me this will (in practice) return the same numbers > as the existing code for all valid setups, and that it's a useful > intermediate step, then I'll apply it. > >> >>> - sphb->lsi_table[i].irq = irq; >>> + for (i = 0; i < PCI_NUM_PINS; i++) { >>> + sphb->lsi_table[i].irq = irq + i; >>> } >>> >>> /* allocate connectors for child PCI devices */ >> >