On Fri, 17 Nov 2017 15:50:53 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> 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 <c...@kaod.org> 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 <c...@kaod.org>
> > > ---
> > >
> > > 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.
>
Is this true for MSIs ? With the current code, the guest can change
the allocation of such interrupts with the ibm,rtas-change-msi RTAS
call. How can the dest know about that ?
> >
> > > +}
> > > +
> > > +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;
> > >
> >
>
pgpPgCLTLu8TC.pgp
Description: OpenPGP digital signature
