* [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
@ 2015-11-10 11:41 Marcel Apfelbaum
2015-12-01 10:29 ` Shmulik Ladkani
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-11-10 11:41 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, kraxel, ehabkost, mst
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(+)
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 \
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-11-10 11:41 [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices Marcel Apfelbaum
@ 2015-12-01 10:29 ` Shmulik Ladkani
2015-12-01 11:48 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Shmulik Ladkani @ 2015-12-01 10:29 UTC (permalink / raw)
To: Marcel Apfelbaum, mst; +Cc: qemu-devel, ehabkost, kraxel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-12-01 10:29 ` Shmulik Ladkani
@ 2015-12-01 11:48 ` Marcel Apfelbaum
2015-12-01 13:18 ` Shmulik Ladkani
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-12-01 11:48 UTC (permalink / raw)
To: Shmulik Ladkani, mst; +Cc: qemu-devel, ehabkost, kraxel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-12-01 11:48 ` Marcel Apfelbaum
@ 2015-12-01 13:18 ` Shmulik Ladkani
2015-12-01 14:01 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Shmulik Ladkani @ 2015-12-01 13:18 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: kraxel, qemu-devel, ehabkost, mst
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-12-01 13:18 ` Shmulik Ladkani
@ 2015-12-01 14:01 ` Marcel Apfelbaum
2015-12-01 14:19 ` Shmulik Ladkani
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-12-01 14:01 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: kraxel, qemu-devel, ehabkost, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-12-01 14:01 ` Marcel Apfelbaum
@ 2015-12-01 14:19 ` Shmulik Ladkani
2015-12-01 14:25 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Shmulik Ladkani @ 2015-12-01 14:19 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: kraxel, qemu-devel, ehabkost, mst
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices
2015-12-01 14:19 ` Shmulik Ladkani
@ 2015-12-01 14:25 ` Marcel Apfelbaum
0 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-12-01 14:25 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: kraxel, qemu-devel, ehabkost, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-01 14:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-10 11:41 [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices Marcel Apfelbaum
2015-12-01 10:29 ` Shmulik Ladkani
2015-12-01 11:48 ` Marcel Apfelbaum
2015-12-01 13:18 ` Shmulik Ladkani
2015-12-01 14:01 ` Marcel Apfelbaum
2015-12-01 14:19 ` Shmulik Ladkani
2015-12-01 14:25 ` Marcel Apfelbaum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.