Re: [PATCH] libxl+hvmloader: extend IGD check part 2

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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

On Tue, Apr 08, 2025 at 04:11:36PM +0200, Jan Beulich wrote:
> On 08.04.2025 15:23, Marek Marczykowski-Górecki wrote:
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -173,6 +173,7 @@ void pci_setup(void)
> >          switch ( class )
> >          {
> >          case 0x0300:
> > +        case 0x0380:
> >              /* If emulated VGA is found, preserve it as primary VGA. */
> >              if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
> >              {
> 
> Unlike here, where vendor IDs are subsequently checked (and the sole question
> that arises is whether any of the combinations can actually come as Display
> rather than VGA), ...
> 
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -2575,7 +2575,8 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
> >  
> >          if (sysfs_dev_get_class(gc, pci, &pci_device_class))
> >              continue;
> > -        if (pci_device_class != 0x030000) /* VGA class */
> > +        if (pci_device_class != 0x030000 && /* VGA class */
> > +                pci_device_class != 0x038000) /* Display class */
> >              continue;
> 
> ... there's no such checking here, and instead very much VGA-specific things
> are being done then. Is that really in line with permitting Display class
> devices here as well?

Well, it was necessary to get IGD passthrough working. But I think
upstream Xen still doesn't have all the pieces here. Specifically,
Qubes's version includes (a variant of)
https://lore.kernel.org/xen-devel/87d74a21bde95cfc7c53fad56bf8f0e47724953e.1592171394.git.gorbak25@gmail.com/T/#m8644141760ee36d691434dfaf55031110492929b
and that adds here access not only to the video memory, but also vbios,
which was needed to get it working.

Anyway, this code is reachable only if gfx_passthrou is enabled and
currently this option is specific to IGD.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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