From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8ySA-0007uQ-8S for qemu-devel@nongnu.org; Wed, 07 Jan 2015 16:47:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8yS5-0007LL-7H for qemu-devel@nongnu.org; Wed, 07 Jan 2015 16:47:46 -0500 Received: from cantor2.suse.de ([195.135.220.15]:47180 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8yS4-0007L7-Tu for qemu-devel@nongnu.org; Wed, 07 Jan 2015 16:47:41 -0500 Message-ID: <54ADA977.2020500@suse.de> Date: Wed, 07 Jan 2015 22:47:35 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1420560191-48029-1-git-send-email-agraf@suse.de> <1420560191-48029-4-git-send-email-agraf@suse.de> <54AD5640.9000606@huawei.com> In-Reply-To: <54AD5640.9000606@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/4] arm: Add PCIe host bridge in virt machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Claudio Fontana , qemu-devel@nongnu.org Cc: Peter Maydell , ard.biesheuvel@linaro.org, mst@redhat.com, rob.herring@linaro.org, stuart.yoder@freescale.com, a.rigo@virtualopensystems.com On 07.01.15 16:52, Claudio Fontana wrote: > On 06.01.2015 17:03, Alexander Graf wrote: >> Now that we have a working "generic" PCIe host bridge driver, we can p= lug >> it into ARMs virt machine to always have PCIe available to normal ARM = VMs. >> >> I've successfully managed to expose a Bochs VGA device, XHCI and an e1= 000 >> into an AArch64 VM with this and they all lived happily ever after. >> >> Signed-off-by: Alexander Graf >> >> --- >> >> Linux 3.19 only supports the generic PCIe host bridge driver for 32bit= ARM >> systems. If you want to use it with AArch64 guests, please apply the f= ollowing >> patch or wait until upstream cleaned up the code properly: >> >> http://csgraf.de/agraf/pci/pci-3.19.patch >> --- >> default-configs/arm-softmmu.mak | 2 + >> hw/arm/virt.c | 83 ++++++++++++++++++++++++++++++++= ++++++--- >> 2 files changed, 80 insertions(+), 5 deletions(-) >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-sof= tmmu.mak >> index f3513fa..7671ee2 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -82,6 +82,8 @@ CONFIG_ZYNQ=3Dy >> CONFIG_VERSATILE_PCI=3Dy >> CONFIG_VERSATILE_I2C=3Dy >> =20 >> +CONFIG_PCI_GENERIC=3Dy >> + >> CONFIG_SDHCI=3Dy >> CONFIG_INTEGRATOR_DEBUG=3Dy >> =20 >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 2353440..b7635ac 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -42,6 +42,7 @@ >> #include "exec/address-spaces.h" >> #include "qemu/bitops.h" >> #include "qemu/error-report.h" >> +#include "hw/pci-host/gpex.h" >> =20 >> #define NUM_VIRTIO_TRANSPORTS 32 >> =20 >> @@ -69,6 +70,7 @@ enum { >> VIRT_MMIO, >> VIRT_RTC, >> VIRT_FW_CFG, >> + VIRT_PCIE, >> }; >> =20 >> typedef struct MemMapEntry { >> @@ -129,13 +131,14 @@ static const MemMapEntry a15memmap[] =3D { >> [VIRT_FW_CFG] =3D { 0x09020000, 0x0000000a }, >> [VIRT_MMIO] =3D { 0x0a000000, 0x00000200 }, >> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of tha= t size */ >> - /* 0x10000000 .. 0x40000000 reserved for PCI */ >> + [VIRT_PCIE] =3D { 0x10000000, 0x30000000 }, >> [VIRT_MEM] =3D { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, >> }; >> =20 >> static const int a15irqmap[] =3D { >> [VIRT_UART] =3D 1, >> [VIRT_RTC] =3D 2, >> + [VIRT_PCIE] =3D 3, >> [VIRT_MMIO] =3D 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ >> }; >> =20 >> @@ -312,7 +315,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo = *vbi) >> } >> } >> =20 >> -static void fdt_add_gic_node(const VirtBoardInfo *vbi) >> +static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi) >> { >> uint32_t gic_phandle; >> =20 >> @@ -331,9 +334,11 @@ static void fdt_add_gic_node(const VirtBoardInfo = *vbi) >> 2, vbi->memmap[VIRT_GIC_CPU].bas= e, >> 2, vbi->memmap[VIRT_GIC_CPU].siz= e); >> qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle); >> + >> + return gic_phandle; >> } >> =20 >> -static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) >> +static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) >> { >> /* We create a standalone GIC v2 */ >> DeviceState *gicdev; >> @@ -380,7 +385,7 @@ static void create_gic(const VirtBoardInfo *vbi, q= emu_irq *pic) >> pic[i] =3D qdev_get_gpio_in(gicdev, i); >> } >> =20 >> - fdt_add_gic_node(vbi); >> + return fdt_add_gic_node(vbi); >> } >> =20 >> static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) >> @@ -556,6 +561,71 @@ static void create_fw_cfg(const VirtBoardInfo *vb= i) >> g_free(nodename); >> } >> =20 >> +static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic, >> + uint32_t gic_phandle) >> +{ >> + hwaddr base =3D vbi->memmap[VIRT_PCIE].base; >> + hwaddr size =3D vbi->memmap[VIRT_PCIE].size; >> + hwaddr size_ioport =3D 64 * 1024; >> + hwaddr size_ecam =3D PCIE_MMCFG_SIZE_MIN; >> + hwaddr size_mmio =3D size - size_ecam - size_ioport; >> + hwaddr base_mmio =3D base; >> + hwaddr base_ioport =3D base_mmio + size_mmio; >> + hwaddr base_ecam =3D base_ioport + size_ioport; >> + int irq =3D vbi->irqmap[VIRT_PCIE]; >> + MemoryRegion *mmio_alias; >> + MemoryRegion *mmio_reg; >> + DeviceState *dev; >> + char *nodename; >> + >> + dev =3D qdev_create(NULL, TYPE_GPEX_HOST); >> + >> + qdev_prop_set_uint64(dev, "mmio_window_size", size_mmio); >> + qdev_init_nofail(dev); >> + >> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base_ecam); >> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_ioport); >> + >> + /* Map the MMIO window at the same spot in bus and cpu layouts */ >> + mmio_alias =3D g_new0(MemoryRegion, 1); >> + mmio_reg =3D sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1); >> + memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio", >> + mmio_reg, base_mmio, size_mmio); >> + memory_region_add_subregion(get_system_memory(), base_mmio, mmio_= alias); >> + >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]); >> + >> + nodename =3D g_strdup_printf("/pcie@%" PRIx64, base); >> + qemu_fdt_add_subnode(vbi->fdt, nodename); >> + qemu_fdt_setprop_string(vbi->fdt, nodename, >> + "compatible", "pci-host-ecam-generic"); >=20 > is this the only compatible string we should set here? Is this not lega= cy pci compatible? > In other device trees I see this mentioned as compatible =3D "arm,versa= tile-pci-hostbridge", "pci" for example, > would it be sensible to make it a list and include "pci" as well? I couldn't find anything that defines what an "pci" compatible should look like. We definitely don't implement the legacy PCI config space accessor registers. >=20 >> + qemu_fdt_setprop_string(vbi->fdt, nodename, "device_type", "pci")= ; >> + qemu_fdt_setprop_cell(vbi->fdt, nodename, "#address-cells", 3); >> + qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2); >> + qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0, 1); >> + >> + qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", >> + 2, base_ecam, 2, size_ecam); >> + qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges", >> + 1, 0x01000000, 2, 0, >> + 2, base_ioport, 2, size_ioport, >> + >> + 1, 0x02000000, 2, base_mmio, >> + 2, base_mmio, 2, size_mmio); >> + >> + qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1); >> + qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupt-map", >> + 0, 0, 0, /* device */ >> + 0, /* PCI irq */ >> + gic_phandle, GIC_FDT_IRQ_TYPE_SPI, irq, >> + GIC_FDT_IRQ_FLAGS_LEVEL_HI /* system irq= */); >> + qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupt-map-mask", >> + 0, 0, 0, /* device */ >> + 0 /* PCI irq */); >=20 > Interrupt map does not seem to work for me; incidentally this ends up b= eing the same kind of undocumented blob that Alvise posted in his series. How exactly is this undocumented? The "mask" is a mask over the first fields of an interrupt-map row plus an IRQ offset. So the mask above means "Any device with any function and any IRQ on it, map to device IRQ 0" which maps to vbi->irqmap[VIRT_PCIE] (IRQ 3). > Can you add a good comment about what the ranges property contains (the= 0x01000000, 0x02000000 which I suspect means IO vs MMIO IIRC, but there = is no need to be cryptic about it). You're saying you'd prefer a define? > How does your interrupt map implementation differ from the patchset pos= ted by Alvise? I ask because that one works for me (tm). His implementation explicitly listed every PCI slot, while mine actually makes use of the mask and simply routes everything to a single IRQ line. The benefit of masking devfn out is that you don't need to worry about the number of slots you support - and anything performance critical should go via MSI-X anyway ;). So what exactly do you test it with? I've successfully received IRQs with a Linux guest, so I'm slightly puzzled you're running into problems. Alex