Re: [Qemu-devel] [PATCH v4 3/4] arm: Add PCIe host bridge in virt machine

[Peter Maydell]

On 5 February 2015 at 14:55, Alexander Graf <address@hidden> wrote:
> Now that we have a working "generic" PCIe host bridge driver, we can plug
> it into ARM's 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 <address@hidden>
> Tested-by: Claudio Fontana <address@hidden>

> +static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
> +                        uint32_t gic_phandle)
> +{
> +    hwaddr base = vbi->memmap[VIRT_PCIE].base;
> +    hwaddr size = vbi->memmap[VIRT_PCIE].size;
> +    hwaddr end = base + size;
> +    hwaddr size_mmio;
> +    hwaddr size_ioport = 64 * 1024;
> +    hwaddr size_ecam = PCIE_MMCFG_SIZE_MIN * 16; /* 16 PCIe buses */
> +    hwaddr base_mmio = base;
> +    hwaddr base_ioport;
> +    hwaddr base_ecam;
> +    int irq = vbi->irqmap[VIRT_PCIE];
> +    MemoryRegion *mmio_alias;
> +    MemoryRegion *mmio_reg;
> +    MemoryRegion *ecam_alias;
> +    MemoryRegion *ecam_reg;
> +    DeviceState *dev;
> +    char *nodename;
> +    int i;
> +
> +    base_ecam = QEMU_ALIGN_DOWN(end - size_ecam, size_ecam);
> +    size_ecam = end - base_ecam;

Why do we do this? If we've had to align down a 256MB ECAM
that doesn't mean the space gets bigger -- it's still 256MB.

> +    base_ioport = QEMU_ALIGN_DOWN(end - size_ecam - size_ioport, 
> size_ioport);

Shouldn't the first argument here be 'base_ecam - size_ioport' ?
Otherwise if we did have to align down the ECAM window we
won't have started the ioport far enough down...

> +    size_ioport = base_ecam - base_ioport;

Again, I don't think the ioport size should get bigger.

> +    size_mmio = base_ioport - base;
> +
> +    dev = qdev_create(NULL, TYPE_GPEX_HOST);
> +    qdev_init_nofail(dev);
> +
> +    /* Map only the first size_ecam bytes of ECAM space */
> +    ecam_alias = g_new0(MemoryRegion, 1);
> +    ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +    memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
> +                             ecam_reg, 0, size_ecam);
> +    memory_region_add_subregion(get_system_memory(), base_ecam, ecam_alias);
> +
> +    /* Map the MMIO window at the same spot in bus and cpu layouts */

I still think this comment is a little cryptic. I suggest:

    /* Map the MMIO window into system address space so as to expose
     * the section of PCI MMIO space which starts at the same base address
     * (ie 1:1 mapping for that part of PCI MMIO space visible through
     * the window).
     */

> +    mmio_alias = g_new0(MemoryRegion, 1);
> +    mmio_reg = 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);
> +
> +    /* Map IO port space */
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_ioport);
> +
> +    for (i = 0; i < GPEX_NUM_IRQS; i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]);
> +    }
> +
> +    nodename = g_strdup_printf("/address@hidden" PRIx64, base);
> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
> +    qemu_fdt_setprop_string(vbi->fdt, nodename,
> +                            "compatible", "pci-host-ecam-generic");
> +    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);

Is this right? It doesn't seem to match the expanded eCAM size
we've used above. (Maybe we should calculate this based on the
size_ecam or something?)

> +
> +    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, FDT_PCI_RANGE_IOPORT, 2, 0,
> +                                 2, base_ioport, 2, size_ioport,
> +

I guess this blank line is deliberate to separate out the io from
the mmio part of this definition...

> +                                 1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> +                                 2, base_mmio, 2, size_mmio);
> +
> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
> +    create_pcie_irq_map(vbi, gic_phandle, irq, nodename);
> +
> +    g_free(nodename);
> +}

Otherwise looks OK.

thanks
-- PMM