On 06/13/2018 07:00 AM, David Gibson wrote: > On Fri, May 18, 2018 at 06:44:04PM +0200, Cédric Le Goater wrote: >> This proposal moves all the related IRQ routines of the sPAPR machine >> behind a class interface to prepare for future changes in the IRQ >> controller model. First of which is a reorganization of the IRQ number >> space layout and a second, coming later, will be to integrate the >> support for the new POWER9 XIVE IRQ controller. >> >> The new interface defines a set of fixed IRQ number ranges, for each >> IRQ type, in which devices allocate the IRQ numbers they need >> depending on a unique device index. Here is the layout : >> >> SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */ >> SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */ >> SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */ >> SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */ >> SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */ >> SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device */ >> >> The IPI range is reserved for future use when XIVE support >> comes in. >> >> The routines of this interface encompass the previous needs and the >> new ones and seem complex but the provided IRQ backend should >> implement what we have today without any functional changes. >> >> Each device model is modified to take the new interface into account >> using the IRQ range/type definitions and a device index. A part from >> the VIO devices, lacking an id, the changes are relatively simple. > > I find your use of "back end" vs. "front end" in this patch and the > next kind of confusing.
This is the the front end, interface used by the machine and devices : int spapr_irq_assign(sPAPRMachineState *spapr, uint32_t range, uint32_t irq, Error **errp); int spapr_irq_alloc(sPAPRMachineState *spapr, uint32_t range, uint32_t index, Error **errp); int spapr_irq_alloc_block(sPAPRMachineState *spapr, uint32_t range, uint32_t index, int num, bool align, Error **errp); void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num, Error **errp); qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq); and the backend, which can be different depending on the machine level, old vs. new layout, or on depending on the interrupt controller. typedef struct sPAPRIrq { uint32_t nr_irqs; const sPAPRPIrqRange *ranges; void (*init)(sPAPRMachineState *spapr, Error **errp); int (*assign)(sPAPRMachineState *spapr, uint32_t range, uint32_t irq, Error **errp); int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index, Error **errp); int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range, uint32_t index, int num, bool align, Error **errp); void (*free)(sPAPRMachineState *spapr, int irq, int num, Error **errp); qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq); void (*print_info)(sPAPRMachineState *spapr, Monitor *mon); } sPAPRIrq; >> Signed-off-by: Cédric Le Goater <c...@kaod.org> >> --- >> include/hw/ppc/spapr.h | 10 +- >> include/hw/ppc/spapr_irq.h | 46 +++++++++ >> hw/ppc/spapr.c | 177 +--------------------------------- >> hw/ppc/spapr_events.c | 7 +- >> hw/ppc/spapr_irq.c | 233 >> +++++++++++++++++++++++++++++++++++++++++++++ >> hw/ppc/spapr_pci.c | 21 +++- >> hw/ppc/spapr_vio.c | 5 +- >> hw/ppc/Makefile.objs | 2 +- >> 8 files changed, 308 insertions(+), 193 deletions(-) >> create mode 100644 include/hw/ppc/spapr_irq.h >> create mode 100644 hw/ppc/spapr_irq.c >> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 2cfdfdd67eaf..4eb212b16a51 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -3,10 +3,10 @@ >> >> #include "sysemu/dma.h" >> #include "hw/boards.h" >> -#include "hw/ppc/xics.h" >> #include "hw/ppc/spapr_drc.h" >> #include "hw/mem/pc-dimm.h" >> #include "hw/ppc/spapr_ovec.h" >> +#include "hw/ppc/spapr_irq.h" >> >> struct VIOsPAPRBus; >> struct sPAPRPHBState; >> @@ -104,6 +104,7 @@ struct sPAPRMachineClass { >> unsigned n_dma, uint32_t *liobns, Error **errp); >> sPAPRResizeHPT resize_hpt_default; >> sPAPRCapabilities default_caps; >> + sPAPRIrq *irq; >> }; >> >> /** >> @@ -773,13 +774,6 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu); >> void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp); >> PowerPCCPU *spapr_find_cpu(int vcpu_id); >> >> -int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp); >> -int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi, >> - bool align, Error **errp); >> -void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num); >> -qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq); >> - >> - >> int spapr_caps_pre_load(void *opaque); >> int spapr_caps_pre_save(void *opaque); >> >> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h >> new file mode 100644 >> index 000000000000..caf4c33d4cec >> --- /dev/null >> +++ b/include/hw/ppc/spapr_irq.h >> @@ -0,0 +1,46 @@ >> +/* >> + * QEMU PowerPC sPAPR IRQ backend definitions >> + * >> + * Copyright (c) 2018, IBM Corporation. >> + * >> + * This code is licensed under the GPL version 2 or later. See the >> + * COPYING file in the top-level directory. >> + */ >> +#ifndef HW_SPAPR_IRQ_H >> +#define HW_SPAPR_IRQ_H >> + >> +#include "hw/ppc/xics.h" >> + >> +/* >> + * IRQ ranges >> + */ >> +#define SPAPR_IRQ_IPI 0x0 /* 1 IRQ per CPU */ >> +#define SPAPR_IRQ_EPOW 0x1000 /* 1 IRQ per device */ >> +#define SPAPR_IRQ_HOTPLUG 0x1001 /* 1 IRQ per device */ >> +#define SPAPR_IRQ_VIO 0x1100 /* 1 IRQ per device */ >> +#define SPAPR_IRQ_PCI_LSI 0x1200 /* 4 IRQs per device */ >> +#define SPAPR_IRQ_PCI_MSI 0x1400 /* 1K IRQs per device covered by >> + * a bitmap allocator */ > > These look like they belong in the next patch with the fixed allocations. They act as ids also, can be discussed. >> +typedef struct sPAPRIrq { > > Much of this structure doesn't make sense to me. AIUI, the idea is > that this method structure will vary with the xics vs. xive backend, > yes? This is not only XIVE. the static allocation scheme changes all the backend. > Comments below are based on that assumption. > >> + uint32_t nr_irqs; >> + >> + void (*init)(sPAPRMachineState *spapr, Error **errp); >> + int (*alloc)(sPAPRMachineState *spapr, uint32_t range, uint32_t index, >> + Error **errp); > > 'range' has no place here - working out the indices should be entirely > on the spapr side, only the final irq number should need to go to the > backend. RAnge identifies the device, see above. Maybe badly named. > I'd also prefer to call it "claim" to distinguish it from the existing > "alloc" function which finds a free irq as well as setting it up for > our exclusive use. ... >> + int (*alloc_block)(sPAPRMachineState *spapr, uint32_t range, >> + uint32_t index, int num, bool align, Error **errp); > > There should be no need for this. We needed an alloc_block routine > before, because we wanted to ensure that we got a contiguous (and > maybe aligned) block of irqs. By the time we go to the backend we > should already have absolute irq numbers, so we can just claim each of > them separately. ... So, David, let's stop wasting our time. please provide what you feel is right and I will build on top of it. For your information, please understand that I generally make sure that a patchset fits older and newer machines, that migration is tested and that the XIVE patchset is resynced. It takes a HUGE amount of time and it's a not a ramdom idea that just looks nice. Thanks, C.