Re: [Qemu-devel] [PATCH for-2.12 v3 05/11] spapr: introduce an IRQ allocator using a bitmap

[David Gibson]

On Tue, Nov 14, 2017 at 10:42:24AM +0100, Greg Kurz wrote:
> On Fri, 10 Nov 2017 15:20:11 +0000
> Cédric Le Goater <address@hidden> wrote:
> 
> > Let's define a new set of XICSFabric IRQ operations for the latest
> > pseries machine. These simply use a a bitmap 'irq_map' as a IRQ number
> > allocator.
> > 
> > The previous pseries machines keep the old set of IRQ operations using
> > the ICSIRQState array.
> > 
> > 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/ppc/spapr.c         | 76 
> > ++++++++++++++++++++++++++++++++++++++++++++++----
> >  include/hw/ppc/spapr.h |  3 ++
> >  2 files changed, 74 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 4bdceb45a14f..4ef0b73559ca 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1681,6 +1681,22 @@ static const VMStateDescription 
> > vmstate_spapr_patb_entry = {
> >      },
> >  };
> >  
> > +static bool spapr_irq_map_needed(void *opaque)
> > +{
> > +    return true;
> 
> I see that the next patch adds some code to avoid sending the
> bitmap if it doesn't contain state, but I guess you should also
> explicitly have this function to return false for older machine
> types (see remark below).

I don't see that you should need to migrate this at all.  The machine
needs to reliably allocate the same interrupts each time, and that
means source and dest should have the same allocations without
migrating data.

> 
> > +}
> > +
> > +static const VMStateDescription vmstate_spapr_irq_map = {
> > +    .name = "spapr_irq_map",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .needed = spapr_irq_map_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_BITMAP(irq_map, sPAPRMachineState, 0, nr_irqs),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  static const VMStateDescription vmstate_spapr = {
> >      .name = "spapr",
> >      .version_id = 3,
> > @@ -1700,6 +1716,7 @@ static const VMStateDescription vmstate_spapr = {
> >          &vmstate_spapr_ov5_cas,
> >          &vmstate_spapr_patb_entry,
> >          &vmstate_spapr_pending_events,
> > +        &vmstate_spapr_irq_map,
> >          NULL
> >      }
> >  };
> > @@ -2337,8 +2354,12 @@ static void ppc_spapr_init(MachineState *machine)
> >      /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
> >      load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> >  
> > +    /* Initialize the IRQ allocator */
> > +    spapr->nr_irqs  = XICS_IRQS_SPAPR;
> > +    spapr->irq_map  = bitmap_new(spapr->nr_irqs);
> > +
> 
> I think you should introduce a sPAPRMachineClass::has_irq_bitmap boolean
> so that the bitmap is only allocated for newer machine types. And you should
> then use this flag in spapr_irq_map_needed() above.
> 
> Apart from that, the rest of the patch looks good.
> 
> >      /* Set up Interrupt Controller before we create the VCPUs */
> > -    xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
> > +    xics_system_init(machine, spapr->nr_irqs, &error_fatal);
> >  
> >      /* Set up containers for ibm,client-architecture-support negotiated 
> > options
> >       */
> > @@ -3560,7 +3581,7 @@ static int ics_find_free_block(ICSState *ics, int 
> > num, int alignnum)
> >      return -1;
> >  }
> >  
> > -static bool spapr_irq_test(XICSFabric *xi, int irq)
> > +static bool spapr_irq_test_2_11(XICSFabric *xi, int irq)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >      ICSState *ics = spapr->ics;
> > @@ -3569,7 +3590,7 @@ static bool spapr_irq_test(XICSFabric *xi, int irq)
> >      return !ICS_IRQ_FREE(ics, srcno);
> >  }
> >  
> > -static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
> > +static int spapr_irq_alloc_block_2_11(XICSFabric *xi, int count, int align)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >      ICSState *ics = spapr->ics;
> > @@ -3583,7 +3604,7 @@ static int spapr_irq_alloc_block(XICSFabric *xi, int 
> > count, int align)
> >      return srcno + ics->offset;
> >  }
> >  
> > -static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
> > +static void spapr_irq_free_block_2_11(XICSFabric *xi, int irq, int num)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> >      ICSState *ics = spapr->ics;
> > @@ -3601,6 +3622,46 @@ static void spapr_irq_free_block(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;
> > +
> > +    return test_bit(srcno, spapr->irq_map);
> > +}
> > +
> > +static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> > +    int start = 0;
> > +    int srcno;
> > +
> > +    /*
> > +     * The 'align_mask' parameter of bitmap_find_next_zero_area()
> > +     * should be one less than a power of 2; 0 means no
> > +     * alignment. Adapt the 'align' value of the former allocator to
> > +     * fit the requirements of bitmap_find_next_zero_area()
> > +     */
> > +    align -= 1;
> > +
> > +    srcno = bitmap_find_next_zero_area(spapr->irq_map, spapr->nr_irqs, 
> > start,
> > +                                       count, align);
> > +    if (srcno == spapr->nr_irqs) {
> > +        return -1;
> > +    }
> > +
> > +    bitmap_set(spapr->irq_map, srcno, count);
> > +    return srcno + spapr->ics->offset;
> > +}
> > +
> > +static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
> > +{
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
> > +    int srcno = irq - spapr->ics->offset;
> > +
> > +    bitmap_clear(spapr->irq_map, srcno, num);
> > +}
> > +
> >  static void spapr_pic_print_info(InterruptStatsProvider *obj,
> >                                   Monitor *mon)
> >  {
> > @@ -3778,7 +3839,12 @@ static void 
> > spapr_machine_2_11_instance_options(MachineState *machine)
> >  
> >  static void spapr_machine_2_11_class_options(MachineClass *mc)
> >  {
> > -    /* Defaults for the latest behaviour inherited from the base class */
> > +    XICSFabricClass *xic = XICS_FABRIC_CLASS(mc);
> > +
> > +    spapr_machine_2_12_class_options(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;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(2_11, "2.11", false);
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 9d21ca9bde3a..5835c694caff 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -7,6 +7,7 @@
> >  #include "hw/ppc/spapr_drc.h"
> >  #include "hw/mem/pc-dimm.h"
> >  #include "hw/ppc/spapr_ovec.h"
> > +#include "qemu/bitmap.h"
> >  
> >  struct VIOsPAPRBus;
> >  struct sPAPRPHBState;
> > @@ -78,6 +79,8 @@ struct sPAPRMachineState {
> >      struct VIOsPAPRBus *vio_bus;
> >      QLIST_HEAD(, sPAPRPHBState) phbs;
> >      struct sPAPRNVRAM *nvram;
> > +    int32_t nr_irqs;
> > +    unsigned long *irq_map;
> >      ICSState *ics;
> >      sPAPRRTCState rtc;
> >  
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature