[PATCH 1/1] qxl: add subsystem_vendor_id property

[PATCH 1/1] qxl: add subsystem_vendor_id property

[Denis V. Lunev]

This property is needed for WHQL/inboxing of Windows drivers. We do need
to get drivers to be separated by the hypervisor vendors and that should
be done as PCI subvendor ID.

This patch adds PCI subsystem vendor ID to QXL device to match that
convention.

The original version of this code has been written by Denis Plotnikov
while he has been working in Virtuozzo.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Denis Plotnikov <den-plotnikov@yandex-team.ru>
CC: Yan Vugenfirer <yvugenfi@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 4 ++++
 hw/display/qxl.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 5b10f697f1..ec117aa90f 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2204,6 +2204,8 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
 
     qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
     qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
+
+    pci_set_word(&config[PCI_SUBSYSTEM_VENDOR_ID], qxl->subsystem_vendor_id);
 }
 
 static void qxl_realize_primary(PCIDevice *dev, Error **errp)
@@ -2469,6 +2471,8 @@ static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("xres", PCIQXLDevice, xres, 0),
         DEFINE_PROP_UINT32("yres", PCIQXLDevice, yres, 0),
         DEFINE_PROP_BOOL("global-vmstate", PCIQXLDevice, vga.global_vmstate, false),
+        DEFINE_PROP_UINT16("subsystem-vendor-id", PCIQXLDevice,
+                            subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index e74de9579d..111edbf0dc 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -126,6 +126,7 @@ struct PCIQXLDevice {
     int                num_dirty_rects;
     QXLRect            dirty[QXL_NUM_DIRTY_RECTS];
     QEMUBH            *update_area_bh;
+    uint16_t           subsystem_vendor_id;
 };
 
 #define TYPE_PCI_QXL "pci-qxl"
-- 
2.34.1

Re: [PATCH 1/1] qxl: add subsystem_vendor_id property

[Gerd Hoffmann]

On Wed, Sep 28, 2022 at 05:52:44PM +0200, Denis V. Lunev wrote:
> This property is needed for WHQL/inboxing of Windows drivers. We do need
> to get drivers to be separated by the hypervisor vendors and that should
> be done as PCI subvendor ID.
> 
> This patch adds PCI subsystem vendor ID to QXL device to match that
> convention.

We have pci_default_sub_vendor_id + pci_default_sub_device_id in
hw/pci/pci.c.  If you want another subsystem id for another vendor
there is a single place to change it for all devices.

Right now there is no runtime switch for them, so updating it requires
a two-liner patch for your vendor build.  We can discuss changing that,
but that should best be coordinated with libvirt folks to make sure
the management stack actually allows setting the subsystem id without
needing hacks.

take care,
  Gerd

Re: [PATCH 1/1] qxl: add subsystem_vendor_id property

[Denis V. Lunev]

On 9/29/22 09:37, Gerd Hoffmann wrote:
> On Wed, Sep 28, 2022 at 05:52:44PM +0200, Denis V. Lunev wrote:
>> This property is needed for WHQL/inboxing of Windows drivers. We do need
>> to get drivers to be separated by the hypervisor vendors and that should
>> be done as PCI subvendor ID.
>>
>> This patch adds PCI subsystem vendor ID to QXL device to match that
>> convention.
> We have pci_default_sub_vendor_id + pci_default_sub_device_id in
> hw/pci/pci.c.  If you want another subsystem id for another vendor
> there is a single place to change it for all devices.
>
> Right now there is no runtime switch for them, so updating it requires
> a two-liner patch for your vendor build.  We can discuss changing that,
> but that should best be coordinated with libvirt folks to make sure
> the management stack actually allows setting the subsystem id without
> needing hacks.
Yes. There is no runtime switch for it. I have also checked this.

The story here seems more complex. We are using in our
downstream the following patch from Ben Warren

https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg02128.html

and I have mistakenly thought that it was accepted in
the mainstream. OK, unfortunately that was not happen.
As this has been pointed out in the above thread
the discussion was moved into

https://patchwork.kernel.org/project/qemu-devel/patch/20171102133115.19195-1-lprosek@redhat.com/

Anyway, we need to support different PCI sub-vendor IDs
in order to be compliant with Microsoft WHQL rules. Though,
actually, at my opinion this requirement has nothing in
common with libvirt people. The most convenient way
here would be to specify these properties within vendor
machine types and this place is a perfect match as any
respectable has its own machine type.

I would also think that PCI level is not a good place for that
as we would not be able to apply this change blindly as at
PCI level this change would be too global and the same
was initially noted by Michael Tsirkin here

https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04384.html

Any thoughts?
What should we do with the original patch from Ben? We
still need an ability to expose vendor identity in QXL/virtio...

Den

Re: [PATCH 1/1] qxl: add subsystem_vendor_id property

[Gerd Hoffmann]

  Hi,

> Anyway, we need to support different PCI sub-vendor IDs
> in order to be compliant with Microsoft WHQL rules. Though,
> actually, at my opinion this requirement has nothing in
> common with libvirt people. The most convenient way
> here would be to specify these properties within vendor
> machine types and this place is a perfect match as any
> respectable has its own machine type.

Yes, when wiring this up via vendor machine types there
is no need to touch libvirt.

> I would also think that PCI level is not a good place for that
> as we would not be able to apply this change blindly as at
> PCI level this change would be too global and the same
> was initially noted by Michael Tsirkin here
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg04384.html
> 
> Any thoughts?
> What should we do with the original patch from Ben? We
> still need an ability to expose vendor identity in QXL/virtio...

I'd suggest to add properties to PCIDevice then.  That will allow to
override the default subsystem id for both specific pci devices and
all pci devices.

take care,
  Gerd