| Message ID | 20171110152017.24324-8-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: introduce an IRQ allocator at the machine level | expand |
On Fri, 10 Nov 2017 15:20:13 +0000 Cédric Le Goater <clg@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 <clg@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 ? 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; >
On 11/14/2017 03:45 PM, Greg Kurz wrote: > On Fri, 10 Nov 2017 15:20:13 +0000 > Cédric Le Goater <clg@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 <clg@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. 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. 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; >> >
On Wed, 15 Nov 2017 15:24:08 +0000 Cédric Le Goater <clg@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 <clg@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 <clg@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; > >> > > >
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; /* 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;
'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 <clg@kaod.org> --- hw/ppc/spapr.c | 7 ++++--- include/hw/ppc/spapr.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)