| Message ID | 20220928155244.1837455-1-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] qxl: add subsystem_vendor_id property | expand |
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
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
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
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"
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(+)