[Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[ben]

From: Ben Warren <ben@skyportsystems.com>

Now that virtio-win guest drivers provided by non-Redhat vendors need to
use a different Subsystem Vendor ID value, a way is needed to set this
parameter on the host.  This works with all of the PCI-based devices,
such as NetKVM, viostor, vioscsi, vioserial and balloon.

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hw/virtio/virtio-pci.c | 5 +++++
 hw/virtio/virtio-pci.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e92837c..b5c86e3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
     if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
         proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
+    /* Set the PCI Subsystem Vendor ID */
+    k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
+    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, proxy->subsystem_vendor_id);
 
     if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
         error_setg(errp, "device cannot work as neither modern nor legacy mode"
@@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
                     VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
     DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
+    DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
+                       subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 12d3a90..8a897ea 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
     VirtIOIRQFD *vector_irqfd;
     int nvqs_with_notifiers;
     VirtioBusState bus;
+    uint16_t subsystem_vendor_id;
 };
 
 static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

On Wed, Dec 13, 2017 at 12:26:44AM -0800, Ben Warren via Qemu-devel wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> Now that virtio-win guest drivers provided by non-Redhat vendors need to
> use a different Subsystem Vendor ID value, a way is needed to set this
> parameter on the host.  This works with all of the PCI-based devices,
> such as NetKVM, viostor, vioscsi, vioserial and balloon.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  hw/virtio/virtio-pci.c | 5 +++++
>  hw/virtio/virtio-pci.h | 1 +
>  2 files changed, 6 insertions(+)

Is it possible to put this property in hw/pci/pci.c:pci_props[] so that
users can override the Subsystem Vendor ID for any PCI adapter, not just
virtio-pci?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Michael S. Tsirkin]

On Wed, Dec 13, 2017 at 10:07:12AM +0000, Stefan Hajnoczi wrote:
> On Wed, Dec 13, 2017 at 12:26:44AM -0800, Ben Warren via Qemu-devel wrote:
> > From: Ben Warren <ben@skyportsystems.com>
> > 
> > Now that virtio-win guest drivers provided by non-Redhat vendors need to
> > use a different Subsystem Vendor ID value, a way is needed to set this
> > parameter on the host.  This works with all of the PCI-based devices,
> > such as NetKVM, viostor, vioscsi, vioserial and balloon.
> > 
> > Signed-off-by: Ben Warren <ben@skyportsystems.com>
> > ---
> >  hw/virtio/virtio-pci.c | 5 +++++
> >  hw/virtio/virtio-pci.h | 1 +
> >  2 files changed, 6 insertions(+)
> 
> Is it possible to put this property in hw/pci/pci.c:pci_props[] so that
> users can override the Subsystem Vendor ID for any PCI adapter, not just
> virtio-pci?

The difficulty is that some devices override the ID you would set,
and there's no way for management to find out.
So I think virtio is preferable for now.

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Benjamin Warren]

Hi Michael,

It looks like this was never applied.  Can it be, please?

On Wed, Dec 20, 2017 at 9:24 AM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Dec 13, 2017 at 10:07:12AM +0000, Stefan Hajnoczi wrote:
> > On Wed, Dec 13, 2017 at 12:26:44AM -0800, Ben Warren via Qemu-devel
> wrote:
> > > From: Ben Warren <ben@skyportsystems.com>
> > >
> > > Now that virtio-win guest drivers provided by non-Redhat vendors need
> to
> > > use a different Subsystem Vendor ID value, a way is needed to set this
> > > parameter on the host.  This works with all of the PCI-based devices,
> > > such as NetKVM, viostor, vioscsi, vioserial and balloon.
> > >
> > > Signed-off-by: Ben Warren <ben@skyportsystems.com>
> > > ---
> > >  hw/virtio/virtio-pci.c | 5 +++++
> > >  hw/virtio/virtio-pci.h | 1 +
> > >  2 files changed, 6 insertions(+)
> >
> > Is it possible to put this property in hw/pci/pci.c:pci_props[] so that
> > users can override the Subsystem Vendor ID for any PCI adapter, not just
> > virtio-pci?
>
> The difficulty is that some devices override the ID you would set,
> and there's no way for management to find out.
> So I think virtio is preferable for now.
>

thanks,
Ben

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Michael S. Tsirkin]

On Wed, Dec 13, 2017 at 12:26:44AM -0800, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> Now that virtio-win guest drivers provided by non-Redhat vendors need to
> use a different Subsystem Vendor ID value, a way is needed to set this
> parameter on the host.  This works with all of the PCI-based devices,
> such as NetKVM, viostor, vioscsi, vioserial and balloon.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>

I applied a related patch by Ladi, pls take a look.

> ---
>  hw/virtio/virtio-pci.c | 5 +++++
>  hw/virtio/virtio-pci.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e92837c..b5c86e3 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>      if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>          proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
> +    /* Set the PCI Subsystem Vendor ID */
> +    k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
> +    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, proxy->subsystem_vendor_id);
>  
>      if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
>          error_setg(errp, "device cannot work as neither modern nor legacy mode"
> @@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
>                      VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>      DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>                      VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> +    DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
> +                       subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 12d3a90..8a897ea 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
>      VirtIOIRQFD *vector_irqfd;
>      int nvqs_with_notifiers;
>      VirtioBusState bus;
> +    uint16_t subsystem_vendor_id;
>  };
>  
>  static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Ben Warren]

[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]

Hi Michael,

> On Dec 19, 2017, at 8:27 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, Dec 13, 2017 at 12:26:44AM -0800, ben@skyportsystems.com wrote:
>> From: Ben Warren <ben@skyportsystems.com>
>> 
>> Now that virtio-win guest drivers provided by non-Redhat vendors need to
>> use a different Subsystem Vendor ID value, a way is needed to set this
>> parameter on the host.  This works with all of the PCI-based devices,
>> such as NetKVM, viostor, vioscsi, vioserial and balloon.
>> 
>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> 
> I applied a related patch by Ladi, pls take a look.
> 
If you’re talking about the one titled "virtio-pci: Don't force Subsystem Vendor ID = Vendor ID”, then this is complementary.  If it’s another one, I must have missed it.  The patch I’m aware of doesn’t provide any way of setting the subsystem vendor ID, which is needed.

BTW - would you prefer this to be contained to virtio-pci, or all of PCI, as another reviewer suggested.  Either is easy.

>> ---
>> hw/virtio/virtio-pci.c | 5 +++++
>> hw/virtio/virtio-pci.h | 1 +
>> 2 files changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index e92837c..b5c86e3 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>>     if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>>         proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>>     }
>> +    /* Set the PCI Subsystem Vendor ID */
>> +    k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
>> +    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, proxy->subsystem_vendor_id);
>> 
>>     if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
>>         error_setg(errp, "device cannot work as neither modern nor legacy mode"
>> @@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
>>                     VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>     DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>                     VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +    DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
>> +                       subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
>>     DEFINE_PROP_END_OF_LIST(),
>> };
>> 
>> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>> index 12d3a90..8a897ea 100644
>> --- a/hw/virtio/virtio-pci.h
>> +++ b/hw/virtio/virtio-pci.h
>> @@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
>>     VirtIOIRQFD *vector_irqfd;
>>     int nvqs_with_notifiers;
>>     VirtioBusState bus;
>> +    uint16_t subsystem_vendor_id;
>> };
>> 
>> static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
>> -- 
>> 2.7.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3866 bytes --]

Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Michael S. Tsirkin]

On Tue, Dec 19, 2017 at 09:11:31PM -0800, Ben Warren wrote:
> Hi Michael,
> 
> > On Dec 19, 2017, at 8:27 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Wed, Dec 13, 2017 at 12:26:44AM -0800, ben@skyportsystems.com wrote:
> >> From: Ben Warren <ben@skyportsystems.com>
> >> 
> >> Now that virtio-win guest drivers provided by non-Redhat vendors need to
> >> use a different Subsystem Vendor ID value, a way is needed to set this
> >> parameter on the host.  This works with all of the PCI-based devices,
> >> such as NetKVM, viostor, vioscsi, vioserial and balloon.
> >> 
> >> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> > 
> > I applied a related patch by Ladi, pls take a look.
> > 
> If you’re talking about the one titled "virtio-pci: Don't force Subsystem Vendor ID = Vendor ID”, then this is complementary.  If it’s another one, I must have missed it.  The patch I’m aware of doesn’t provide any way of setting the subsystem vendor ID, which is needed.
> 
> BTW - would you prefer this to be contained to virtio-pci, or all of PCI, as another reviewer suggested.  Either is easy.

Let me respond to that one.

> >> ---
> >> hw/virtio/virtio-pci.c | 5 +++++
> >> hw/virtio/virtio-pci.h | 1 +
> >> 2 files changed, 6 insertions(+)
> >> 
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index e92837c..b5c86e3 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> >>     if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
> >>         proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> >>     }
> >> +    /* Set the PCI Subsystem Vendor ID */
> >> +    k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
> >> +    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, proxy->subsystem_vendor_id);
> >> 
> >>     if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
> >>         error_setg(errp, "device cannot work as neither modern nor legacy mode"
> >> @@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
> >>                     VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
> >>     DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
> >>                     VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> >> +    DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
> >> +                       subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
> >>     DEFINE_PROP_END_OF_LIST(),
> >> };
> >> 
> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> >> index 12d3a90..8a897ea 100644
> >> --- a/hw/virtio/virtio-pci.h
> >> +++ b/hw/virtio/virtio-pci.h
> >> @@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
> >>     VirtIOIRQFD *vector_irqfd;
> >>     int nvqs_with_notifiers;
> >>     VirtioBusState bus;
> >> +    uint16_t subsystem_vendor_id;
> >> };
> >> 
> >> static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
> >> -- 
> >> 2.7.4
>