Re: [Qemu-devel] [PATCH for-2.12 v3 09/11] spapr: split the IRQ number space for LSI interrupts

[Greg Kurz]

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

> On 11/15/2017 03:52 PM, Greg Kurz wrote:
> > On Fri, 10 Nov 2017 15:20:15 +0000
> > Cédric Le Goater <address@hidden> wrote:
> >   
> >> The type of an interrupt, MSI or LSI, is stored under the flag
> >> attribute of the ICSIRQState array. To reduce the use of this array
> >> and consequently of the ICSState object (This is needed to introduce
> >> the new XIVE model), we choose to split the IRQ number space of the
> >> machine in two: first the LSIs and then the MSIs.
> >>
> >> This also has the benefit to keep the LSI IRQ numbers in a well known
> >> range which will be useful for PHB hotplug.
> >>  
> > 
> > Well... LSIs indeed land in a well known range, but it isn't enough for PHB
> > hotplug. Each PHB is uniquely identified by its 'index' property, and we
> > want each PHB to have fixed LSIs, so that they are invariant across 
> > migration.  
> 
> ok. 
> 
> So, as said in another email, we should think about segmenting
> the allocation per device. At least for PHBs. This is specified in
> the PAPR specs, each device has one or more Bus Unit IDentifier (BUID)

BTW, the code currently uses "BUID" to refer to the PHB Unit ID which
is a different concept in the PAPR spec. Maybe this should be fixed
for the sake of clarity ?

> acting as a prefix for the IRQ number.
> 

Since the user can instantiate multiple VIO devices, should we also
have an irq segment for the VIO bus as well ?

> We could model that by using a specific range for each PHB in the 
> overall IRQ number space, depending on some index. LSIs would be 

Some index should be the "index" property I was mentioning before.

> allocated at the beginning of this range, when the device is realized 
> and MSIs later on when the guest starts.  
> 
> Identifying a LSI could be done using a mask on the IRQ number range 
> of each PHB. It should be fast enough. I don't see other devices 
> using LSIs under the sPAPR platform.
> 

There aren't any other AFAICT.

> 
> C.
> 
> 
> 
> >> This change only applies to the latest pseries machines. Older
> >> machines still use the ICSIRQState array to define the IRQ type.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>
> >>  Changes since v2 :
> >>
> >>  - introduced a second set of XICSFabric IRQ operations for older
> >>    pseries machines
> >>
> >>  hw/intc/xics_spapr.c  |  6 +++---
> >>  hw/ppc/spapr.c        | 33 +++++++++++++++++++++++++++++----
> >>  include/hw/ppc/xics.h |  2 +-
> >>  3 files changed, 33 insertions(+), 8 deletions(-)pe-total-#msi
> >>
> >> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> >> index de9e65d35247..b8e91aaf52bd 100644
> >> --- a/hw/intc/xics_spapr.c
> >> +++ b/hw/intc/xics_spapr.c
> >> @@ -260,7 +260,7 @@ int spapr_ics_alloc(ICSState *ics, int irq_hint, bool 
> >> lsi, Error **errp)
> >>          }
> >>          irq = irq_hint;
> >>      } else {
> >> -        irq = xic->irq_alloc_block(ics->xics, 1, 1);
> >> +        irq = xic->irq_alloc_block(ics->xics, 1, 1, lsi);
> >>          if (irq < 0) {
> >>              error_setg(errp, "can't allocate IRQ: no IRQ left");
> >>              return -1;
> >> @@ -297,9 +297,9 @@ int spapr_ics_alloc_block(ICSState *ics, int num, bool 
> >> lsi,
> >>      if (align) {
> >>          assert((num == 1) || (num == 2) || (num == 4) ||
> >>                 (num == 8) || (num == 16) || (num == 32));
> >> -        first = xic->irq_alloc_block(ics->xics, num, num);
> >> +        first = xic->irq_alloc_block(ics->xics, num, num, lsi);
> >>      } else {
> >> -        first = xic->irq_alloc_block(ics->xics, num, 1);
> >> +        first = xic->irq_alloc_block(ics->xics, num, 1, lsi);
> >>      }
> >>      if (first < 0) {
> >>          error_setg(errp, "can't find a free %d-IRQ block", num);
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index ce314fcf38db..f14eae6196cd 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3596,7 +3596,8 @@ static bool spapr_irq_test_2_11(XICSFabric *xi, int 
> >> irq)
> >>      return !ICS_IRQ_FREE(ics, srcno);
> >>  }
> >>  
> >> -static int spapr_irq_alloc_block_2_11(XICSFabric *xi, int count, int 
> >> align)
> >> +static int spapr_irq_alloc_block_2_11(XICSFabric *xi, int count, int 
> >> align,
> >> +                                      bool lsi)
> >>  {
> >>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >>      ICSState *ics = spapr->ics;
> >> @@ -3628,7 +3629,7 @@ static void spapr_irq_free_block_2_11(XICSFabric 
> >> *xi, int irq, int num)
> >>      }
> >>  }
> >>  
> >> -static bool spapr_irq_is_lsi(XICSFabric *xi, int irq)
> >> +static bool spapr_irq_is_lsi_2_11(XICSFabric *xi, int irq)
> >>  {
> >>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >>      int srcno = irq - spapr->ics->offset;
> >> @@ -3644,10 +3645,21 @@ static bool spapr_irq_test(XICSFabric *xi, int irq)
> >>      return test_bit(srcno, spapr->irq_map);
> >>  }
> >>  
> >> -static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
> >> +
> >> +/*
> >> + * Let's provision 4 LSIs per PHBs
> >> + */
> >> +#define SPAPR_MAX_LSI (SPAPR_MAX_PHBS * 4)
> >> +
> >> +/*
> >> + * Split the IRQ number space of the machine in two: first the LSIs
> >> + * and then the MSIs. This allows us to keep the LSI IRQ numbers in a
> >> + * well known range which is useful for PHB hotplug.
> >> + */
> >> +static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align, 
> >> bool lsi)
> >>  {
> >>      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >> -    int start = 0;
> >> +    int start = lsi ? 0 : SPAPR_MAX_LSI;
> >>      int srcno;
> >>  
> >>      /*
> >> @@ -3664,6 +3676,10 @@ static int spapr_irq_alloc_block(XICSFabric *xi, 
> >> int count, int align)
> >>          return -1;
> >>      }
> >>  
> >> +    if (lsi && srcno >= SPAPR_MAX_LSI) {
> >> +        return -1;
> >> +    }
> >> +
> >>      bitmap_set(spapr->irq_map, srcno, count);
> >>      return srcno + spapr->irq_base;
> >>  }
> >> @@ -3676,6 +3692,14 @@ static void spapr_irq_free_block(XICSFabric *xi, 
> >> int irq, int num)
> >>      bitmap_clear(spapr->irq_map, srcno, num);
> >>  }
> >>  
> >> +static bool spapr_irq_is_lsi(XICSFabric *xi, int irq)
> >> +{
> >> +    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >> +    int srcno = irq - spapr->irq_base;
> >> +
> >> +    return (srcno >= 0) && (srcno < SPAPR_MAX_LSI);
> >> +}
> >> +
> >>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
> >>                                   Monitor *mon)
> >>  {
> >> @@ -3860,6 +3884,7 @@ static void 
> >> spapr_machine_2_11_class_options(MachineClass *mc)
> >>      xic->irq_test = spapr_irq_test_2_11;
> >>      xic->irq_alloc_block = spapr_irq_alloc_block_2_11;
> >>      xic->irq_free_block = spapr_irq_free_block_2_11;
> >> +    xic->irq_is_lsi = spapr_irq_is_lsi_2_11;
> >>  }
> >>  
> >>  DEFINE_SPAPR_MACHINE(2_11, "2.11", false);
> >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >> index 478f8e510179..292b929e88eb 100644
> >> --- a/include/hw/ppc/xics.h
> >> +++ b/include/hw/ppc/xics.h
> >> @@ -177,7 +177,7 @@ typedef struct XICSFabricClass {
> >>      ICPState *(*icp_get)(XICSFabric *xi, int server);
> >>      /* IRQ allocator helpers */
> >>      bool (*irq_test)(XICSFabric *xi, int irq);
> >> -    int (*irq_alloc_block)(XICSFabric *xi, int count, int align);
> >> +    int (*irq_alloc_block)(XICSFabric *xi, int count, int align, bool 
> >> lsi);
> >>      void (*irq_free_block)(XICSFabric *xi, int irq, int num);
> >>      bool (*irq_is_lsi)(XICSFabric *xi, int irq);
> >>  } XICSFabricClass;  
> >   
>