Re: [Qemu-devel] [PATCH v5 1/3] spapr: introduce a fixed IRQ number space
On Mon, Jul 30, 2018 at 11:05:58AM +0200, Cédric Le Goater wrote:
> On 07/27/2018 05:56 AM, David Gibson wrote:
> > On Thu, Jul 26, 2018 at 03:37:21PM +0200, Cédric Le Goater wrote:
> >> This proposal introduces a new IRQ number space layout using static
> >> numbers for all devices, depending on a device index, and a bitmap
> >> allocator for the MSI IRQ numbers which are negotiated by the guest at
> >> runtime.
> >>
> >> As the VIO device model does not have a device index but a "reg"
> >> property, we introduce a formula to compute an IRQ number from a "reg"
> >> value. It should minimize most of the collisions.
> >>
> >> The previous layout is kept in pre-3.1 machines raising the
> >> 'legacy_irq_allocation' machine class flag.
> >>
> >> Signed-off-by: Cédric Le Goater
> >
> > One nit left..
> >
> > [snip]
> >> +static inline uint32_t spapr_vio_reg_to_irq(uint32_t reg)
> >> +{
> >> +uint32_t irq;
> >> +
> >> +if (reg >= SPAPR_VIO_REG_BASE) {
> >> +/*
> >> + * VIO device register values when allocated by QEMU. For
> >> + * these, we simply mask the high bits to fit the overall
> >> + * range: [0x00 - 0xff].
> >> + *
> >> + * The nvram VIO device (reg=0x7100) is a static device of
> >> + * the pseries machine and so is always allocated by QEMU. Its
> >> + * IRQ number is 0x0.
> >> + */
> >> +irq = reg & 0xff;
> >> +
> >> +} else if (reg >= 0x3000) {
> >> +/*
> >> + * VIO tty devices register values, when allocated by livirt,
> >> + * are mapped in range [0xf0 - 0xff], gives us a maximum of 16
> >> + * vtys.
> >> + */
> >> +irq = 0xf0 | ((reg >> 12) & 0xf);
> >> +
> >> +} else {
> >> +/*
> >> + * Other VIO devices register values, when allocated by
> >> + * livirt, are mapped in range [0x00 - 0xef].
> >> + */
> >> +irq = (reg >> 12) & 0xef;
> >
> > This mask doesn't do what you intend - it will map 0x10 to 0, for
> > example. You could use % 0xf0, but actually you might as well just
> > use & 0xff. Yes, it could collide with the vty devices, but either
> > way you can still have collisions if you try hard enough. And, either
> > way, they'll get detected later.
> >
>
>
> David,
>
> Shall I resend a v6 with this fix or should I wait for the patch adding
> 3.1. I could also send a 3.1 pseries machine also if you prefer.
We'll need a pseries-3.1 at some point, so if you can send that, it
would be great. Then I can stage the lot in ppc-for-3.1. It might
need some tweaking before final merge, but that shouldn't be too hard.
--
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
Re: [Qemu-devel] [PATCH v5 1/3] spapr: introduce a fixed IRQ number space
On 07/27/2018 05:56 AM, David Gibson wrote:
> On Thu, Jul 26, 2018 at 03:37:21PM +0200, Cédric Le Goater wrote:
>> This proposal introduces a new IRQ number space layout using static
>> numbers for all devices, depending on a device index, and a bitmap
>> allocator for the MSI IRQ numbers which are negotiated by the guest at
>> runtime.
>>
>> As the VIO device model does not have a device index but a "reg"
>> property, we introduce a formula to compute an IRQ number from a "reg"
>> value. It should minimize most of the collisions.
>>
>> The previous layout is kept in pre-3.1 machines raising the
>> 'legacy_irq_allocation' machine class flag.
>>
>> Signed-off-by: Cédric Le Goater
>
> One nit left..
>
> [snip]
>> +static inline uint32_t spapr_vio_reg_to_irq(uint32_t reg)
>> +{
>> +uint32_t irq;
>> +
>> +if (reg >= SPAPR_VIO_REG_BASE) {
>> +/*
>> + * VIO device register values when allocated by QEMU. For
>> + * these, we simply mask the high bits to fit the overall
>> + * range: [0x00 - 0xff].
>> + *
>> + * The nvram VIO device (reg=0x7100) is a static device of
>> + * the pseries machine and so is always allocated by QEMU. Its
>> + * IRQ number is 0x0.
>> + */
>> +irq = reg & 0xff;
>> +
>> +} else if (reg >= 0x3000) {
>> +/*
>> + * VIO tty devices register values, when allocated by livirt,
>> + * are mapped in range [0xf0 - 0xff], gives us a maximum of 16
>> + * vtys.
>> + */
>> +irq = 0xf0 | ((reg >> 12) & 0xf);
>> +
>> +} else {
>> +/*
>> + * Other VIO devices register values, when allocated by
>> + * livirt, are mapped in range [0x00 - 0xef].
>> + */
>> +irq = (reg >> 12) & 0xef;
>
> This mask doesn't do what you intend - it will map 0x10 to 0, for
> example. You could use % 0xf0, but actually you might as well just
> use & 0xff. Yes, it could collide with the vty devices, but either
> way you can still have collisions if you try hard enough. And, either
> way, they'll get detected later.
>
David,
Shall I resend a v6 with this fix or should I wait for the patch adding
3.1. I could also send a 3.1 pseries machine also if you prefer.
Thanks,
C.
Re: [Qemu-devel] [PATCH v5 1/3] spapr: introduce a fixed IRQ number space
>> +/* >> + * Other VIO devices register values, when allocated by >> + * livirt, are mapped in range [0x00 - 0xef]. >> + */ >> +irq = (reg >> 12) & 0xef; > > This mask doesn't do what you intend - it will map 0x10 to 0, for oops. yes indeed. > example. You could use % 0xf0, but actually you might as well just > use & 0xff. Yes, it could collide with the vty devices, but either > way you can still have collisions if you try hard enough. > And, either way, they'll get detected later. I will do that. C.
Re: [Qemu-devel] [PATCH v5 1/3] spapr: introduce a fixed IRQ number space
On Thu, Jul 26, 2018 at 03:37:21PM +0200, Cédric Le Goater wrote:
> This proposal introduces a new IRQ number space layout using static
> numbers for all devices, depending on a device index, and a bitmap
> allocator for the MSI IRQ numbers which are negotiated by the guest at
> runtime.
>
> As the VIO device model does not have a device index but a "reg"
> property, we introduce a formula to compute an IRQ number from a "reg"
> value. It should minimize most of the collisions.
>
> The previous layout is kept in pre-3.1 machines raising the
> 'legacy_irq_allocation' machine class flag.
>
> Signed-off-by: Cédric Le Goater
One nit left..
[snip]
> +static inline uint32_t spapr_vio_reg_to_irq(uint32_t reg)
> +{
> +uint32_t irq;
> +
> +if (reg >= SPAPR_VIO_REG_BASE) {
> +/*
> + * VIO device register values when allocated by QEMU. For
> + * these, we simply mask the high bits to fit the overall
> + * range: [0x00 - 0xff].
> + *
> + * The nvram VIO device (reg=0x7100) is a static device of
> + * the pseries machine and so is always allocated by QEMU. Its
> + * IRQ number is 0x0.
> + */
> +irq = reg & 0xff;
> +
> +} else if (reg >= 0x3000) {
> +/*
> + * VIO tty devices register values, when allocated by livirt,
> + * are mapped in range [0xf0 - 0xff], gives us a maximum of 16
> + * vtys.
> + */
> +irq = 0xf0 | ((reg >> 12) & 0xf);
> +
> +} else {
> +/*
> + * Other VIO devices register values, when allocated by
> + * livirt, are mapped in range [0x00 - 0xef].
> + */
> +irq = (reg >> 12) & 0xef;
This mask doesn't do what you intend - it will map 0x10 to 0, for
example. You could use % 0xf0, but actually you might as well just
use & 0xff. Yes, it could collide with the vty devices, but either
way you can still have collisions if you try hard enough. And, either
way, they'll get detected later.
--
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
[Qemu-devel] [PATCH v5 1/3] spapr: introduce a fixed IRQ number space
This proposal introduces a new IRQ number space layout using static
numbers for all devices, depending on a device index, and a bitmap
allocator for the MSI IRQ numbers which are negotiated by the guest at
runtime.
As the VIO device model does not have a device index but a "reg"
property, we introduce a formula to compute an IRQ number from a "reg"
value. It should minimize most of the collisions.
The previous layout is kept in pre-3.1 machines raising the
'legacy_irq_allocation' machine class flag.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/spapr.h | 5 +++
include/hw/ppc/spapr_irq.h | 32 +++
hw/ppc/spapr.c | 32 ++-
hw/ppc/spapr_events.c | 12 ---
hw/ppc/spapr_irq.c | 56
hw/ppc/spapr_pci.c | 29 +
hw/ppc/spapr_vio.c | 65 ++
hw/ppc/Makefile.objs | 2 +-
8 files changed, 214 insertions(+), 19 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 7e5de1a6fd42..73067f5ee8aa 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -8,6 +8,7 @@
#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;
@@ -101,6 +102,8 @@ struct sPAPRMachineClass {
bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
bool pre_2_10_has_unused_icps;
+bool legacy_irq_allocation;
+
void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
hwaddr *mmio32, hwaddr *mmio64,
@@ -167,6 +170,8 @@ struct sPAPRMachineState {
char *kvm_type;
const char *icp_type;
+int32_t irq_map_nr;
+unsigned long *irq_map;
bool cmd_line_caps[SPAPR_CAP_NUM];
sPAPRCapabilities def, eff, mig;
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
new file mode 100644
index ..6f7f50548809
--- /dev/null
+++ b/include/hw/ppc/spapr_irq.h
@@ -0,0 +1,32 @@
+/*
+ * 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
+
+/*
+ * IRQ range offsets per device type
+ */
+#define SPAPR_IRQ_EPOW 0x1000 /* XICS_IRQ_BASE offset */
+#define SPAPR_IRQ_HOTPLUG0x1001
+#define SPAPR_IRQ_VIO0x1100 /* 256 VIO devices */
+#define SPAPR_IRQ_PCI_LSI0x1200 /* 32+ PHBs devices */
+
+#define SPAPR_IRQ_MSI0x1300 /* Offset of the dynamic range covered
+ * by the bitmap allocator */
+
+typedef struct sPAPRMachineState sPAPRMachineState;
+
+void spapr_irq_msi_init(sPAPRMachineState *spapr, uint32_t nr_msis);
+int spapr_irq_msi_alloc(sPAPRMachineState *spapr, uint32_t num, bool align,
+Error **errp);
+void spapr_irq_msi_free(sPAPRMachineState *spapr, int irq, uint32_t num);
+void spapr_irq_msi_reset(sPAPRMachineState *spapr);
+
+#endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 421b2dd09b51..1794952d4e2a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -189,6 +189,11 @@ static void xics_system_init(MachineState *machine, int
nr_irqs, Error **errp)
sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
Error *local_err = NULL;
+/* Initialize the MSI IRQ allocator. */
+if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+spapr_irq_msi_init(spapr, XICS_IRQ_BASE + nr_irqs - SPAPR_IRQ_MSI);
+}
+
if (kvm_enabled()) {
if (machine_kernel_irqchip_allowed(machine) &&
!xics_kvm_init(spapr, &local_err)) {
@@ -1636,6 +1641,10 @@ static void spapr_machine_reset(void)
ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
}
+if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
+spapr_irq_msi_reset(spapr);
+}
+
qemu_devices_reset();
/* DRC reset may cause a device to be unplugged. This will cause troubles
@@ -1910,6 +1919,24 @@ static const VMStateDescription vmstate_spapr_patb_entry
= {
},
};
+static bool spapr_irq_map_needed(void *opaque)
+{
+sPAPRMachineState *spapr = opaque;
+
+return spapr->irq_map && !bitmap_empty(spapr->irq_map, spapr->irq_map_nr);
+}
+
+static const VMStateDescription vmstate_spapr_irq_map = {
+.name = "spapr_irq_map",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = spapr_irq_map_needed,
+.fields = (VMStateField[]) {
+VMSTATE_BITMAP(irq_map, sPAPRMachineState, 0, irq_map_nr),
+VMSTATE_END_OF_LIST()
+},
+};
+
static co
