Re: [Qemu-devel] [PATCH v2 2/4] pci: Add generic PCIe host bridge

[Peter Maydell]

On 21 January 2015 at 16:18, Alexander Graf <address@hidden> wrote:
> With simple exposure of MMFG, ioport window, mmio window and an IRQ line we

Four :-)

> can successfully create a workable PCIe host bridge that can be mapped 
> anywhere
> and only needs to get described to the OS using whatever means it likes.
>
> This patch implements such a "generic" host bridge. It only supports a single
> legacy IRQ line so far. MSIs need to be handled external to the host bridge.
>
> This device is particularly useful for the "pci-host-ecam-generic" driver in
> Linux.
>
> Signed-off-by: Alexander Graf <address@hidden>
> Reviewed-by: Claudio Fontana <address@hidden>
> Tested-by: Claudio Fontana <address@hidden>

Checkpatch complains about a few over-80-chars lines;
the URL is an unavoidable one but could you fold the others,
please?

> +static void gpex_host_realize(DeviceState *dev, Error **errp)
> +{
> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> +    GPEXHost *s = GPEX_HOST(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
> +    int i;
> +
> +    pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MIN);

So this gives us 1MB of ECAM (config) space. That means
we can't specify a target bus, so we're restricted to
the 31 devices directly attached to the root complex.
Among other things, this will mean we can't do PCIe
hotplug. I think we should make this at least a bit bigger,
even if we don't go up to the full 256MB.

Probably the best thing for the gpex device itself is
to make the config space be the spec maximum (PCIE_MMCFG_SIZE_MAX)
and let the user map only a portion of that into their
address space if they want.

Ideally we should just do that in the base class, and
get the q35 subclass to do the right thing with aliases
to handle the dynamic resizing it wants. Then we wouldn't
need to track "size of cfg region" in the baseclass either.

> +    memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
> +    memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
> +
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +    sysbus_init_mmio(sbd, &s->io_mmio);
> +    sysbus_init_mmio(sbd, &s->io_ioport);
> +    for (i = 0; i < 4; i++) {

Can we at least have a constant rather than hardcoded 4s ?
(qdev property for number-of-irqs if you're really feeling
enthusiastic...)

> +
> +/****************************************************************************
> + * GPEX Root D0:F0
> + */

What does "D0:F0" mean here?

> +
> +static const VMStateDescription vmstate_gpex_root = {
> +    .name = "gpex_root",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(parent_obj, GPEXRootState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void gpex_root_class_init(ObjectClass *klass, void *data)
> +{
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "Host bridge";

We can be a bit less terse: "QEMU generic PCIe host bridge".

> +    dc->vmsd = &vmstate_gpex_root;
> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +    k->device_id = PCI_DEVICE_ID_REDHAT_BRIDGE;

Pretty sure we shouldn't be reusing the PCI bridge IDs
for a host bridge. We should allocate ourselves another
device ID in the range...

> +    k->revision = 0;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
> +}
> +
> +static const TypeInfo gpex_root_info = {
> +    .name = TYPE_GPEX_ROOT_DEVICE,
> +    .parent = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(GPEXRootState),
> +    .class_init = gpex_root_class_init,
> +};
> +
> +static void gpex_register(void)
> +{
> +    type_register_static(&gpex_root_info);
> +    type_register_static(&gpex_host_info);
> +}
> +
> +type_init(gpex_register);

We seem to prefer no trailing ';' on type_init by a ratio
of more than 10:1.

> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> new file mode 100644
> index 0000000..b465667
> --- /dev/null
> +++ b/include/hw/pci-host/gpex.h
> @@ -0,0 +1,54 @@
> +/*
> + * gpex.h

Would be nice to say "QEMU Generic PCI Express Bridge Emulation" here
(like the .c file's header does).

thanks
-- PMM