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

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

On 8/14/2026 11:23 AM, Chuck Zmudzinski wrote:
> 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 --
>> 
>>>>>>> +    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.
> 
> So I would just include it in the new intel-opregion.c file. Also, maybe 
> igd-related
> declarations should be moved there too, such as the currently existing extern 
> variable
> igd_opregion_pgbase and my newly proposed extern variable 
> igd_opregion_e820_pages,
> which would mean the new header would also need to be included in 
> hvmloader/e820.c.

Actually, those igd-related declarations do not need to be in a public header. 
But I
think if we go to a public header for any DM to consume, we need to fixup 
oddities
like the current definition of IGD_OPREGION_PAGES of 3 when the actual number 
of pages
in the OpRegion is exactly 2. So I propose the next version of this patch should
add a preliminary patch to cleanup the oddities in the current implementation 
such as
having IGD_OPREGION_PAGES set to 3 without introducing any functional change by
redefining IGD_OPREGION_PAGES to the value it should be, which is 2. Then we can
include IGD_OPREGION2_SUPPORT_MASK, IGD_OPREGION_PAGES, etc. as defines in a 
public
header for any DM to consume. I can probably build such a public header 
directly from
IGD-related header files in use in the Linux kernel or in the Qemu/vfio 
IGD-related
headers files.

Chuck