| Message ID | 1432464666-4825-5-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hello! > typedef struct MemMapEntry { > @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { > int fdt_size; > uint32_t clock_phandle; > uint32_t gic_phandle; > + uint32_t v2m_phandle; > } VirtBoardInfo; Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be used by GICv3 ITS implementation. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 25 May 2015 at 14:09, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> typedef struct MemMapEntry { >> @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { >> int fdt_size; >> uint32_t clock_phandle; >> uint32_t gic_phandle; >> + uint32_t v2m_phandle; >> } VirtBoardInfo; > > Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be > used by GICv3 ITS implementation. Why? The v2m device isn't really related to the GICv3... -- PMM
On 05/25/2015 05:01 PM, Peter Maydell wrote: > On 25 May 2015 at 14:09, Pavel Fedin <p.fedin@samsung.com> wrote: >> Hello! >> >>> typedef struct MemMapEntry { >>> @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { >>> int fdt_size; >>> uint32_t clock_phandle; >>> uint32_t gic_phandle; >>> + uint32_t v2m_phandle; >>> } VirtBoardInfo; >> >> Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be >> used by GICv3 ITS implementation. > > Why? The v2m device isn't really related to the GICv3... In the future this handle could point to GICv2m or GICv3 ITS or GICv3 (if I understand it correctly, when it supports message base interrupts and no ITS). They all would play the same role of msi-controller. Best Regards Eric > > -- PMM >
Hi Pavel, On Mon, May 25, 2015 at 04:09:58PM +0300, Pavel Fedin wrote: > Hello! > > > typedef struct MemMapEntry { > > @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { > > int fdt_size; > > uint32_t clock_phandle; > > uint32_t gic_phandle; > > + uint32_t v2m_phandle; > > } VirtBoardInfo; > > Could you rename v2m_phandle to something more neutral like msi_phandle ? It will also be > used by GICv3 ITS implementation. > That's sort of how to speculate about. Why can't those patches just rename the variable then? Right now, as the code stands, msi_phandle would be less clear IMHO. -Christoffer
Hi! > > Why? The v2m device isn't really related to the GICv3... > In the future this handle could point to GICv2m or GICv3 ITS or GICv3 > (if I understand it correctly, when it supports message base interrupts > and no ITS). They all would play the same role of msi-controller. Yes, exactly. In my implementation being developed i actually reused first patch from this set, and indeed 'v2m_handle' appeared to be useful for ITS. My code flow is something like: if (gicv3) create_its() else create_v2m() Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Reviewed-by: Eric Auger <eric.auger@linaro.org> The only question I have is related to mid-term virt strategy about GICv3 integration. Are we going to reuse that memory map for the machine instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere else to leave space for GICv3 redistributors, assuming we reuse the shared distributor region. I understood the memory map is difficult to change once applied once. Best Regards Eric On 05/24/2015 12:51 PM, Christoffer Dall wrote: > Add a GICv2m device to the virt board to enable MSIs on the generic PCI > host controller. We allocate 64 SPIs in the IRQ space for now (this can > be increased/decreased later) and map the GICv2m right after the GIC in > the memory map. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > Changes since v2: > - Factored out changes to GIC DT node to previous patch. > - Renamed QOM type name to "arm-gicv2m" > Changes since v1: > - Remove stray merge conflict line > - Reworded commmit message. > > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 6797c6f..2972bb3 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -45,6 +45,7 @@ > #include "hw/pci-host/gpex.h" > > #define NUM_VIRTIO_TRANSPORTS 32 > +#define NUM_GICV2M_SPIS 64 > > /* Number of external interrupt lines to configure the GIC with */ > #define NUM_IRQS 128 > @@ -71,6 +72,7 @@ enum { > VIRT_RTC, > VIRT_FW_CFG, > VIRT_PCIE, > + VIRT_GIC_V2M, > }; > > typedef struct MemMapEntry { > @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { > int fdt_size; > uint32_t clock_phandle; > uint32_t gic_phandle; > + uint32_t v2m_phandle; > } VirtBoardInfo; > > typedef struct { > @@ -127,6 +130,7 @@ static const MemMapEntry a15memmap[] = { > /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */ > [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 }, > [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 }, > + [VIRT_GIC_V2M] = { 0x08020000, 0x00001000 }, > [VIRT_UART] = { 0x09000000, 0x00001000 }, > [VIRT_RTC] = { 0x09010000, 0x00001000 }, > [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, > @@ -148,6 +152,7 @@ static const int a15irqmap[] = { > [VIRT_RTC] = 2, > [VIRT_PCIE] = 3, /* ... to 6 */ > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > + [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ > }; > > static VirtBoardInfo machines[] = { > @@ -323,9 +328,21 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) > } > } > > -static void fdt_add_gic_node(VirtBoardInfo *vbi) > +static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi) > { > + vbi->v2m_phandle = qemu_fdt_alloc_phandle(vbi->fdt); > + qemu_fdt_add_subnode(vbi->fdt, "/intc/v2m"); > + qemu_fdt_setprop_string(vbi->fdt, "/intc/v2m", "compatible", > + "arm,gic-v2m-frame"); > + qemu_fdt_setprop(vbi->fdt, "/intc/v2m", "msi-controller", NULL, 0); > + qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc/v2m", "reg", > + 2, vbi->memmap[VIRT_GIC_V2M].base, > + 2, vbi->memmap[VIRT_GIC_V2M].size); > + qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle", vbi->v2m_phandle); > +} > > +static void fdt_add_gic_node(VirtBoardInfo *vbi) > +{ > vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt); > qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->gic_phandle); > > @@ -347,6 +364,25 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi) > > } > > +static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic) > +{ > + int i; > + int irq = vbi->irqmap[VIRT_GIC_V2M]; > + DeviceState *dev; > + > + dev = qdev_create(NULL, "arm-gicv2m"); > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vbi->memmap[VIRT_GIC_V2M].base); > + qdev_prop_set_uint32(dev, "base-spi", irq); > + qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS); > + qdev_init_nofail(dev); > + > + for (i = 0; i < NUM_GICV2M_SPIS; i++) { > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]); > + } > + > + fdt_add_v2m_gic_node(vbi); > +} > + > static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic) > { > /* We create a standalone GIC v2 */ > @@ -397,6 +433,8 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic) > } > > fdt_add_gic_node(vbi); > + > + create_v2m(vbi, pic); > } > > static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) > @@ -707,6 +745,8 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic) > qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0, > nr_pcie_buses - 1); > > + qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent", vbi->v2m_phandle); > + > qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", > 2, base_ecam, 2, size_ecam); > qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges", >
On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote: > Reviewed-by: Eric Auger <eric.auger@linaro.org> > > The only question I have is related to mid-term virt strategy about > GICv3 integration. Are we going to reuse that memory map for the machine > instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere > else to leave space for GICv3 redistributors, assuming we reuse the > shared distributor region. I understood the memory map is difficult to > change once applied once. I wouldn't expect that you'd have a GICv2M at all in a system with a GICv3 in it, would you? -- PMM
On 05/26/2015 02:55 PM, Peter Maydell wrote: > On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote: >> Reviewed-by: Eric Auger <eric.auger@linaro.org> >> >> The only question I have is related to mid-term virt strategy about >> GICv3 integration. Are we going to reuse that memory map for the machine >> instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere >> else to leave space for GICv3 redistributors, assuming we reuse the >> shared distributor region. I understood the memory map is difficult to >> change once applied once. > > I wouldn't expect that you'd have a GICv2M at all in a > system with a GICv3 in it, would you? no indeed. but we currently use a single static a15memmap memory map in virt. This one is currently planned to be reused for machines instantiating GICv3 so we start seeing things like VIRT_GIC_CPU = VIRT_GIC_REDIST. I fear this is going to become messy. What is your guidance, should we introduce new memory maps for GICv3 enabled machine or should we move to a single dynamic memory map? Best Regards Eric > > -- PMM >
On 26 May 2015 at 14:07, Eric Auger <eric.auger@linaro.org> wrote: > On 05/26/2015 02:55 PM, Peter Maydell wrote: >> On 26 May 2015 at 13:54, Eric Auger <eric.auger@linaro.org> wrote: >>> Reviewed-by: Eric Auger <eric.auger@linaro.org> >>> >>> The only question I have is related to mid-term virt strategy about >>> GICv3 integration. Are we going to reuse that memory map for the machine >>> instantiating the GICv3? If yes, shouldn't we put the GICv2M somewhere >>> else to leave space for GICv3 redistributors, assuming we reuse the >>> shared distributor region. I understood the memory map is difficult to >>> change once applied once. >> >> I wouldn't expect that you'd have a GICv2M at all in a >> system with a GICv3 in it, would you? > > no indeed. but we currently use a single static a15memmap memory map in > virt. This one is currently planned to be reused for machines > instantiating GICv3 so we start seeing things like > VIRT_GIC_CPU = VIRT_GIC_REDIST. I fear this is going to become messy. I think the answer is not to reuse constant names like that. The v3 redistributor is not a v2 CPU interface, just as a UART is not an RTC. -- PMM
Hi! My word... > What is your guidance, should we introduce new memory maps for GICv3 > enabled machine or should we move to a single dynamic memory map? IMHO there's no reason to introduce another memory map. I have already done test integration some time ago, and here is what i got: --- cut --- enum { VIRT_FLASH, VIRT_MEM, VIRT_CPUPERIPHS, VIRT_GIC_DIST, VIRT_GIC_CPU, VIRT_GIC_DIST_SPI = VIRT_GIC_CPU, VIRT_ITS_CONTROL, VIRT_ITS_TRANSLATION, VIRT_LPI, VIRT_UART, VIRT_MMIO, VIRT_RTC, VIRT_FW_CFG, VIRT_PCIE, VIRT_GIC_V2M = VIRT_ITS_CONTROL, }; --- cut --- static const MemMapEntry a15memmap[] = { /* Space up to 0x8000000 is reserved for a boot ROM */ [VIRT_FLASH] = { 0, 0x08000000 }, [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 }, /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */ [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 }, [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 }, /* VIRT_GIC_DIST_SPI for v3 */ [VIRT_ITS_CONTROL] = { 0x08020000, 0x0010000 }, /* VIRT_GIC_V2M for v2 */ [VIRT_ITS_TRANSLATION] = { 0x08030000, 0x00010000 }, [VIRT_LPI] = { 0x08040000, 0x00800000 }, [VIRT_UART] = { 0x09000000, 0x00001000 }, [VIRT_RTC] = { 0x09010000, 0x00001000 }, [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ /* * PCIE verbose map: * * MMIO window { 0x10000000, 0x2eff0000 }, * PIO window { 0x3eff0000, 0x00010000 }, * ECAM { 0x3f000000, 0x01000000 }, */ [VIRT_PCIE] = { 0x10000000, 0x30000000 }, [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, }; --- cut --- As you can see, it's perfectly readable and memory maps are identical. I even see no problem with merging ITS_CONTROL and ITS_TRANSPATION, because i don't see any harm in increasing v2m area. P.S. And yes, VIRT_GIC_DIST_SPI should be VIRT_GIC_DIST_MBI instead (Reviewed-by: Eric Auger <eric.auger@linaro.org>), just this fragment is from my old integration branch, which i currently don't work on, because my new test environment doesn't use GICv2 at all. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 6797c6f..2972bb3 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -45,6 +45,7 @@ #include "hw/pci-host/gpex.h" #define NUM_VIRTIO_TRANSPORTS 32 +#define NUM_GICV2M_SPIS 64 /* Number of external interrupt lines to configure the GIC with */ #define NUM_IRQS 128 @@ -71,6 +72,7 @@ enum { VIRT_RTC, VIRT_FW_CFG, VIRT_PCIE, + VIRT_GIC_V2M, }; typedef struct MemMapEntry { @@ -88,6 +90,7 @@ typedef struct VirtBoardInfo { int fdt_size; uint32_t clock_phandle; uint32_t gic_phandle; + uint32_t v2m_phandle; } VirtBoardInfo; typedef struct { @@ -127,6 +130,7 @@ static const MemMapEntry a15memmap[] = { /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */ [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 }, [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 }, + [VIRT_GIC_V2M] = { 0x08020000, 0x00001000 }, [VIRT_UART] = { 0x09000000, 0x00001000 }, [VIRT_RTC] = { 0x09010000, 0x00001000 }, [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, @@ -148,6 +152,7 @@ static const int a15irqmap[] = { [VIRT_RTC] = 2, [VIRT_PCIE] = 3, /* ... to 6 */ [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ + [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ }; static VirtBoardInfo machines[] = { @@ -323,9 +328,21 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) } } -static void fdt_add_gic_node(VirtBoardInfo *vbi) +static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi) { + vbi->v2m_phandle = qemu_fdt_alloc_phandle(vbi->fdt); + qemu_fdt_add_subnode(vbi->fdt, "/intc/v2m"); + qemu_fdt_setprop_string(vbi->fdt, "/intc/v2m", "compatible", + "arm,gic-v2m-frame"); + qemu_fdt_setprop(vbi->fdt, "/intc/v2m", "msi-controller", NULL, 0); + qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc/v2m", "reg", + 2, vbi->memmap[VIRT_GIC_V2M].base, + 2, vbi->memmap[VIRT_GIC_V2M].size); + qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle", vbi->v2m_phandle); +} +static void fdt_add_gic_node(VirtBoardInfo *vbi) +{ vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt); qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->gic_phandle); @@ -347,6 +364,25 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi) } +static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic) +{ + int i; + int irq = vbi->irqmap[VIRT_GIC_V2M]; + DeviceState *dev; + + dev = qdev_create(NULL, "arm-gicv2m"); + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vbi->memmap[VIRT_GIC_V2M].base); + qdev_prop_set_uint32(dev, "base-spi", irq); + qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS); + qdev_init_nofail(dev); + + for (i = 0; i < NUM_GICV2M_SPIS; i++) { + sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]); + } + + fdt_add_v2m_gic_node(vbi); +} + static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic) { /* We create a standalone GIC v2 */ @@ -397,6 +433,8 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic) } fdt_add_gic_node(vbi); + + create_v2m(vbi, pic); } static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) @@ -707,6 +745,8 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic) qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0, nr_pcie_buses - 1); + qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent", vbi->v2m_phandle); + qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", 2, base_ecam, 2, size_ecam); qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
Add a GICv2m device to the virt board to enable MSIs on the generic PCI host controller. We allocate 64 SPIs in the IRQ space for now (this can be increased/decreased later) and map the GICv2m right after the GIC in the memory map. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- Changes since v2: - Factored out changes to GIC DT node to previous patch. - Renamed QOM type name to "arm-gicv2m" Changes since v1: - Remove stray merge conflict line - Reworded commmit message. hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)