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

[Alexander Graf]

On 27.01.15 16:31, Peter Maydell wrote:
> 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?

Sure

> 
>> +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.

Sounds like a good plan. Will do.

> 
> 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...)

Sure.

> 
>> +
>> +/****************************************************************************
>> + * GPEX Root D0:F0
>> + */
> 
> What does "D0:F0" mean here?

Device 0 Function 0. It's pretty common PCI speech ;).

> 
>> +
>> +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...

Ah, right, I forgot about that. Unfortunately I can't reserve PCI IDs in
the Red Hat PCI range.

Michael, what's the process here?

> 
>> +    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.

Ok

> 
>> 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).

Ok


Alex