From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43427) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG2OO-00078B-8C for qemu-devel@nongnu.org; Tue, 27 Jan 2015 04:25:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YG2OK-0006Dw-7k for qemu-devel@nongnu.org; Tue, 27 Jan 2015 04:25:04 -0500 Received: from lhrrgout.huawei.com ([194.213.3.17]:22251) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG2OJ-0006Ct-SW for qemu-devel@nongnu.org; Tue, 27 Jan 2015 04:25:00 -0500 Message-ID: <54C75957.9070809@huawei.com> Date: Tue, 27 Jan 2015 10:24:39 +0100 From: Claudio Fontana MIME-Version: 1.0 References: <1421857131-18539-1-git-send-email-agraf@suse.de> <1421857131-18539-4-git-send-email-agraf@suse.de> <54C1171D.1000500@huawei.com> <54C11CAD.6030109@suse.de> In-Reply-To: <54C11CAD.6030109@suse.de> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/4] arm: Add PCIe host bridge in virt machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , 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 22.01.2015 16:52, Alexander Graf wrote: > > > On 22.01.15 16:28, Claudio Fontana wrote: >> Hi Alexander, >> >> thank you for the respin. I retested with the new mappings on OSv for AArch64, >> and they show up ok in the guest. >> >> Just a couple minor comments below, otherwise I think this is good. >> >> On 21.01.2015 17:18, Alexander Graf wrote: >>> Now that we have a working "generic" PCIe host bridge driver, we can plug >>> 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 e1000 >>> into an AArch64 VM with this and they all lived happily ever after. >>> >>> Signed-off-by: Alexander Graf >>> Tested-by: Claudio Fontana >>> >>> --- >>> >>> 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 following >>> patch or wait until upstream cleaned up the code properly: >>> >>> http://csgraf.de/agraf/pci/pci-3.19.patch >>> >>> v1 -> v2: >>> >>> - Add define for pci range types >>> - Remove mmio_window_size >>> - Use 4 PCI INTX IRQ lines >>> --- >>> default-configs/arm-softmmu.mak | 2 + >>> hw/arm/virt.c | 112 ++++++++++++++++++++++++++++++++++++++-- >>> include/sysemu/device_tree.h | 9 ++++ >>> 3 files changed, 118 insertions(+), 5 deletions(-) >>> >>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >>> index f3513fa..7671ee2 100644 >>> --- a/default-configs/arm-softmmu.mak >>> +++ b/default-configs/arm-softmmu.mak >>> @@ -82,6 +82,8 @@ CONFIG_ZYNQ=y >>> CONFIG_VERSATILE_PCI=y >>> CONFIG_VERSATILE_I2C=y >>> >>> +CONFIG_PCI_GENERIC=y >>> + >>> CONFIG_SDHCI=y >>> CONFIG_INTEGRATOR_DEBUG=y >>> >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>> index 2353440..a727b4e 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" >>> >>> #define NUM_VIRTIO_TRANSPORTS 32 >>> >>> @@ -69,6 +70,7 @@ enum { >>> VIRT_MMIO, >>> VIRT_RTC, >>> VIRT_FW_CFG, >>> + VIRT_PCIE, >>> }; >>> >>> typedef struct MemMapEntry { >>> @@ -129,13 +131,14 @@ static const MemMapEntry a15memmap[] = { >>> [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, >>> [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, >>> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ >>> - /* 0x10000000 .. 0x40000000 reserved for PCI */ >>> + [VIRT_PCIE] = { 0x10000000, 0x30000000 }, >>> [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, >>> }; >>> >>> static const int a15irqmap[] = { >>> [VIRT_UART] = 1, >>> [VIRT_RTC] = 2, >>> + [VIRT_PCIE] = 3, /* ... to 6 */ >>> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ >>> }; >>> >>> @@ -312,7 +315,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) >>> } >>> } >>> >>> -static void fdt_add_gic_node(const VirtBoardInfo *vbi) >>> +static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi) >>> { >>> uint32_t gic_phandle; >>> >>> @@ -331,9 +334,11 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi) >>> 2, vbi->memmap[VIRT_GIC_CPU].base, >>> 2, vbi->memmap[VIRT_GIC_CPU].size); >>> qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle); >>> + >>> + return gic_phandle; >>> } >>> >>> -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, qemu_irq *pic) >>> pic[i] = qdev_get_gpio_in(gicdev, i); >>> } >>> >>> - fdt_add_gic_node(vbi); >>> + return fdt_add_gic_node(vbi); >>> } >>> >>> static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) >>> @@ -556,6 +561,100 @@ static void create_fw_cfg(const VirtBoardInfo *vbi) >>> g_free(nodename); >>> } >>> >>> +static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t gic_phandle, >>> + int first_irq, const char *nodename) >>> +{ >>> + int devfn, pin; >>> + uint32_t full_irq_map[4 * 4 * 8] = { 0 }; >>> + uint32_t *irq_map = full_irq_map; >>> + >>> + for (devfn = 0; devfn <= 0x18; devfn+=0x8) { >> >> devfn += 0x8 (spaces) > > Yeah, I had it like that and it looked uglier. I guess it's a matter of > personal preference? You don't have coding standards for this ? > >> >>> + for (pin = 0; pin < 4; pin++) { >>> + int irq_type = GIC_FDT_IRQ_TYPE_SPI; >>> + int irq_nr = first_irq + ((pin + PCI_SLOT(devfn)) % PCI_NUM_PINS); >>> + int irq_level = GIC_FDT_IRQ_FLAGS_LEVEL_HI; >>> + int i; >>> + >>> + uint32_t map[] = { >>> + devfn << 8, 0, 0, /* devfn */ >>> + pin + 1, /* PCI pin */ >>> + gic_phandle, irq_type, irq_nr, irq_level }; /* GIC irq */ >>> + >>> + /* Convert map to big endian */ >>> + for (i = 0; i < 8; i++) { >>> + irq_map[i] = cpu_to_be32(map[i]); >>> + } >>> + irq_map += 8; >>> + } >>> + } >>> + >>> + qemu_fdt_setprop(vbi->fdt, nodename, "interrupt-map", >>> + full_irq_map, sizeof(full_irq_map)); >>> +} >> >> I raise again (? or maybe for the first time) the suggestion to add a comment like: >> >> /* see the openfirmware specs at >> * http://www.firmware.org/1275/practice/imap/imap0_9d.pdf >> */ >> >> and optionally also a reference to the linux host-generic-pci bindings: >> >> /* see the linux Documentation about device tree bindings at: >> * https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt >> */ > > I added those links to the big description comment at the top of this file. > It is not on top of this file unfortunately. You put the description comment in an unrelated .h file: the pointers to the docs belong here I think. Ciao, Claudio