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 <ybet...@redhat.com> > > > --- > > > 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