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

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

[Marek Marczykowski-Górecki]

Consider also "Display controller" an IGD, not only "VGA compatible
controller" in few more places.

Fixes: 4191619e0893 ("libxl: extend IGD check")
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Do you prefer this to be split into two patches (libxl, hvmloader)?

 tools/firmware/hvmloader/pci.c | 1 +
 tools/libs/light/libxl_pci.c   | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index c3c61ca060a6..1ee97a5b4b20 100644
--- 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) )
             {
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 1647fd6f4756..db1299832cce 100644
--- 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;
 
         stubdom_domid = libxl_get_stubdom_id(CTX, domid);
-- 
2.48.1

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

[Andrew Cooper]

On 08/04/2025 2:23 pm, Marek Marczykowski-Górecki wrote:
> Consider also "Display controller" an IGD, not only "VGA compatible
> controller" in few more places.
>
> Fixes: 4191619e0893 ("libxl: extend IGD check")
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Do you prefer this to be split into two patches (libxl, hvmloader)?

Probably not for something this trivial.

>
>  tools/firmware/hvmloader/pci.c | 1 +
>  tools/libs/light/libxl_pci.c   | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index c3c61ca060a6..1ee97a5b4b20 100644
> --- 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:

Now we've got multiple, we should have /* VGA */ and /* Display */
comments like libxl has.

Can fix on commit.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Marek Marczykowski-Górecki]

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

On Tue, Apr 08, 2025 at 02:31:10PM +0100, Andrew Cooper wrote:
> On 08/04/2025 2:23 pm, Marek Marczykowski-Górecki wrote:
> > Consider also "Display controller" an IGD, not only "VGA compatible
> > controller" in few more places.
> >
> > Fixes: 4191619e0893 ("libxl: extend IGD check")
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Do you prefer this to be split into two patches (libxl, hvmloader)?
> 
> Probably not for something this trivial.
> 
> >
> >  tools/firmware/hvmloader/pci.c | 1 +
> >  tools/libs/light/libxl_pci.c   | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index c3c61ca060a6..1ee97a5b4b20 100644
> > --- 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:
> 
> Now we've got multiple, we should have /* VGA */ and /* Display */
> comments like libxl has.
> 
> Can fix on commit.

Fine with me, thanks!

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

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

[Jan Beulich]

On 08.04.2025 15:23, Marek Marczykowski-Górecki wrote:
> --- 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 */

Nit: Indentation looks suspicious here (can likely also be adjusted
while committing).

Jan

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

[Jan Beulich]

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?

Jan

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

[Marek Marczykowski-Górecki]

[-- 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 --]

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

[Anthony PERARD]

On Tue, Apr 08, 2025 at 03:23:13PM +0200, Marek Marczykowski-Górecki wrote:
> Consider also "Display controller" an IGD, not only "VGA compatible
> controller" in few more places.
> 
> Fixes: 4191619e0893 ("libxl: extend IGD check")
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Do you prefer this to be split into two patches (libxl, hvmloader)?
> 
>  tools/firmware/hvmloader/pci.c | 1 +
>  tools/libs/light/libxl_pci.c   | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index c3c61ca060a6..1ee97a5b4b20 100644
> --- 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) )
>              {
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 1647fd6f4756..db1299832cce 100644
> --- 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 */

According to some not too random document on internet [1][2], the whole
0x03 would be the display class, with 0x0380 been other display
controller. So it might be better to change the new comment to "Other
display controller".

[1] https://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_12__v9_Jan_2020.pdf
[2] https://wiki.osdev.org/PCI#Class_Codes

Otherwise, change looks fine to me:
Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD