Re: [Qemu-devel] [PATCH for-2.12 v3 07/11] spapr: introduce an 'irq_base' number

[Greg Kurz]

On Wed, 15 Nov 2017 15:24:08 +0000
Cédric Le Goater <address@hidden> wrote:

> On 11/14/2017 03:45 PM, Greg Kurz wrote:
> > On Fri, 10 Nov 2017 15:20:13 +0000
> > Cédric Le Goater <address@hidden> 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 <address@hidden>
> >> ---
> >>  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;
> >>    
> >   
>