| Message ID | 20171213082644.83113-1-ben@skyportsystems.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-pci: Add subsystem-vendor-id property | expand |
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?
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
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
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 >
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.
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
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)