[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3] pci: removed the is_express field since a un
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH V3] pci: removed the is_express field since a uniform interface was inserted |
Date: |
Mon, 11 Dec 2017 12:19:05 -0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Dec 11, 2017 at 03:11:39PM +0200, Yoni Bettan wrote:
>
>
> On 12/07/2017 10:58 PM, Eduardo Habkost wrote:
> > On Tue, Dec 05, 2017 at 07:17:06PM +0200, Yoni Bettan wrote:
> > > * according to Eduardo Habkost's commit
> > > fd3b02c8896d597dd8b9e053dec579cf0386aee1
> > >
> > > * since all PCIEs now implement INTERFACE_PCIE_DEVICE we
> > > don't need this field anymore
> > >
> > > * Devices that where only INTERFACE_PCIE_DEVICE (is_express == 1)
> > > or
> > > devices that where only INTERFACE_CONVENTIONAL_PCI_DEVICE
> > > (is_express == 0)
> > > where not affected by the change
> > >
> > > The only devices that were affected are those that are hybrid
> > > and also
> > > had (is_express == 1) - therefor only:
> > > - hw/vfio/pci.c
> > > - hw/usb/hcd-xhci.c
> > >
> > > For both I made sure that QEMU_PCI_CAP_EXPRESS is on
> > >
> > > Signed-off-by: Yoni Bettan <address@hidden>
> > > ---
> > > docs/pcie_pci_bridge.txt | 2 +-
> > > hw/block/nvme.c | 1 -
> > > hw/net/e1000e.c | 1 -
> > > hw/pci-bridge/pcie_pci_bridge.c | 1 -
> > > hw/pci-bridge/pcie_root_port.c | 1 -
> > > hw/pci-bridge/xio3130_downstream.c | 1 -
> > > hw/pci-bridge/xio3130_upstream.c | 1 -
> > > hw/pci-host/xilinx-pcie.c | 1 -
> > > hw/pci/pci.c | 4 +++-
> > > hw/scsi/megasas.c | 4 ----
> > > hw/usb/hcd-xhci.c | 7 ++++++-
> > > hw/vfio/pci.c | 3 ++-
> > > include/hw/pci/pci.h | 3 ---
> > > 13 files changed, 12 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt
> > > index 5a4203f97c..ab35ebf3ca 100644
> > > --- a/docs/pcie_pci_bridge.txt
> > > +++ b/docs/pcie_pci_bridge.txt
> > > @@ -110,5 +110,5 @@ To enable device hot-plug into the bridge on Linux
> > > there're 3 ways:
> > > Implementation
> > > ==============
> > > The PCIE-PCI bridge is based on PCI-PCI bridge, but also accumulates
> > > PCI Express
> > > -features as a PCI Express device (is_express=1).
> > > +features as a PCI Express device.
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 441e21ed1f..9325bc0911 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -1087,7 +1087,6 @@ static void nvme_class_init(ObjectClass *oc, void
> > > *data)
> > > pc->vendor_id = PCI_VENDOR_ID_INTEL;
> > > pc->device_id = 0x5845;
> > > pc->revision = 2;
> > > - pc->is_express = 1;
> > > set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > > dc->desc = "Non-Volatile Memory Express";
> > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > > index f1af279e8d..c360f0d8c9 100644
> > > --- a/hw/net/e1000e.c
> > > +++ b/hw/net/e1000e.c
> > > @@ -675,7 +675,6 @@ static void e1000e_class_init(ObjectClass *class,
> > > void *data)
> > > c->revision = 0;
> > > c->romfile = "efi-e1000e.rom";
> > > c->class_id = PCI_CLASS_NETWORK_ETHERNET;
> > > - c->is_express = 1;
> > > dc->desc = "Intel 82574L GbE Controller";
> > > dc->reset = e1000e_qdev_reset;
> > > diff --git a/hw/pci-bridge/pcie_pci_bridge.c
> > > b/hw/pci-bridge/pcie_pci_bridge.c
> > > index a4d827c99d..b7d9ebbec2 100644
> > > --- a/hw/pci-bridge/pcie_pci_bridge.c
> > > +++ b/hw/pci-bridge/pcie_pci_bridge.c
> > > @@ -169,7 +169,6 @@ static void pcie_pci_bridge_class_init(ObjectClass
> > > *klass, void *data)
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > > - k->is_express = 1;
> > > k->is_bridge = 1;
> > > k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
> > > diff --git a/hw/pci-bridge/pcie_root_port.c
> > > b/hw/pci-bridge/pcie_root_port.c
> > > index 9b6e4ce512..45f9e8cd4a 100644
> > > --- a/hw/pci-bridge/pcie_root_port.c
> > > +++ b/hw/pci-bridge/pcie_root_port.c
> > > @@ -145,7 +145,6 @@ static void rp_class_init(ObjectClass *klass, void
> > > *data)
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > - k->is_express = 1;
> > > k->is_bridge = 1;
> > > k->config_write = rp_write_config;
> > > k->realize = rp_realize;
> > > diff --git a/hw/pci-bridge/xio3130_downstream.c
> > > b/hw/pci-bridge/xio3130_downstream.c
> > > index 1e09d2afb7..613a0d6bb7 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -177,7 +177,6 @@ static void xio3130_downstream_class_init(ObjectClass
> > > *klass, void *data)
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > - k->is_express = 1;
> > > k->is_bridge = 1;
> > > k->config_write = xio3130_downstream_write_config;
> > > k->realize = xio3130_downstream_realize;
> > > diff --git a/hw/pci-bridge/xio3130_upstream.c
> > > b/hw/pci-bridge/xio3130_upstream.c
> > > index 227997ce46..d4645bddee 100644
> > > --- a/hw/pci-bridge/xio3130_upstream.c
> > > +++ b/hw/pci-bridge/xio3130_upstream.c
> > > @@ -148,7 +148,6 @@ static void xio3130_upstream_class_init(ObjectClass
> > > *klass, void *data)
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > - k->is_express = 1;
> > > k->is_bridge = 1;
> > > k->config_write = xio3130_upstream_write_config;
> > > k->realize = xio3130_upstream_realize;
> > > diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> > > index 7659253090..a4ca3ba30f 100644
> > > --- a/hw/pci-host/xilinx-pcie.c
> > > +++ b/hw/pci-host/xilinx-pcie.c
> > > @@ -298,7 +298,6 @@ static void xilinx_pcie_root_class_init(ObjectClass
> > > *klass, void *data)
> > > k->device_id = 0x7021;
> > > k->revision = 0;
> > > k->class_id = PCI_CLASS_BRIDGE_HOST;
> > > - k->is_express = true;
> > > k->is_bridge = true;
> > > k->init = xilinx_pcie_root_init;
> > > k->exit = pci_bridge_exitfn;
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index b2d139bd9a..6b5676b0f4 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2014,12 +2014,14 @@ static void pci_qdev_realize(DeviceState *qdev,
> > > Error **errp)
> > > {
> > > PCIDevice *pci_dev = (PCIDevice *)qdev;
> > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> > > + ObjectClass *klass = OBJECT_CLASS(pc);
> > > Error *local_err = NULL;
> > > PCIBus *bus;
> > > bool is_default_rom;
> > > /* initialize cap_present for pci_is_express() and
> > > pci_config_size() */
> > > - if (pc->is_express) {
> > > + if (object_class_dynamic_cast(klass, INTERFACE_PCIE_DEVICE) &&
> > > + !object_class_dynamic_cast(klass,
> > > INTERFACE_CONVENTIONAL_PCI_DEVICE)) {
> > > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > I suggest a comment here explaining that hybrid devices must
> > manage QEMU_PCI_CAP_EXPRESS manually themselves.
> It is a good idea, I will do it!
> > > }
> > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > > index d5eae6239a..ee51feda59 100644
> > > --- a/hw/scsi/megasas.c
> > > +++ b/hw/scsi/megasas.c
> > > @@ -2447,7 +2447,6 @@ typedef struct MegasasInfo {
> > > uint16_t subsystem_id;
> > > int ioport_bar;
> > > int mmio_bar;
> > > - bool is_express;
> > > int osts;
> > > const VMStateDescription *vmsd;
> > > Property *props;
> > > @@ -2465,7 +2464,6 @@ static struct MegasasInfo megasas_devices[] = {
> > > .ioport_bar = 2,
> > > .mmio_bar = 0,
> > > .osts = MFI_1078_RM | 1,
> > > - .is_express = false,
> > > .vmsd = &vmstate_megasas_gen1,
> > > .props = megasas_properties_gen1,
> > > .interfaces = (InterfaceInfo[]) {
> > > @@ -2482,7 +2480,6 @@ static struct MegasasInfo megasas_devices[] = {
> > > .ioport_bar = 0,
> > > .mmio_bar = 1,
> > > .osts = MFI_GEN2_RM,
> > > - .is_express = true,
> > > .vmsd = &vmstate_megasas_gen2,
> > > .props = megasas_properties_gen2,
> > > .interfaces = (InterfaceInfo[]) {
> > > @@ -2506,7 +2503,6 @@ static void megasas_class_init(ObjectClass *oc,
> > > void *data)
> > > pc->subsystem_vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
> > > pc->subsystem_id = info->subsystem_id;
> > > pc->class_id = PCI_CLASS_STORAGE_RAID;
> > > - pc->is_express = info->is_express;
> > > e->mmio_bar = info->mmio_bar;
> > > e->ioport_bar = info->ioport_bar;
> > > e->osts = info->osts;
> > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > > index af3a9d88de..2e4dd71248 100644
> > > --- a/hw/usb/hcd-xhci.c
> > > +++ b/hw/usb/hcd-xhci.c
> > > @@ -3649,6 +3649,11 @@ static Property xhci_properties[] = {
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > > +static void xhci_instance_init(Object *obj)
> > > +{
> > > + PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > I suggest adding a comment explaining why we need to set
> > QEMU_PCI_CAP_EXPRESS manually here.
> >
> > I just noticed that every other device that sets/unsets
> > QEMU_PCI_CAP_EXPRESS do it on realize:
> >
> > $ g grep -p QEMU_PCI_CAP_EXPRESS
> > hw/net/vmxnet3.c=static void vmxnet3_realize(DeviceState *qdev, Error
> > **errp)
> > hw/net/vmxnet3.c: pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > hw/pci/pci.c=static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> > hw/pci/pci.c: pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > hw/scsi/vmw_pvscsi.c=static void pvscsi_realize(DeviceState *qdev, Error
> > **errp)
> > hw/scsi/vmw_pvscsi.c: pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > hw/vfio/pci.c=static void vfio_populate_device(VFIOPCIDevice *vdev, Error
> > **errp)
> > hw/vfio/pci.c: vdev->pdev.cap_present &= ~QEMU_PCI_CAP_EXPRESS;
> > hw/virtio/virtio-pci.c=static void virtio_pci_realize(PCIDevice *pci_dev,
> > Error **errp)
> > hw/virtio/virtio-pci.c: pci_dev->cap_present &=
> > ~QEMU_PCI_CAP_EXPRESS;
> > hw/virtio/virtio-pci.c=static void virtio_pci_dc_realize(DeviceState *qdev,
> > Error **errp)
> > hw/virtio/virtio-pci.c: pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > include/hw/pci/pci.h=enum {
> > include/hw/pci/pci.h: QEMU_PCI_CAP_EXPRESS = 0x4,
> > include/hw/pci/pci.h=static inline int pci_is_express(const PCIDevice *d)
> > include/hw/pci/pci.h: return d->cap_present & QEMU_PCI_CAP_EXPRESS;
>
> Those devices are initialized in instance_init rather than in realize
> because unlike other
> devices that are initialized in realize those devices are not a function of
> the Qemu command
> line so we don't need to wait to the realize function.
Thanks, I think now I get it: vmxnet3, pvscsi and virtio-pci do
set QEMU_PCI_CAP_EXPRESS conditionally, but vfio and xhci don't.
Now, my question is: why is this inconsistent? Can't we make all
devices use the same mechanisms to enable/disable
QEMU_PCI_CAP_EXPRESS, including xhci and vfio?
>
> I added a comment about it.
> >
> > We probably should address this inconsistency, while being
> > careful to not introduce compatibility bugs. Probably
> > pci_config_alloc() is with the QEMU_PCI_CAP_EXPRESS cleared is on
> > vmxnet3, pvscsi, and virtio-pci?
>
> I am not sure I understood what you meant about the pci_config_alloc()
> function.
I was confused because this gets called before
PCIDeviceClass::realize:
pci_qdev_realize() -> do_pci_register_device() -> pci_config_alloc()
-> pci_config_size() -> pci_is_express()
But my mistake was to assume that pvscsi_realize(),
virtio_pci_dc_realize() and vmxnet3_realize() were
PCIDeviceClass::realize. They are not: they are
DeviceClass::realize methods, and run before pci_qdev_realize().
Anyway, I think this is confusing and requires too much
boilerplate code on the hybrid devices. We could have a common
mechanism for hybrid devices to enable/disable
QEMU_PCI_CAP_EXPRESS, instead of requiring each device to
reimplement DeviceClass::realize?
Note that I'm just speculating about potential cleanups for the
future. Your patch is still a step in the right direction.
> >
> >
> > > +}
> > > +
> > > static void xhci_class_init(ObjectClass *klass, void *data)
> > > {
> > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > @@ -3661,7 +3666,6 @@ static void xhci_class_init(ObjectClass *klass,
> > > void *data)
> > > k->realize = usb_xhci_realize;
> > > k->exit = usb_xhci_exit;
> > > k->class_id = PCI_CLASS_SERIAL_USB;
> > > - k->is_express = 1;
> > > }
> > > static const TypeInfo xhci_info = {
> > > @@ -3669,6 +3673,7 @@ static const TypeInfo xhci_info = {
> > > .parent = TYPE_PCI_DEVICE,
> > > .instance_size = sizeof(XHCIState),
> > > .class_init = xhci_class_init,
> > > + .instance_init = xhci_instance_init,
> > > .abstract = true,
> > > .interfaces = (InterfaceInfo[]) {
> > > { INTERFACE_PCIE_DEVICE },
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index c977ee327f..afad0c002f 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2972,6 +2972,8 @@ static void vfio_instance_init(Object *obj)
> > > vdev->host.function = ~0U;
> > > vdev->nv_gpudirect_clique = 0xFF;
> > > +
> > > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > Same as above: a comment explaining why this is needed would be
> > useful.
> same as above: I added a comment
> >
> > > }
> > > static Property vfio_pci_dev_properties[] = {
> > > @@ -3026,7 +3028,6 @@ static void vfio_pci_dev_class_init(ObjectClass
> > > *klass, void *data)
> > > pdc->exit = vfio_exitfn;
> > > pdc->config_read = vfio_pci_read_config;
> > > pdc->config_write = vfio_pci_write_config;
> > > - pdc->is_express = 1; /* We might be */
> > > }
> > > static const TypeInfo vfio_pci_dev_info = {
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index 8d02a0a383..a27be85111 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -236,9 +236,6 @@ typedef struct PCIDeviceClass {
> > > */
> > > int is_bridge;
> > > - /* pcie stuff */
> > > - int is_express; /* is this device pci express? */
> > > -
> > > /* rom bar */
> > > const char *romfile;
> > > } PCIDeviceClass;
> > > --
> > > 2.14.3
> > >
> > >
>
--
Eduardo