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

[Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx>]

On 3/30/22 1:15 PM, Anthony PERARD wrote:
> Hi Chuck,
>
> On Sun, Mar 13, 2022 at 11:41:37PM -0400, Chuck Zmudzinski wrote:
> > When gfx_passthru is enabled for the Intel IGD, hvmloader maps the IGD
> > opregion to the guest but libxl does not grant the guest permission to
> I'm not reading the same thing when looking at code in hvmloader. It
> seems that hvmloader allocate some memory for the IGD opregion rather
> than mapping it.
>
>
> tools/firmware/hvmloader/pci.c:184
>      if ( vendor_id == 0x8086 )
>      {
>          igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);
>          /*
>           * Write the the OpRegion offset to give the opregion
>           * address to the device model. The device model will trap
>           * and map the OpRegion at the give address.
>           */
>          pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>                     igd_opregion_pgbase << PAGE_SHIFT);
>      }
>
> I think this write would go through QEMU, does it do something with it?
> (I kind of replying to this question at the end of the mail.)
>
> Is this code in hvmloader actually run in your case?
Hi Anthony,

Let me try to answer your question again. My tests indicate
that this code in hvmloader is actually run in my case. As I
mentioned in an earlier message, the allocation of the three
pages for the opregion is not done for the guest if I remove
this code from hvmloader. The only concern I had was about
the difference in what I was reading for the opregion address
in sysfs (fcffc018 in my case) and the address that hvmloader
wrote (fcffc000 in my case). The change is easily explained by
what the Qemu device model (both upstream and traditional)
does when the device model writes the opregion address into
the Intel IGD config attribute:

This is the traditional Qemu code in hw/pt_graphics.c:66

void igd_write_opregion(struct pt_dev *real_dev, uint32_t val)
{
    uint32_t host_opregion = 0;
    int ret;

    if ( igd_guest_opregion )
    {
        PT_LOG("opregion register already been set, ignoring %x\n", val);
        return;
    }

    host_opregion = pt_pci_host_read(real_dev->pci_dev, 
PCI_INTEL_OPREGION, 4);
    igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);

------------------------ End of code snippet 
-----------------------------------
The offset of the opregion in the guest (0x18 in my case) is
recovered by that last statement. The upstream model does the
same thing using the constant XEN_PCI_INTEL_OPREGION_MASK
set to 0xfff to recover the offset.

So what we have in hvmloader is correct and necessary.

> > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> > index 4bbbfe9f16..a4fc473de9 100644
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -2531,6 +2572,37 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, 
> > const uint32_t domid,
> >                     domid, vga_iomem_start, (vga_iomem_start + 0x20 - 1));
> >               return ret;
> >           }
> > +
> > +        /* If this is an Intel IGD, allow access to the IGD opregion */
> > +        if (!libxl__is_igd_vga_passthru(gc, d_config)) return 0;
> > +
> > +        uint32_t igd_opregion = sysfs_dev_get_igd_opregion(gc, pci);
> > +        uint32_t error = 0xffffffff;
> > +        if (igd_opregion == error) break;
> > +
> > +        vga_iomem_start = ( (uint64_t) igd_opregion ) >> XC_PAGE_SHIFT;
> > +        ret = xc_domain_iomem_permission(CTX->xch, stubdom_domid,
> > +                                         vga_iomem_start,
> > +                                         IGD_OPREGION_PAGES, 1);
> > +        if (ret < 0) {
> > +            LOGED(ERROR, domid,
> > +                  "failed to give stubdom%d access to iomem range "
> > +                  "%"PRIx64"-%"PRIx64" for IGD passthru",
> > +                  stubdom_domid, vga_iomem_start, (vga_iomem_start +
> > +                                                IGD_OPREGION_PAGES - 1));
> > +            return ret;
> > +        }
> > +        ret = xc_domain_iomem_permission(CTX->xch, domid,
> > +                                         vga_iomem_start,
> > +                                         IGD_OPREGION_PAGES, 1);
> Here, you seems to add permission to an address that is read from the
> pci config space of the device, but as I've pointed above hvmloader
> seems to overwrite this address.
No, hvmloader wrote the mapped address and here we are
reading the opregion address of the host, not the mapped
address of the guest. There is no problem here.
> It this call to
> xc_domain_iomem_permission() does actually anything useful?
> Or is it by luck that the address returned by
> sysfs_dev_get_igd_opregion() happened to be the address that hvmloader
> is going to write?
No, it is not luck, we use the same constant in hvmloader,
Qemu, and here in this patch to properly map the opregion
to the guest, and the constant is PCI_INTEL_OPREGION, set
to 0xfc, the offset of where in the config attribute the
opregion address is stored.
> Or maybe hvmloader doesn't actually do anything?
It does do something, and what it does is necessary.

Regards,

Chuck