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

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

On 8/14/2026 9:46 AM, Jan Beulich wrote:
> On 14.08.2026 15:18, Chuck Zmudzinski wrote:
>> On 8/14/2026 3:35 AM, Jan Beulich wrote:
>>> 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 agree that having the spec at hand would be better. To be more precise, I
>> can say that what this patch essentially does is port the support for
>> the extended VBT with OpRegion 2+ for the Intel IGD passthrough that exists
>> in KVM/vfio to Xen. Should I explicitly say in the title of the commit
>> message that this is a port of KVM/vfio support for extended VBT to Xen?
> 
> Not in the title, as that would likely make it too long, but perhaps in the
> description.
> 
>>>> 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?
>> 
>> No, I do not expect every reader to decipher the underlying information...
>> 
>> That is why I provided these two links at the bottom of the commit message.
>> Perhaps you did not notice them:
>> 
>> Link: https://lore.kernel.org/kvm/20211012124855.52463-1-colin.xu@xxxxxxxxx/
>> Link: https://lore.kernel.org/kvm/20210325170953.24549-1-fred.gao@xxxxxxxxx/
>> 
>> They are the patches to the vfio kernel driver that added support for the
>> extended VBT for KVM/vfio guests.
> 
> Patches can still be in flight, so provide only limited help. Would it be a
> problem to instead reference commits, or the actual localtion in Linux
> sources?
> 
>>>>>> +    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.
>> 
>> Ok. Perhaps the IGD_* defines could be moved to a public header to define the
>> interface to be used to support the Intel IGD. Would it be OK to move those
>> to a separate igd.h header
> 
> This may require input by others, as in the given situation I'm not quite
> sure what is best. Anthony - do you possibly have any suggestion here?
> 
>> and include it in hvmloader/config.h?
> 
> I don't see why that would be needed. The few files which need the #define-s
> can include that new public header, without impacting anything else.
> 
>>>>>> +    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.
>> 
>> I don't think I understand what you mean by "No full stop in messages..."
> 
> That's the period at the end of a sentence (when in log messages the term
> "sentence" is of questionable nature).
> 
>> We have code like this in hvmloader/e820.c:
>> 
>>     if ( rc || !nr_entries )
>>     {
>>         printf("Get guest memory maps[%d] failed. (%d)\n", nr_entries, rc);
>>         BUG();
>>     }
> 
> Well, you'll almost always be able to find bad pre-existing examples.
> 
>>>>>> +    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.
>> 
>> I think the host OpRegion is not currently accessible by the DM.
> 
> Can you explain to me how the region becomes accessible to the guest?
> That would then (hopefully) help me understand why the DM would not have
> access. Fundamentally any MMIO and any I/O ports that are assigned to a
> guest are also assigned to its DM.

This is what I don't understand about your objection to how both the current
implementation and my proposed changes makes the host OpRegion accessible to
the guest. What do you mean when you say any MMIO and I/O ports assigned to
a guest are also assigned to its DM? What does it mean to assign an MMIO
region to a DM? Is it the DM you mean or the DM domain, which need not be
dom0 if we are running the device model in an unprivileged domain. I also
am presuming you know that dom0 for Intel IGD passthrough is a PV dom0,
not a PVH dom0. I have never tried Intel IGD passthrough with a PVH dom0,
because as far as I can tell vt-d is not supported with PVH dom0.

Take a look at this code from our current implementation in qemu-xen. This
is from the current master branch of qemu-xen on xenbits.xen.org, the
hw/xen/xen_pt_graphics.c file, the igd_write_opregion function:

--- snip ---

#define XEN_PCI_INTEL_OPREGION_PAGES 0x3
#define XEN_PCI_INTEL_OPREGION_ENABLE_ACCESSED 0x1
void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val)
{
    int ret;

    if (igd_guest_opregion) {
        XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring %x\n",
                   val);
        return;
    }

    /* We just work with LE. */
    xen_host_pci_get_block(&s->real_device, XEN_PCI_INTEL_OPREGION,
            (uint8_t *)&igd_host_opregion, 4);
    igd_guest_opregion = (unsigned long)(val & ~XEN_PCI_INTEL_OPREGION_MASK)
                            | (igd_host_opregion & XEN_PCI_INTEL_OPREGION_MASK);

    ret = xc_domain_iomem_permission(xen_xc, xen_domid,
            (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
            XEN_PCI_INTEL_OPREGION_PAGES,
            XEN_PCI_INTEL_OPREGION_ENABLE_ACCESSED);

    if (ret) {
        XEN_PT_ERR(&s->dev, "[%d]:Can't enable to access IGD host opregion:"
                    " 0x%lx.\n", ret,
                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT)),
        igd_guest_opregion = 0;
        return;
    }

    ret = xc_domain_memory_mapping(xen_xc, xen_domid,
            (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT),
            (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
            XEN_PCI_INTEL_OPREGION_PAGES,
            DPCI_ADD_MAPPING);

    if (ret) {
        XEN_PT_ERR(&s->dev, "[%d]:Can't map IGD host opregion:0x%lx to"
                    " guest opregion:0x%lx.\n", ret,
                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
                    (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT));
        igd_guest_opregion = 0;
        return;
    }

    XEN_PT_LOG(&s->dev, "Map OpRegion: 0x%lx -> 0x%lx\n",
                    (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
                    (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT));
}

--- snip ---

This function is called when the guest (i.e. hvmloader, seabios/ovmf, or
guest kernel code) tries to access (write to) what is known as the ASLS
register in the PCI config space of the Intel IGD. The config space is
256 bytes long, and the ASLS register is the last four bytes of that space
according to the proprietary spec from Intel for the OpRegion. So the address
for the ASLS register in the config space is 0xfc, and the four bytes stored
there is supposed to be the address of the OpRegion according to Intel's spec.
That is fundamentally what we are trying to do here - program that ASLS register
so it points to the location, in the guest, of the OpRegion. If you examine
the code above, you will notice the call to xen_host_pci_get_block(), with
XEN_PCI_INTEL_OPREGION as one of the parameters. Did you look up its value?
It is 0xfc, the value for the ASLS register in the Intel spec. How does the
DM, qemu-xen, get the value stored there? Well, the xen_host_pci_get_block()
function accesses the PCI config space from the device model not directly as
kernel code or platform firmware code such as hvmloader or OVMF/Seabios could,
but only indirectly, through the 256-byte config file that is exposed by the
Linux kernel sysfs interface at /sys/bus/pci/devices/0000:00:02.0/config in
the Linux host filesystem. If you don't believe me, take a look at the code
in hw/xen/xen-host-pci-device.c where the xen_host_pci_get_block() function is
implemented in qemu-xen.

So the device model can, indirectly, access the PCI device's config space
because the Linux kernel exposes it via the sysfs interface. The point is,
the DM's access to these resources of the passed through PCI device has
nothing to do with MMIO or I/O port mappings, but is entirely dependent on
the host dom0 kernel for access. But sysfs does not provide access to the
OpRegion, that is, the actual two pages that comprise the OpRegion whose
base address is the value stored in the ASLS register. That is fundamentally
why the DM does not have access to the OpRegion.

Do you understand now?

Chuck

> 
>> On the KVM
>> platform, this is made possible via the kernel vfio driver and then Qemu 
>> exposes
>> the OpRegion to the guest using the Qemu FwCfg device interface. How should 
>> we make
>> the OpRegion and VBT accessible to the device model and then, to the guest, 
>> on Xen?
>> I think it could be done via the xen-pciback kernel driver. Should we do that
>> instead? I think to do that we would have to convince the kernel developers 
>> that
>> the Intel OpRegion, as you say, "should" be accessible by the Xen device 
>> model.
>> I can imagine them saying, why not use the vfio driver?
> 
> I can't answer this; all I can say is that it feels wrong to involve e.g.
> xen-pciback here.
> 
> Jan