On Wed, 15 Nov 2017 15:24:08 +0000
Cédric Le Goater <c...@kaod.org> wrote:
> On 11/14/2017 03:45 PM, Greg Kurz wrote:
> > On Fri, 10 Nov 2017 15:20:13 +0000
> > Cédric Le Goater <c...@kaod.org> wrote:
> >
> >> 'irq_base' is a base IRQ number which lets us allocate only the subset
> >> of the IRQ numbers used on the sPAPR platform. It is sync with the
> >> ICSState 'offset' attribute and this is slightly redundant. We could
> >> also choose to waste some extra bytes (512) and allocate the whole
> >> number space. To be discussed.
> >>
> >> But more important, it removes a dependency on the ICSState object of
> >> the sPAPR machine which is required for XIVE.
> >>
> >> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> >> ---
> >> hw/ppc/spapr.c | 7 ++++---
> >> include/hw/ppc/spapr.h | 1 +
> >> 2 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index bf0e5b4f815b..1cbbd7715a85 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -2362,6 +2362,7 @@ static void ppc_spapr_init(MachineState *machine)
> >> /* Initialize the IRQ allocator */
> >> spapr->nr_irqs = XICS_IRQS_SPAPR;
> >> spapr->irq_map = bitmap_new(spapr->nr_irqs);
> >> + spapr->irq_base = XICS_IRQ_BASE;
> >>
> >
> > Since this is a constant value, do we really need a machine-level value ?
>
> no. I don't think either.
>
> But I would like to know why we are starting to allocate IRQ numbers
> at 4096 ? Only 2 is reserved fo IPIs. So that seems a little large.
> I have not found the reason though.
>
Same here... I've tried to git blame/log and google qemu-devel archives
and couldn't find anything either.
>
> Also I am starting to think that we should probably segment the allocation
> per device like this is specified in the PAPR specs. Each device has one
> or more Bus Unit IDentifier (BUID) which acts as a prefix for the IRQ
> number. That would facilitate the IRQ numbering and fix some issues
> in migration when devices are hotplugged. I am thinking about phbs
> mostly.
Makes sense. Also there's something we should clarify: we create one ICS for
the entire machine, able to handle XICS_IRQS_SPAPR (== 1024) irqs. But each PHB
advertises it can provide XICS_IRQS_SPAPR MSIs through the “ibm,pe-total-#msi”
DT prop... this looks wrong.
>
> C.
>
>
> > Especially now that all the code that needs it is in spapr.c, I guess it
> > can directly use the macro, no ?
> >
> >> /* Set up Interrupt Controller before we create the VCPUs */
> >> xics_system_init(machine, spapr->nr_irqs, &error_fatal);
> >> @@ -3630,7 +3631,7 @@ static void spapr_irq_free_block_2_11(XICSFabric
> >> *xi, int irq, int num)
> >> static bool spapr_irq_test(XICSFabric *xi, int irq)
> >> {
> >> sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >> - int srcno = irq - spapr->ics->offset;
> >> + int srcno = irq - spapr->irq_base;
> >>
> >> return test_bit(srcno, spapr->irq_map);
> >> }
> >> @@ -3656,13 +3657,13 @@ static int spapr_irq_alloc_block(XICSFabric *xi,
> >> int count, int align)
> >> }
> >>
> >> bitmap_set(spapr->irq_map, srcno, count);
> >> - return srcno + spapr->ics->offset;
> >> + return srcno + spapr->irq_base;
> >> }
> >>
> >> static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
> >> {
> >> sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >> - int srcno = irq - spapr->ics->offset;
> >> + int srcno = irq - spapr->irq_base;
> >>
> >> bitmap_clear(spapr->irq_map, srcno, num);
> >> }
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 023436c32b2a..200667dcff9d 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -82,6 +82,7 @@ struct sPAPRMachineState {
> >> int32_t nr_irqs;
> >> unsigned long *irq_map;
> >> unsigned long *irq_map_ref;
> >> + uint32_t irq_base;
> >> ICSState *ics;
> >> sPAPRRTCState rtc;
> >>
> >
>
