Re: [PATCH v2] tools/hvmloader: implement Intel IGD extended VBT support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2026 02:45, Chuck Zmudzinski wrote:
> On 8/13/2026 6:35 AM, Jan Beulich wrote:
>> On 02.08.2026 07:08, Chuck Zmudzinski wrote:
>>> -- snip --
>>> To address this problem, this patch implements support for
>>> Intel IGD devices with an extended VBT and OpRegion version 2
>>> and higher which is required for most modern Intel IGD devices.
>>
>> First of all: Where's the spec of all of this?
> 
> Well, your first question is quite provocative. Certainly more
> social/legal than technical.

Well, it was very much meant to be technical. I've had a hard time following
what your new code does, and having a spec to hand would likely have helped.

> I presume by "all this" you mean code in this patch such as:
> 
> #define IGD_OPREGION_RVDA 0x3ba
> #define IGD_OPREGION_RVDS 0x3c2
> #define IGD_OPREGION_VERSION 0x16
> 
> which defines the offsets of the rvda, rvds, and version fields from
> the base address of the Intel OpRegion.
> 
> Also, I presume that "all this" includes the meaning of the 8-byte
> rvda value, the meaning of the 4-byte rvds value, and the meaning of
> the 2-byte version value.

"All this" certainly goes beyond this, i.e. also covering the intended
interactions.

> So my answer is as follows:
> 
> I do not have access to the official spec that defines "all this" but
> I do have access, as does the general public, to the Linux kernel's
> implementation of support for the Intel IGD from many sources such as
> git.kernel.org. The Linux kernel has enough accurate information about
> the spec of "all this" to provide very good support for the Intel IGD
> on bare metal.
> 
> To elaborate a bit more, the spec of "all this" can be derived from the
> Linux kernel code that supports the Intel IGD.

So you expect every reader to locate and decipher the underlying information
from a (afaik) pretty large piece of code in the Linux kernel? If the Linux
kernel sources are the reference, please can you at least provide pointers
into there?

>>> --- a/tools/firmware/hvmloader/config.h
>>> +++ b/tools/firmware/hvmloader/config.h
>>> -- snip -- 
>>>  #define PAGE_SHIFT 12
>>>  #define PAGE_SIZE  (1ul << PAGE_SHIFT)
>>> +#define tools/hvmloader/pci.c3
>>> +#define IGD_OPREGION_SIZE ((IGD_OPREGION_PAGES - 1) << PAGE_SHIFT)
>>
>> This is odd, and hence wants a comment.
> 
> Yes, I could add a comment, probably a long one, to explain this
> oddity. It is a problem of backward compatibility where we have a
> definition, IGD_OPREGION_PAGES, that is currently set to 3 both here
> in hvmloader and in the Qemu DM, but should be 2 because the OpRegion
> size is really exactly two pages but the current implementation set it
> to 3 because the host OpRegion is not always aligned on a 4k page
> boundary so three pages are needed to map the entire host OpRegion to
> the guest. I could re-write the patch setting IGD_OPREGION_PAGES to 2
> and avoid a comment here, but that would complicate the logic of how
> igd_opregion_e820_pages is calculated and probably introduce the need
> for comments in other places. 
> 
> I am open to suggestions about how best to handle the backward compatibility
> problem and the problem of ensuring compatibility between hvmloader support
> for Intel IGD passthrough and DM support for that same feature. For now,
> however, I am trying to keep what is applicable to the current implementation,
> and this odd value of 3 for IGD_OPREGION_PAGES is one of those things
> I am keeping for backward compatibility.

Personally I don't view breaking backward compatibility as an option. Hence
a comment is going to be needed, and preferably not an overly long one.

>>> +#define IGD_OPREGION_RVDA 0x3ba
>>> +#define IGD_OPREGION_RVDS 0x3c2
>>> +#define IGD_OPREGION_VERSION 0x16
>>> +#define IGD_OPREGION_MASK 0xfff
>>> +#define IGD_OPREGION2_SUPPORT_MASK 0x1
>>> +#define IGD_OPREGION_SIGNATURE "IntelGraphicsMem"
>>> +#define IGD_VBT_SIGNATURE "$VBT"
>>> +extern unsigned long igd_opregion_pgbase;
>>> +extern uint32_t igd_opregion_e820_pages;
>>> +void intel_opregion_setup(uint32_t vga_devfn);
>>
>> Blank lines please ahead of the new #define-s you add and between those new
>> #define-s and the new decls.
>>
>> For igd_opregion_e820_pages I further cannot spot any use which would justify
>> the use of a fixed-width type; unsigned int will do, and will then be in line
>> with ./CODING_STYLE.
> 
> Ok I will pay more attention to CODING_STYLE. I know that libxl
> has a specific CODING_STYLE document. Is there a specific one
> for hvmloader? I do not see one in the tools/firmware/hvmloader
> directory. I assume the one that matters for hvmloader is the
> one at the top level of the Xen code source tree, not the libxl one.

Yes, hvmloader follows (better: ought to follow) hypervisor style.


>>> --- /dev/null
>>> +++ b/tools/firmware/hvmloader/intel_opregion.c
>>> @@ -0,0 +1,297 @@
>>> +/*
>>> + * intel_opregion.c: HVM Intel OpRegion setup.
>>> + *
>>> + * Leendert van Doorn, leendert@xxxxxxxxxxxxxx
>>> + * Copyright (c) 2005, International Business Machines Corporation.
>>> + *
>>> + * Copyright (c) 2006, Keir Fraser, XenSource Inc.
>>
>> What do these cover?
> 
> I am considering this new file to be a modified/derived version of
> tools/hvmloader/pci.c, so if I understand correctly this file needs
> to retain the copyright information of tools/hvmloader/pci.c. At the
> very least, the #include statements at the top of this new file which
> are from tools/hvmloader/pci.c are covered by these copyrights. I also
> consider the statements that are moved from tools/hvmloader/pci.c to
> this new file to be covered by these copyrights. IANAL, so to be safe,
> I include these copyrights even though the covered code is relatively
> small compared to the rest of the file.

Nowadays our preferred option is to omit such copyright statements
altogether, but we wouldn't insist on the omission. I further don't think
#include-s are copyrightable. 

>>> +    static unsigned long rvda_host;
>>> +    static unsigned long rvda_guest;
>>
>> Why static? The function can't be called more than once, if I'm not mistaken.
> 
> I think you are right that we only do the setup once so I will
> drop static here. I still think if I drop static I will want to
> initialize these to zero later, because (correct me if I am wrong)
> only static variables are initialized to zero if not explicitly
> initialized, and without either static or an initialized value,
> these would be initialized to some undetermined random value
> until explicitly set to the desired initial value. Of the two,
> I think that the more important one to intitialize to zero is
> rvda_host, because I use an initial value of zero for that variable
> to test for the case when we do not need extended VBT support.

Well, like all variables, these ones also will need to be sensibly
initialized. That's entirely unrelated to the use of static; all
static gets you in this regard is that there's implicit default
initialization. Yet that alone is no reason to use static.

>>> +    igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);
>>> +    /*
>>> +     * Tentative value for the number of pages to reserve
>>> +     * in the E820 map for the OpRegion and VBT.
>>> +     *
>>> +     * This will be the final value for the E820 map if
>>> +     * the device model lacks support for OpRegion 2 or
>>> +     * if the host OpRegion version is < 2 or if we never
>>> +     * allocate more pages in the E820 map for the VBT.
>>> +     */
>>> +    igd_opregion_e820_pages = IGD_OPREGION_PAGES;
>>> +
>>> +    /*
>>> +     * Read the value the device model is initialized with.
>>> +     * If the device model supports OpRegion 2, it will
>>> +     * return the host IGD OpRegion address. If not, it
>>> +     * will return 0. If the device model does not support
>>> +     * OpRegion 2, the device model expects us to give it
>>> +     * the address to which it will map the OpRegion in the
>>> +     * guest and then expects us to do nothing more to setup
>>> +     * the OpRegion, so that is all we will do in that case.
>>> +     */
>>
>> Hmm, exposing the host opregion to a guest certainly feels like an issue.
> 
> Well, that is how it is now. I am only retaining it to maintain backward
> compatiblily with DM versions that do not support the extended VBT and
> OpRegion 2+. My previous comment about backward compatibilty and DM
> compatibility also applies here. If we don't worry about that, we can do
> away with any cases where we are permanently mapping the host opregion to
> the guest and implement this new approach of always exposing a copy of
> the OpRegion and VBT to the guest instead.

How does "permanently mapping" matter? hvmloader runs inside the guest, so
exposure just to copy the data isn't any better in terms of this being a
layering violation. The more correct thing to do might be for the DM to
put in place a copy before the guest (i.e. hvmloader) even gains control.
(How in turn the DM would learn of the contents of the opregion is a
separate question then.)

>>> +    const uint32_t igd_host_opregion = pci_readl(vga_devfn,
>>> +                                                 PCI_INTEL_OPREGION);
>>> +    if ( !igd_host_opregion ) {
>>
>> Nit (style) Brace placement (throughout).
> 
> Ok. I see this is not the proper coding style.
> 
>>
>>> +        printf("device model lacks extended VBT "
>>> +               "support. Continuing with legacy support only\n");
>>
>> This message can easily confuse / worry people. (If it was to be kept, it
>> would also need style adjustment.)
> 
> I think some message is needed here to indicate the incompatibility of
> versions of the DM that do not support the extended VBT with versions
> of hvmloader that do, especially if we are not going to worry as much
> about the backward compatibility / DM compatibility problem I mentioned
> multiple times in previous comments above.
> 
> This message could encourage upgrading the DM to a version that supports
> the extended VBT instead of just giving this scary notification.

But someone expecting legacy behavior could be misguided by the message
(e.g. into wondering whether there's something wrong.)

>>> +    const uint32_t igd_host_opregion_page_offset =
>>> +                   igd_host_opregion & IGD_OPREGION_MASK;
>>
>> I think like in the hypervisor we don't want to mix declarations and
>> statements just yet.
> 
> The only way I could separate the declaration from the statement would be
> to drop the const modifier because if I do:
> 
>     const uint32_t igd_host_opregion_page_offset;
> ...
>     igd_host_opregion_page_offset = igd_host_opregion &
>                                     IGD_OPREGION_MASK;
> 
> The compiler will report an error. If I drop the const modifier from
> the declaration, the compiler will not report an error but I lose the
> protection the compiler gives me from making mistakes by modifying a
> variable's value that should be constant.
> 
> I am not a C guru but some research indicates that while it is legal in
> C to declare a variable with the const modifier without also assigning
> it a value at the same time with a statement, it is not recommended to
> do this because the variable will be initialized with some undefined
> random value that cannot be changed because we used the const modifier
> in the declaration. This implies strict enforcemnt of the rule "we
> don't mix declarations and statements" results also in the corollary
> rule "we never use the const modifier for variables in C."

Indeed we rarely use const on variables (or parameters) themselves. It's
primary use is on pointed-to types.

>>> +    pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>>> +               (igd_opregion_pgbase << PAGE_SHIFT) |
>>> +                IGD_OPREGION2_SUPPORT_MASK);
>>
>> This looks to imply qemu is the only possible device model.
> 
> Yeah, this is an issue. Other device models that intend to support
> the Intel IGD with hvmloader will also have to be compatible with this.
> It would be easier if we did not have to worry about backward
> compatibility and supporting what we had in the codebase for many years
> in both hvmloader and Qemu and we would not need IGD_OPREGION2_SUPPORT_MASK
> in that case. Instead, we would just completely deprecate all previous
> implementations of the Intel IGD passthrough feature in both hvmloader and
> the Qemu DM as unsupported. So my previous comments about backward
> compatibility apply here again.

As said, I don't think backward compatibility can be dropped. My comment
also didn't really mean to hint in that direction. Instead I was wondering
in how far, even if perhaps by only a few #define-s, the necessary
interfacing couldn't be put down in a public header, for any DM to consume.

>>> +    printf("guest OpRegion tentative "
>>> +           "address: 0x%x\n", igd_guest_opregion);
>>> +
>>> +    if ( !verify_opregion(igd_guest_opregion) ) {
>>> +        printf("error: IGD OpRegion signature "
>>> +               "not found.\n");
>>
>> No full stop in messages please.
> 
> Would it be OK to just get rid of the error message here?

That would then leave ...

>>> +        BUG();

... an un-annotated BUG(), which generally isn't very nice.

>>> +    }
>>> +   --snip --
>>> +        rvda_host = 0;
>>> +    }
>>> +    const uint32_t rvda_host_page_offset = rvda_host &
>>> +                                           IGD_OPREGION_MASK;
>>
>> Why host_page_offset here when ...
>>
>>> +    const uint32_t rvds = *(uint32_t *)(opregion_scratch +
>>> +                                        IGD_OPREGION_RVDS);
>>> +    const uint32_t rvds_page_offset = rvds & IGD_OPREGION_MASK;
>>
>> ... it's just page_offset here, and when further you use it below to set
>> rvda_guest?
> 
> The size of the VBT, rvds, is the same on both host and guest, so we do not
> need to specify host or guest, but the base address of the VBT, rvda, is
> not the same on the guest as it is on the host, so we need to specify which
> one for rvda. I can change this to rvds_host_page_offset because it is
> not wrong, but it might be confusing because I use that value later on
> for computations involving the guest also.

Why not simply drop the "host" infix, when it's not relevant?

>>> +    printf("VBT size: 0x%x\n", rvds);
>>> +
>>> +    if ( !rvds || !rvda_host ) {
>>> +        printf("guest OpRegion address: 0x%x\n", igd_guest_opregion);
>>> +        rvda_host = 0;
>>> +    }
>>> +    /*
>>> +     * Write rvda_host as 2 successive 32-bit values
>>> +     * to communicate location of the VBT to the device
>>> +     * model. If rvda_host is not 0, The device model
>>> +     * unmaps the OpRegion and eventually maps the VBT
>>> +     * after we also write the guest address where the
>>> +     * VBT will be mapped.
>>> +     *
>>> +     * If we send rvda_host = 0 to the device model, it
>>> +     * will assume we do not need OpRegion 2 support and
>>> +     * it will not unmap the OpRegion.
>>> +     */
>>> +    pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>>> +               (uint32_t)(rvda_host & 0xfffffffful));
>>> +    unsigned long rvda_host_upper_32 = (uint64_t)rvda_host >> 32;
>>> +    pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>>> +               (uint32_t)rvda_host_upper_32);
>>
>> Why would you need to communicate a host property to the DM?
> 
> The DM cannot access the host rvda value because it is only accessible
> from the host kernel, and the DM is only a user-space process on the host.

I don't follow this: Anything the guest can access should also be accessible
by its DM.

>>> +    /*
>>> +     * Update the number of pages the device model
>>> +     * needs to map for us to get a copy of the VBT.
>>> +     *
>>> +     * N.B.: Here, igd_opregion_pgbase is really the page
>>> +     * base of the location where the device model will
>>> +     * map the VBT.
>>> +     */
>>> +    uint32_t vbt_pages_needed = rvds >> PAGE_SHIFT;
>>> +    if ( rvds & IGD_OPREGION_MASK )
>>> +        vbt_pages_needed++;
>>> +    if ( vbt_pages_needed > igd_opregion_e820_pages ) {
>>> +        igd_opregion_pgbase = mem_hole_alloc
>>> +                              (vbt_pages_needed - igd_opregion_e820_pages);
>>
>> Nit: Indentation.
> 
> Ok. It should always be a multiple of four spaces, I presume. I admit I did
> not check that.

Not quite. Within a wrapped expression, you need to determine what I like
to call the "anchor point". In a function call that's the start of the
function name. The wrapped part of the expression would then start one
extra level (4 spaces) deeper than the anchor point. Things are different
when there are pending open parentheses: There the wrapped part of an
expression starts with as many extra spaces as there are pending open
parentheses, with the outermost pending open parenthesis being the anchor
point. E.g. (taking the example above and adding extra wrapping in the
function argument expression just for demonstration purposes):

        igd_opregion_pgbase = mem_hole_alloc
                                  (vbt_pages_needed -
                                   igd_opregion_e820_pages);

Or alternatively

        igd_opregion_pgbase =
            mem_hole_alloc(vbt_pages_needed - igd_opregion_e820_pages);

Jan