| Message ID | 20170911171235.29331-2-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | Guest exploitation of the XIVE interrupt controller (POWER9) | expand |
On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: > Start with a couple of attributes for the XIVE sPAPR controller > model. The number of provisionned IRQ is necessary to size the > different internal XIVE tables, the number of CPUs is also. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> [snip] > +static void spapr_xive_realize(DeviceState *dev, Error **errp) > +{ > + sPAPRXive *xive = SPAPR_XIVE(dev); > + > + if (!xive->nr_targets) { > + error_setg(errp, "Number of interrupt targets needs to be greater 0"); > + return; > + } > + /* We need to be able to allocate at least the IPIs */ > + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { > + error_setg(errp, "Number of interrupts too small"); > + return; > + } > +} > + > +static Property spapr_xive_properties[] = { > + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), > + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), I'm a bit uneasy about the number of targets having to be set in advance: this can make life awkward when CPUs are hotplugged. I know there's something similar in xics, but it has caused some hassles, and we're starting to move away from it. Do you really need this?
On 09/19/2017 04:27 AM, David Gibson wrote: > On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: >> Start with a couple of attributes for the XIVE sPAPR controller >> model. The number of provisionned IRQ is necessary to size the >> different internal XIVE tables, the number of CPUs is also. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > [snip] > >> +static void spapr_xive_realize(DeviceState *dev, Error **errp) >> +{ >> + sPAPRXive *xive = SPAPR_XIVE(dev); >> + >> + if (!xive->nr_targets) { >> + error_setg(errp, "Number of interrupt targets needs to be greater 0"); >> + return; >> + } >> + /* We need to be able to allocate at least the IPIs */ >> + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { >> + error_setg(errp, "Number of interrupts too small"); >> + return; >> + } >> +} >> + >> +static Property spapr_xive_properties[] = { >> + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), >> + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), > > I'm a bit uneasy about the number of targets having to be set in > advance: this can make life awkward when CPUs are hotplugged. I know > there's something similar in xics, but it has caused some hassles, and > we're starting to move away from it. > > Do you really need this? > Some of the internal table size depend on the number of cpus defined for the machine. When the sPAPRXive object is instantiated, we use xics_max_server_number() to get the max number of cpus provisioned. C.
On Tue, Sep 19, 2017 at 03:15:44PM +0200, Cédric Le Goater wrote: > On 09/19/2017 04:27 AM, David Gibson wrote: > > On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: > >> Start with a couple of attributes for the XIVE sPAPR controller > >> model. The number of provisionned IRQ is necessary to size the > >> different internal XIVE tables, the number of CPUs is also. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > [snip] > > > >> +static void spapr_xive_realize(DeviceState *dev, Error **errp) > >> +{ > >> + sPAPRXive *xive = SPAPR_XIVE(dev); > >> + > >> + if (!xive->nr_targets) { > >> + error_setg(errp, "Number of interrupt targets needs to be greater 0"); > >> + return; > >> + } > >> + /* We need to be able to allocate at least the IPIs */ > >> + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { > >> + error_setg(errp, "Number of interrupts too small"); > >> + return; > >> + } > >> +} > >> + > >> +static Property spapr_xive_properties[] = { > >> + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), > >> + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), > > > > I'm a bit uneasy about the number of targets having to be set in > > advance: this can make life awkward when CPUs are hotplugged. I know > > there's something similar in xics, but it has caused some hassles, and > > we're starting to move away from it. > > > > Do you really need this? > > > > Some of the internal table size depend on the number of cpus > defined for the machine. Which ones? My impression was that there needed to be at least #cpus * #priority-levels EQs, but there could be more than that, so it was no longer as tightly bound to the number if "interrupt servers" as xics. > When the sPAPRXive object is instantiated, > we use xics_max_server_number() to get the max number of cpus > provisioned. > > C. >
On 09/22/2017 01:00 PM, David Gibson wrote: > On Tue, Sep 19, 2017 at 03:15:44PM +0200, Cédric Le Goater wrote: >> On 09/19/2017 04:27 AM, David Gibson wrote: >>> On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: >>>> Start with a couple of attributes for the XIVE sPAPR controller >>>> model. The number of provisionned IRQ is necessary to size the >>>> different internal XIVE tables, the number of CPUs is also. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> >>> [snip] >>> >>>> +static void spapr_xive_realize(DeviceState *dev, Error **errp) >>>> +{ >>>> + sPAPRXive *xive = SPAPR_XIVE(dev); >>>> + >>>> + if (!xive->nr_targets) { >>>> + error_setg(errp, "Number of interrupt targets needs to be greater 0"); >>>> + return; >>>> + } >>>> + /* We need to be able to allocate at least the IPIs */ >>>> + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { >>>> + error_setg(errp, "Number of interrupts too small"); >>>> + return; >>>> + } >>>> +} >>>> + >>>> +static Property spapr_xive_properties[] = { >>>> + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), >>>> + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), >>> >>> I'm a bit uneasy about the number of targets having to be set in >>> advance: this can make life awkward when CPUs are hotplugged. I know >>> there's something similar in xics, but it has caused some hassles, and >>> we're starting to move away from it. >>> >>> Do you really need this? >>> >> >> Some of the internal table size depend on the number of cpus >> defined for the machine. > > Which ones? My impression was that there needed to be at least #cpus > * #priority-levels EQs, but there could be more than that, euh no, not in spapr mode at least. There are 8 queues per cpu. > so it was no longer as tightly bound to the number if "interrupt servers"> as xics. ah. I think I see what you mean, that we could allocate them on the fly when needed by some hcalls ? The other place where I use the nr_targets is to provision the IRQ numbers for the IPIs but that could probably be done in some other way, specially it there is a IRQ allocator at the machine level. C. >> When the sPAPRXive object is instantiated, >> we use xics_max_server_number() to get the max number of cpus >> provisioned. >> >> C. >> >
On Fri, Sep 22, 2017 at 02:42:07PM +0200, Cédric Le Goater wrote: > On 09/22/2017 01:00 PM, David Gibson wrote: > > On Tue, Sep 19, 2017 at 03:15:44PM +0200, Cédric Le Goater wrote: > >> On 09/19/2017 04:27 AM, David Gibson wrote: > >>> On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: > >>>> Start with a couple of attributes for the XIVE sPAPR controller > >>>> model. The number of provisionned IRQ is necessary to size the > >>>> different internal XIVE tables, the number of CPUs is also. > >>>> > >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >>> > >>> [snip] > >>> > >>>> +static void spapr_xive_realize(DeviceState *dev, Error **errp) > >>>> +{ > >>>> + sPAPRXive *xive = SPAPR_XIVE(dev); > >>>> + > >>>> + if (!xive->nr_targets) { > >>>> + error_setg(errp, "Number of interrupt targets needs to be greater 0"); > >>>> + return; > >>>> + } > >>>> + /* We need to be able to allocate at least the IPIs */ > >>>> + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { > >>>> + error_setg(errp, "Number of interrupts too small"); > >>>> + return; > >>>> + } > >>>> +} > >>>> + > >>>> +static Property spapr_xive_properties[] = { > >>>> + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), > >>>> + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), > >>> > >>> I'm a bit uneasy about the number of targets having to be set in > >>> advance: this can make life awkward when CPUs are hotplugged. I know > >>> there's something similar in xics, but it has caused some hassles, and > >>> we're starting to move away from it. > >>> > >>> Do you really need this? > >>> > >> > >> Some of the internal table size depend on the number of cpus > >> defined for the machine. > > > > Which ones? My impression was that there needed to be at least #cpus > > * #priority-levels EQs, but there could be more than that, > > euh no, not in spapr mode at least. There are 8 queues per cpu. Ok. > > so it was no longer as tightly bound to the number if "interrupt servers"> as xics. > > ah. I think I see what you mean, that we could allocate them on the > fly when needed by some hcalls ? Not at hcall time, no, but at cpu hot(un)plug time I was wondering if we could (de)allocate them then. > The other place where I use the nr_targets is to provision the > IRQ numbers for the IPIs but that could probably be done in some > other way, specially it there is a IRQ allocator at the machine > level. Hm, ok. > > C. > >> When the sPAPRXive object is instantiated, > >> we use xics_max_server_number() to get the max number of cpus > >> provisioned. > >> > >> C. > >> > > >
On Tue, 2017-09-26 at 13:54 +1000, David Gibson wrote: > > > > > > Which ones? My impression was that there needed to be at least #cpus > > > * #priority-levels EQs, but there could be more than that, > > > > euh no, not in spapr mode at least. There are 8 queues per cpu. > > Ok. There's a HW feature of XIVE in DD2.x that I will start exploiting soon that sacrifices a queue btw, keep that in mind. We should probably only expose 0...6 to guests, not 0...7. > > > so it was no longer as tightly bound to the number if "interrupt servers"> as xics. > > > > ah. I think I see what you mean, that we could allocate them on the > > fly when needed by some hcalls ? > > Not at hcall time, no, but at cpu hot(un)plug time I was wondering if we > could (de)allocate them then. > > > The other place where I use the nr_targets is to provision the > > IRQ numbers for the IPIs but that could probably be done in some > > other way, specially it there is a IRQ allocator at the machine > > level. > > Hm, ok. >
On 09/26/2017 05:54 AM, David Gibson wrote: > On Fri, Sep 22, 2017 at 02:42:07PM +0200, Cédric Le Goater wrote: >> On 09/22/2017 01:00 PM, David Gibson wrote: >>> On Tue, Sep 19, 2017 at 03:15:44PM +0200, Cédric Le Goater wrote: >>>> On 09/19/2017 04:27 AM, David Gibson wrote: >>>>> On Mon, Sep 11, 2017 at 07:12:15PM +0200, Cédric Le Goater wrote: >>>>>> Start with a couple of attributes for the XIVE sPAPR controller >>>>>> model. The number of provisionned IRQ is necessary to size the >>>>>> different internal XIVE tables, the number of CPUs is also. >>>>>> >>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>> >>>>> [snip] >>>>> >>>>>> +static void spapr_xive_realize(DeviceState *dev, Error **errp) >>>>>> +{ >>>>>> + sPAPRXive *xive = SPAPR_XIVE(dev); >>>>>> + >>>>>> + if (!xive->nr_targets) { >>>>>> + error_setg(errp, "Number of interrupt targets needs to be greater 0"); >>>>>> + return; >>>>>> + } >>>>>> + /* We need to be able to allocate at least the IPIs */ >>>>>> + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { >>>>>> + error_setg(errp, "Number of interrupts too small"); >>>>>> + return; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +static Property spapr_xive_properties[] = { >>>>>> + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), >>>>>> + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), >>>>> >>>>> I'm a bit uneasy about the number of targets having to be set in >>>>> advance: this can make life awkward when CPUs are hotplugged. I know >>>>> there's something similar in xics, but it has caused some hassles, and >>>>> we're starting to move away from it. >>>>> >>>>> Do you really need this? >>>>> >>>> >>>> Some of the internal table size depend on the number of cpus >>>> defined for the machine. >>> >>> Which ones? My impression was that there needed to be at least #cpus >>> * #priority-levels EQs, but there could be more than that, >> >> euh no, not in spapr mode at least. There are 8 queues per cpu. > > Ok. > >>> so it was no longer as tightly bound to the number if "interrupt servers"> as xics. >> >> ah. I think I see what you mean, that we could allocate them on the >> fly when needed by some hcalls ? > > Not at hcall time, no, but at cpu hot(un)plug time I was wondering if we > could (de)allocate them then. Yes. I am currently reshuffling the ppc/xive patchset on top of the sPAPR allocator, also trying to take into account your comments and Ben's. And I think we can do that. I have introduced a specific XiveICPState (stored under the ->intc pointer of the CPUState) but I think we can go a little further and also store the XiveEQ array there. So that all the state related to a CPU would be allocated at init time or hot(un)plug time hot time. We could call the resulting compound object (ICP + EQs) a sPAPRXiveCore or something like that. And we won't need to keep 'nr_targets' around the sPAPR Xive object anymore *if* we do one more thing. See below. >> The other place where I use the nr_targets is to provision the >> IRQ numbers for the IPIs but that could probably be done in some >> other way, specially it there is a IRQ allocator at the machine >> level. > > Hm, ok. The sPAPRXiveCore object above could also allocate the IPI for XIVE when it is created. But, the resulting IRQ number should be in a range not overlapping with the other devices' IRQs. That means that we should probably introduce the BUID concept in the IRQ allocator. This concept was left out from the original design of XICS as it was using a single ICS but it would be useful to define ranges for devices now. for PHBs for instance. We could start with 2K or 4K ranges. The first one would be for IPIs. Dave, could you please take a quick look at the allocator patchset when you have some time. I don't think there is much to do in the API to introduce ranges. It is just a question of defining a 'start' value when looking for an empty slot. Thanks, C. >> >> C. >>>> When the sPAPRXive object is instantiated, >>>> we use xics_max_server_number() to get the max number of cpus >>>> provisioned. >>>> >>>> C. >>>> >>> >> >
On 09/26/2017 11:45 AM, Benjamin Herrenschmidt wrote: > On Tue, 2017-09-26 at 13:54 +1000, David Gibson wrote: >>>> >>>> Which ones? My impression was that there needed to be at least #cpus >>>> * #priority-levels EQs, but there could be more than that, >>> >>> euh no, not in spapr mode at least. There are 8 queues per cpu. >> >> Ok. > > There's a HW feature of XIVE in DD2.x that I will start exploiting soon > that sacrifices a queue btw, keep that in mind. > > We should probably only expose 0...6 to guests, not 0...7. yes. This is achieved using the "ibm,plat-res-int-priorities" property of the device tree. I don't think we also need to check these priority ranges when allocating the EQs. We can just allocate them all (8 priorities) and let the hcalls do the checks. They should return H_P3 or H_P4 if the prio is invalid. C.
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak index 46c95993217d..8294df31c0f5 100644 --- a/default-configs/ppc64-softmmu.mak +++ b/default-configs/ppc64-softmmu.mak @@ -56,6 +56,7 @@ CONFIG_SM501=y CONFIG_XICS=$(CONFIG_PSERIES) CONFIG_XICS_SPAPR=$(CONFIG_PSERIES) CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM)) +CONFIG_XIVE_SPAPR=$(CONFIG_PSERIES) # For PReP CONFIG_SERIAL_ISA=y CONFIG_MC146818RTC=y diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs index 78426a7dafcd..2dae80bdf611 100644 --- a/hw/intc/Makefile.objs +++ b/hw/intc/Makefile.objs @@ -35,6 +35,7 @@ obj-$(CONFIG_SH4) += sh_intc.o obj-$(CONFIG_XICS) += xics.o obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o obj-$(CONFIG_XICS_KVM) += xics_kvm.o +obj-$(CONFIG_XIVE_SPAPR) += spapr_xive.o obj-$(CONFIG_POWERNV) += xics_pnv.o obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o obj-$(CONFIG_S390_FLIC) += s390_flic.o diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c new file mode 100644 index 000000000000..c83796519586 --- /dev/null +++ b/hw/intc/spapr_xive.c @@ -0,0 +1,76 @@ +/* + * QEMU PowerPC sPAPR XIVE model + * + * Copyright (c) 2017, IBM Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ +#include "qemu/osdep.h" +#include "qemu/log.h" +#include "qapi/error.h" +#include "target/ppc/cpu.h" +#include "sysemu/cpus.h" +#include "sysemu/dma.h" +#include "monitor/monitor.h" +#include "hw/ppc/xics.h" +#include "hw/ppc/spapr_xive.h" + + +/* + * Main XIVE object + */ + +static void spapr_xive_realize(DeviceState *dev, Error **errp) +{ + sPAPRXive *xive = SPAPR_XIVE(dev); + + if (!xive->nr_targets) { + error_setg(errp, "Number of interrupt targets needs to be greater 0"); + return; + } + + /* We need to be able to allocate at least the IPIs */ + if (!xive->nr_irqs || xive->nr_irqs < xive->nr_targets) { + error_setg(errp, "Number of interrupts too small"); + return; + } +} + +static Property spapr_xive_properties[] = { + DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0), + DEFINE_PROP_UINT32("nr-targets", sPAPRXive, nr_targets, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +static void spapr_xive_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = spapr_xive_realize; + dc->props = spapr_xive_properties; + dc->desc = "sPAPR XIVE interrupt controller"; +} + +static const TypeInfo spapr_xive_info = { + .name = TYPE_SPAPR_XIVE, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(sPAPRXive), + .class_init = spapr_xive_class_init, +}; + +static void spapr_xive_register_types(void) +{ + type_register_static(&spapr_xive_info); +} + +type_init(spapr_xive_register_types) diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h new file mode 100644 index 000000000000..5b99f7fc2b81 --- /dev/null +++ b/include/hw/ppc/spapr_xive.h @@ -0,0 +1,37 @@ +/* + * QEMU PowerPC sPAPR XIVE model + * + * Copyright (c) 2017, IBM Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef PPC_SPAPR_XIVE_H +#define PPC_SPAPR_XIVE_H + +#include <hw/sysbus.h> + +typedef struct sPAPRXive sPAPRXive; + +#define TYPE_SPAPR_XIVE "spapr-xive" +#define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) + +struct sPAPRXive { + SysBusDevice parent; + + /* Properties */ + uint32_t nr_targets; + uint32_t nr_irqs; +}; + +#endif /* PPC_SPAPR_XIVE_H */
Start with a couple of attributes for the XIVE sPAPR controller model. The number of provisionned IRQ is necessary to size the different internal XIVE tables, the number of CPUs is also. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- default-configs/ppc64-softmmu.mak | 1 + hw/intc/Makefile.objs | 1 + hw/intc/spapr_xive.c | 76 +++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr_xive.h | 37 +++++++++++++++++++ 4 files changed, 115 insertions(+) create mode 100644 hw/intc/spapr_xive.c create mode 100644 include/hw/ppc/spapr_xive.h