Re: [PATCH v4 1/2] tools/hvmloader: implement Intel IGD extended VBT support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2026 05:27, Chuck Zmudzinski wrote:
> Modern Intel IGD devices do not work well with the current implementation of
> support for the Intel IGD in hvmloader because it lacks support for an
> extended video bios table (VBT).
> 
> Code 43 errors in Windows guests and failure of the guest screen to light up
> are some of the problems that occur with the current implementation.
> 
> To address this problem, this patch implements support for Intel IGD devices
> with an extended VBT and OpRegion version 2+ which is required for most modern
> Intel IGD devices, as noted in the Linux kernel vfio commits referenced in the
> Link tags below. This patch ports support for devices with an extended VBT and
> OpRegion 2+ which was added in those commits for KVM/vfio, but adapted for Xen
> HVM guests with PCI passthrough.
> 
> This patch depends on compatible support in the device model. If hvmloader
> detects the device model lacks such support, it will fall back to the
> currently implemented protocol for configuring the OpRegion to provide
> backward compatibiltiy for systems that lack a device model with support for
> an extended VBT.
> 
> The primary reason the OpRegion needs to be patched in some cases is that with
> the addition of the RVDA and RVDS fields to the OpRegion, the OpRegion is not
> position-independent and may need to be patched if it is moved to a different
> address in the guest. This means the current protocol of having the device
> model directly map the unmodified host OpRegion to the guest is not compatible
> with the requiremnts of the newer devices that in some cases require that the
> OpRegion be modified for it to be compatible with the guest address space.
> 
> In this implementation, the device model has the responsibility to read the
> host OpRegion and patch it as needed before exposing it to the guest. Since an
> extended VBT means more pages are needed for the OpRegion, depending on the
> size of the extended VBT, hvmloader has the responsibility to edit the E820
> map to accomodate the additional pages needed to contain the OpRegion + VBT.
> 
> To implement this in hvmloader, use a variable, igd_opregion_e820_pages,
> instead of the constant, IGD_OPREGION_PAGES, to represent the number of pages
> to reserve in the E820 map for the OpRegion + VBT. Also, to remove the
> confusion introduced by setting IGD_OPREGION_PAGES to 3 in an earlier patch
> to account for the fact that the OpRegion is not guaranteed to be aligned on
> a page boundary, reset IGD_OPREGION_PAGES to 2 so it matches the actual size
> of the OpRegion.
> 
> Instead of only writing to the PCI_INTEL_OPREGION register, first read from it
> to provide a way for both hvmloader and the device model to discover if both
> components have support for an extended VBT and more than 3 pages reserved for
> the OpRegion + VBT. The device model detects the read of the register before
> the write to learn that hvmloader has support, and hvmoader detects that the
> device model returns the number of pages to reserve for the OpRegion + VBT
> instead of 0 when it first reads the register to learn that that the device
> model has support. When this new protocol is supported by both hvmloader and
> the device model, the device model will not expose the host OpRegion directly
> to the guest via a direct mapping as the old protocol does but instead exposes
> an emulated copy of the OpRegion and VBT using its ioreq server. This has the
> added benefit of preventing extraneous host memory in the regions before or
> after a non-page-aligned OpRegion that should be confidential to the host from
> being exposed to the guest.
> 
> Testing reveals that when the device model exposes the OpRegion to the guest
> by mapping it to the device model's ioreq server, the Windows Intel IGD
> graphics dirvers are unable to access the OpRegion and report Code 43 errors
> with the result being that the guest screen never lights up. So after the
> device model exposes the OpRegion to the guest using its ioreq server, make a
> copy of it and use a copy of the OpRegion and VBT which is backed by RAM
> allocated to the guest instead of mapped via the ioreq server. This fixes the
> Code 43 errors reported by the Windows IGD graphics drivers.
> 
> Link: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/vfio/pci/vfio_pci_igd.c?id=bab2c1990b78
> Link: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/vfio/pci/vfio_pci_igd.c?id=49ba1a2976c8
> Signed-off-by: Chuck Zmudzinski <brchuckz@xxxxxxx>

Despite the non-negligible number of comments below, I think this is in a
much better shape now.

> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -8,7 +8,8 @@ enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
>  extern enum virtual_vga virtual_vga;
>  
>  extern unsigned long igd_opregion_pgbase;
> -#define IGD_OPREGION_PAGES 3
> +extern unsigned int igd_opregion_e820_pages;
> +#define IGD_OPREGION_PAGES 2

I wonder if this is a good way of expressing things. Specifying a page
count here kind of implies page alignment. Imo specifying a size in bytes
here would be better, as that then makes more natural that an extra page
needs adding in the legacy case (to account for the area not starting at
a page boundary). See also a comment further down, likely leading to
elimination of most of the users of this constant.

> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -44,6 +44,7 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>  
>  enum virtual_vga virtual_vga = VGA_none;
>  unsigned long igd_opregion_pgbase = 0;
> +unsigned int igd_opregion_e820_pages = 0;
>  
>  /* Check if the specified range conflicts with any reserved device memory. */
>  static bool check_overlap_all(uint64_t start, uint64_t size)
> @@ -93,6 +94,9 @@ void pci_setup(void)
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
>      uint8_t pci_devfn_decode_type[256] = {};
> +    uint32_t igd_opregion;
> +    void *opregion_vbt_scratch;
> +    bool opregion_is_direct_mapped;

Please declare these in the narrowest possible scope, i.e. ...

> @@ -192,12 +196,98 @@ void pci_setup(void)
>                  {

... apparently here.

>                      igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);

Why do you leave this untouched? By splitting the mem_hole_alloc() calls, you
rely on implementation details of that function without there actually being
a need. 

>                      /*
> -                     * 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.
> +                     * To be compatible with this interface for programming 
> the
> +                     * the PCI_INTEL_OPREGION register, the device model must
> +                     * check if the guest reads the register before it writes
> +                     * to the register. To indicate to the device model that
> +                     * we have support for an extended VBT, we read the
> +                     * PCI_INTEL_OPREGION register before writing to it. If 
> the
> +                     * device model supports an extended VBT, it will return
> +                     * the number of pages needed for the OpRegion + VBT. If
> +                     * not, it will return 0 which indicates that it does not
> +                     * implement this interface for supporting an extended 
> VBT.
> +                     */

This approach will need buyoff by qemu folks before it can be acked here. 
(Buyoff
doesn't necessarily mean the change must have gone in there first, yet of course
it would be best if things went in that order.)

> +                    igd_opregion_e820_pages = pci_readl(vga_devfn,
> +                                                        PCI_INTEL_OPREGION);
> +                    if ( !igd_opregion_e820_pages )
> +                    {
> +                        /*
> +                         * This case provides backward compatibility with
> +                         * device model versions that lack support for an
> +                         * extended VBT. In this case the device model
> +                         * expects us to allocate an extra page in case the
> +                         * OpRegion is not aligned on a page boundary. Also,
> +                         * in this case, the host OpRegion is direct mapped
> +                         * into the guest.
> +                         */
> +                        igd_opregion_pgbase = mem_hole_alloc(1);
> +                        igd_opregion_e820_pages = IGD_OPREGION_PAGES + 1;
> +                        opregion_is_direct_mapped = true;
> +                    }
> +                    else
> +                    {
> +                        /* Allocate extra pages for an extended VBT */
> +                        if ( igd_opregion_e820_pages > IGD_OPREGION_PAGES )
> +                        {
> +                            igd_opregion_pgbase =
> +                                mem_hole_alloc(igd_opregion_e820_pages -
> +                                               IGD_OPREGION_PAGES);
> +                        }
> +                        opregion_is_direct_mapped = false;
> +                    }
> +                    /*
> +                     * This ensures the OpRegion + VBT does not take up more
> +                     * than 1/32 of the reserved region. Also, we must reject
> +                     * a value of 1 for igd_opregion_e820_pages.
> +                     */
> +                    if ( igd_opregion_e820_pages >
> +                        RESERVED_MEMORY_DYNAMIC_PAGES >> 5 ||

Parentheses please around the shift expression.

The 1/32th also looks to be entirely arbitrary. If so, the comment also wants
to state this fact.

> +                        igd_opregion_e820_pages == 1 )
> +                    {
> +                        printf("too many or too few pages (%u) for 
> OpRegion\n",
> +                               igd_opregion_e820_pages);
> +                        BUG();
> +                    }

Why does this entire if() not live in the "else" branch of the earlier if()?
It's applicable only there afaict.

> +                    /*
> +                     * Write the the OpRegion offset to give the OpRegion

Nit: As you move the comment, please also drop the stray extra "the".

> +                     * address to the device model. The device model will 
> trap
> +                     * and make the OpRegion accessible at the given address.
> +                     * The device model is also expected to verify that the
> +                     * OpRegion is compatible with the guest address space 
> and
> +                     * patch it if necessary to make it compatible.
>                       */

The last sentence of the comment is new, and I fear I don't understand what
it is trying to describe. How can one "patch a region" to "make it compatible"?

>                      pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>                                 igd_opregion_pgbase << PAGE_SHIFT);
> +
> +                    /* Don't use our own copy if OpRegion is direct mapped */
> +                    if ( opregion_is_direct_mapped )
> +                        break;
> +
> +                    /*
> +                     * Windows IGD drivers do not work properly when the
> +                     * OpRegion is exposed by the device model's ioreq 
> server,
> +                     * so make a copy of the OpRegion and use that copy which
> +                     * will be backed by RAM allocated to the guest.
> +                     */

I'm struggling with this comment: What does "do not work properly" mean here?
How the region is backed should be of no interest to the driver?

> +                    opregion_vbt_scratch =
> +                        scratch_alloc(igd_opregion_e820_pages <<
> +                                      PAGE_SHIFT, 0);
> +                    memcpy(opregion_vbt_scratch,
> +                           (void *)(igd_opregion_pgbase << PAGE_SHIFT),
> +                           igd_opregion_e820_pages << PAGE_SHIFT);
> +
> +                    igd_opregion = pci_readl(vga_devfn, PCI_INTEL_OPREGION);

Aren't you reading back here what you wrote above? Why the read then?

> +                    /*
> +                     * The device model will unmap the OpRegion from the 
> ioreq
> +                     * server so we can use our own copy of the OpRegion.
> +                     */
> +                    pci_writel(vga_devfn, PCI_INTEL_OPREGION, igd_opregion);

Despite the comment this looks like black magic: You write back the value
you read. That's hardly expected behavior, and hardly how real hardware
would behave. I think the comment will need extending, and I think this is
another behavioral aspect which needs buyoff by qemu folks.

One further request: As you're already tidying things, would you mind also
correcting the comment on PCI_INTEL_OPREGION's definition (which I think
means "bytes" when it says "bits")?

Jan