On 27.01.15 16:31, Peter Maydell wrote:
> On 21 January 2015 at 16:18, Alexander Graf <ag...@suse.de> 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 <ag...@suse.de>
>> Reviewed-by: Claudio Fontana <claudio.font...@huawei.com>
>> Tested-by: Claudio Fontana <claudio.font...@huawei.com>
>
> 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
