| Message ID | 1447155689-26230-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Tue, 10 Nov 2015 13:41:29 +0200, marcel@redhat.com wrote: > The virtio devices are converted to PCI-Express > if they are plugged into a PCI-Express bus and > the 'modern' protocol is enabled. > > @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) > + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > + && pci_bus_is_express(pci_dev->bus) > + && !pci_bus_is_root(pci_dev->bus)) { > + int pos; > + > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; Setting QEMU_PCI_CAP_EXPRESS here in 'virtio_pci_realize' is too late. This is since 'pci_qdev_realize' (DeviceClass.realize of TYPE_PCI_DEVICE) is invoked prior the PCIDeviceClass's specific 'realize' method (virtio_pci_realize in this case). During 'pci_qdev_realize' (specifically, within do_pci_register_device), the QEMU_PCI_CAP_EXPRESS gets tested indirectly, when pci_is_express and pci_config_size helpers are called. For example: 'pci_config_alloc' uses 'pci_config_size' which relies on QEMU_PCI_CAP_EXPRESS property. Since virtio_pci sets QEMU_PCI_CAP_EXPRESS *after* pci_qdev_realize has finished, we end up having an insufficient pci config space allocated for the virtio "pcie" device. May I suggest the following: - Expose 'pci_qdev_realize' - Have 'virtio_pci_class_init' arm it's own dc->realize, which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, and then call 'pci_qdev_realize' - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of directly checking the proxy->flags If this sounds ok, I'll submit a fix. Regards, Shmulik
On 12/01/2015 12:29 PM, Shmulik Ladkani wrote: > Hi, > > On Tue, 10 Nov 2015 13:41:29 +0200, marcel@redhat.com wrote: >> The virtio devices are converted to PCI-Express >> if they are plugged into a PCI-Express bus and >> the 'modern' protocol is enabled. >> >> @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) >> >> + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) >> + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) >> + && pci_bus_is_express(pci_dev->bus) >> + && !pci_bus_is_root(pci_dev->bus)) { >> + int pos; >> + >> + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > Setting QEMU_PCI_CAP_EXPRESS here in 'virtio_pci_realize' is too late. > > This is since 'pci_qdev_realize' (DeviceClass.realize of TYPE_PCI_DEVICE) > is invoked prior the PCIDeviceClass's specific 'realize' method > (virtio_pci_realize in this case). > > During 'pci_qdev_realize' (specifically, within do_pci_register_device), > the QEMU_PCI_CAP_EXPRESS gets tested indirectly, when pci_is_express > and pci_config_size helpers are called. > > For example: 'pci_config_alloc' uses 'pci_config_size' which relies on > QEMU_PCI_CAP_EXPRESS property. > > Since virtio_pci sets QEMU_PCI_CAP_EXPRESS *after* pci_qdev_realize > has finished, we end up having an insufficient pci config space > allocated for the virtio "pcie" device. Hi, Thanks for catching it. It is a good time for a fix since we don't have yet an actual PCIe express capability. > > May I suggest the following: > - Expose 'pci_qdev_realize' > - Have 'virtio_pci_class_init' arm it's own dc->realize, > which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, > and then call 'pci_qdev_realize' > - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of > directly checking the proxy->flags > > If this sounds ok, I'll submit a fix. Give it a try, sure, but you don't need to expose pci_qdev_realize, you can 'hijack' parent's realize method before replacing it. Thanks, Marcel > > Regards, > Shmulik >
Hi, On Tue, 1 Dec 2015 13:48:13 +0200, marcel@redhat.com wrote: > > May I suggest the following: > > - Expose 'pci_qdev_realize' > > - Have 'virtio_pci_class_init' arm it's own dc->realize, > > which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, > > and then call 'pci_qdev_realize' > > - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of > > directly checking the proxy->flags > > > > If this sounds ok, I'll submit a fix. > > Give it a try, sure, but you don't need to expose pci_qdev_realize, > you can 'hijack' parent's realize method before replacing it. Actually I caught this as I needed to do something very similar to vmxnet3. Since we need this in 2 places, I thought it would be better to expose pci_qdev_realize (maybe name it better) and not store parent's original realize before replacing it. WDYT?
On 12/01/2015 03:18 PM, Shmulik Ladkani wrote: > Hi, > > On Tue, 1 Dec 2015 13:48:13 +0200, marcel@redhat.com wrote: >>> May I suggest the following: >>> - Expose 'pci_qdev_realize' >>> - Have 'virtio_pci_class_init' arm it's own dc->realize, >>> which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, >>> and then call 'pci_qdev_realize' >>> - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of >>> directly checking the proxy->flags >>> >>> If this sounds ok, I'll submit a fix. >> >> Give it a try, sure, but you don't need to expose pci_qdev_realize, >> you can 'hijack' parent's realize method before replacing it. > > Actually I caught this as I needed to do something very similar to > vmxnet3. > > Since we need this in 2 places, I thought it would be better to expose > pci_qdev_realize (maybe name it better) and not store parent's original > realize before replacing it. > > WDYT? > Personally I don't think pci_qdev_realize should be visible outside hw/pci/pci.c because "realize" should be private to object... However I didn't see yet the vmxnet3 changes you propose. Maybe you can send an RFC and we'll check then. Thanks, Marcel
Hi, On Tue, 1 Dec 2015 16:01:00 +0200, marcel@redhat.com wrote: > On 12/01/2015 03:18 PM, Shmulik Ladkani wrote: > > On Tue, 1 Dec 2015 13:48:13 +0200, marcel@redhat.com wrote: > >>> May I suggest the following: > >>> - Expose 'pci_qdev_realize' > >>> - Have 'virtio_pci_class_init' arm it's own dc->realize, > >>> which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, > >>> and then call 'pci_qdev_realize' > >>> - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of > >>> directly checking the proxy->flags > >>> > >>> If this sounds ok, I'll submit a fix. > >> > >> Give it a try, sure, but you don't need to expose pci_qdev_realize, > >> you can 'hijack' parent's realize method before replacing it. > > > > Actually I caught this as I needed to do something very similar to > > vmxnet3. > > > > Since we need this in 2 places, I thought it would be better to expose > > pci_qdev_realize (maybe name it better) and not store parent's original > > realize before replacing it. > > > > WDYT? > > Personally I don't think pci_qdev_realize should be visible outside hw/pci/pci.c > because "realize" should be private to object... Thanks I'll follow that path and submit a patch shortly.
On 12/01/2015 04:19 PM, Shmulik Ladkani wrote: > Hi, > > On Tue, 1 Dec 2015 16:01:00 +0200, marcel@redhat.com wrote: >> On 12/01/2015 03:18 PM, Shmulik Ladkani wrote: >>> On Tue, 1 Dec 2015 13:48:13 +0200, marcel@redhat.com wrote: >>>>> May I suggest the following: >>>>> - Expose 'pci_qdev_realize' >>>>> - Have 'virtio_pci_class_init' arm it's own dc->realize, >>>>> which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed, >>>>> and then call 'pci_qdev_realize' >>>>> - Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of >>>>> directly checking the proxy->flags >>>>> >>>>> If this sounds ok, I'll submit a fix. >>>> >>>> Give it a try, sure, but you don't need to expose pci_qdev_realize, >>>> you can 'hijack' parent's realize method before replacing it. >>> >>> Actually I caught this as I needed to do something very similar to >>> vmxnet3. >>> >>> Since we need this in 2 places, I thought it would be better to expose >>> pci_qdev_realize (maybe name it better) and not store parent's original >>> realize before replacing it. >>> >>> WDYT? >> >> Personally I don't think pci_qdev_realize should be visible outside hw/pci/pci.c >> because "realize" should be private to object... > > Thanks I'll follow that path and submit a patch shortly. > Thanks and please post the fix before our next rc. If you don't have the time, please let me know and I'll submit a fix. Thanks again! Marcel
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index f55dd2b..ba02e25 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) + && pci_bus_is_express(pci_dev->bus) + && !pci_bus_is_root(pci_dev->bus)) { + int pos; + + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; + pos = pcie_endpoint_cap_init(pci_dev, 0); + assert(pos > 0); + + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); + assert(pos > 0); + + /* + * Indicates that this function complies with revision 1.2 of the + * PCI Power Management Interface Specification. + */ + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); + } + virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); if (k->realize) { k->realize(proxy, errp); @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), + DEFINE_PROP_BIT("x-disable-pcie", VirtIOPCIProxy, flags, + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 801c23a..1a487fc 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; /* virtio version flags */ #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) typedef struct { MSIMessage msg; diff --git a/include/hw/compat.h b/include/hw/compat.h index 93e71af..3dfdb60 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -6,6 +6,10 @@ .driver = "virtio-blk-device",\ .property = "scsi",\ .value = "true",\ + },{\ + .driver = "virtio-pci",\ + .property = "x-disable-pcie",\ + .value = "on",\ }, #define HW_COMPAT_2_3 \
The virtio devices are converted to PCI-Express if they are plugged into a PCI-Express bus and the 'modern' protocol is enabled. Devices plugged directly into the Root Complex as Integrated Endpoints remain PCI. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- v4 -> v5: - Addressed Michael S. Tsirkin's comments: - Renamed disable-pcie => x-disable-pcie - Rebased on master v3 -> v4: - Addressed Eduardo Habkost's comments: - used a single virtio-pci.disable-pcie=on entry for HW_COMPAT, instead of one entry for each subclass v2 -> v3: - Addressed Michael S. Tsirkin's comments: - enable pcie only for 2.5+ machines. v1 -> v2: - Addressed Michael S. Tsirkin's comments: - Added the minimum required capabilities for PCIe devices - Integrated Endpoints remain PCI - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. - Regarding Gerd Hoffman's comments: - Creating virtio-pcie devices: For the moment I prefer to not duplicate the virtio definitions, at least until we don't have a consensus (Personally I don't like it) - Removing the IO bar: This would be my next patch on the "virtio to express" series, I plan to remove it only for "modern" devices. Thanks, Marcel hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ hw/virtio/virtio-pci.h | 2 ++ include/hw/compat.h | 4 ++++ 3 files changed, 28 insertions(+)