[Qemu-devel] [PATCH V2] hw/virtio: Add PCIe capability to virtio devices

[Qemu-devel] [PATCH V2] hw/virtio: Add PCIe capability to virtio devices

[Marcel Apfelbaum]

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>
---

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.

Any comments would be appreciated,
Thanks,
Marcel

 hw/virtio/virtio-pci.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f55dd2b..12d0ebb 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1592,6 +1592,25 @@ 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_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);
-- 
2.1.0

Re: [Qemu-devel] [PATCH V2] hw/virtio: Add PCIe capability to virtio devices

[Michael S. Tsirkin]

On Wed, Oct 28, 2015 at 07:16:06PM +0200, Marcel Apfelbaum wrote:
> 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>
> ---
> 
> 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.
> 
> Any comments would be appreciated,
> Thanks,
> Marcel
> 
>  hw/virtio/virtio-pci.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Looks good to me.  One minor point: need to make this conditional on 2.5
machine type.

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f55dd2b..12d0ebb 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1592,6 +1592,25 @@ 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_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);
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V2] hw/virtio: Add PCIe capability to virtio devices

[Marcel Apfelbaum]

On 10/28/2015 07:34 PM, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2015 at 07:16:06PM +0200, Marcel Apfelbaum wrote:
>> 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>
>> ---
>>
>> 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.
>>
>> Any comments would be appreciated,
>> Thanks,
>> Marcel
>>
>>   hw/virtio/virtio-pci.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>
> Looks good to me.  One minor point: need to make this conditional on 2.5
> machine type.

Hi Michael,

Makes sense to make this for 2.5+ machines.

Thanks for the review,
Marcel


>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index f55dd2b..12d0ebb 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1592,6 +1592,25 @@ 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_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);
>> --
>> 2.1.0