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

[Anthony PERARD <anthony.perard@citrix.com>]

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?

> Currently, because of another bug in Qemu upstream, this crash can
> only be reproduced using the traditional Qemu device model, and of

qemu-traditional is a bit old... What is the bug in QEMU? Do you have a
link to a patch/mail?

> 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. 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?

Or maybe hvmloader doesn't actually do anything?


Some more though on that, looking at QEMU, it seems that there's already
a call to xc_domain_iomem_permission(), in igd_write_opregion(). So
adding one in libxl would seems redundant, or maybe it the one for the
device model's domain that's needed  (named 'stubdom_domid' here)?

Thanks,

-- 
Anthony PERARD