Re: [XEN PATCH] tools/libs/light/libxl_pci.c: explicitly grant access to Intel IGD opregion

[Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx>]

On 3/15/22 7:38 AM, Jan Beulich wrote:
> On 14.03.2022 04:41, Chuck Zmudzinski wrote:
>
> > @@ -610,6 +612,45 @@ out:
> >       return ret;
> >   }
> >   
> > +static uint32_t sysfs_dev_get_igd_opregion(libxl__gc *gc,
> > +                                           libxl_device_pci *pci)
> > +{
> > +    char *pci_device_config_path =
> > +            GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/config",
> > +                      pci->domain, pci->bus, pci->dev, pci->func);
> > +    size_t read_items;
> > +    uint32_t igd_opregion;
> > +    uint32_t error = 0xffffffff;
> I think this constant wants to gain a #define, to be able to correlate
> the use sites. I'm also not sure of the value - in principle the
> register can hold this value, but of course then it won't be 3 pages.
I have one more comment to add here. I am not intending
to define igd_opregion as a data structure 3 pages (12k)
long, much less as a pointer to such a structure. However,
it would be nice to have access to the actual data structure
in libxl, because we could use it to validate its contents.
I looked in the code for the i915 Linux kernel module, and
the IGD opregion does have a signature that we could check
if we have access to it. That would mitigate my concerns
expressed in my first version of the patch about a false
positive when identifying an Intel IGD. Hvmloader should
probably also do this check before it maps the Intel IGD
into guest memory if that is possible. However, I expect
that it is not a memory that libxl has access to. It is
probably a structure in kernel space, but it might be
possible for libxl to ask the hypervisor for access to it.
Perhaps the libxl maintainers can shed some light on that
possibility. If this is possible, I will include such a check for
the validity of the contents in the IGD in version 2 of the
patch.

Regards,

Chuck