|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] tools/hvmloader: implement Intel IGD extended VBT support
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? Hi Jan, Thank you for your review. Well, your first question is quite provocative. Certainly more social/legal than technical. 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. 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. It would certainly be better to have the official spec from Intel, but alas, as far as I can tell, it is a proprietary spec that is most likely only available to Intel's OEM customers who need the spec to write the firmware for these devices. Of course we could ask Intel for the spec because we write firmware for these Intel IGD devices too. How do you think that would go? You, as the maintainer of Xen firmware that (at least implicitly in xl.cfg man pages, etc.) claims to support the Intel IGD, certainly have the right to ask them for the spec. Me, as a lowly customer/user of a handful of their devices at most, probably has less of a right to ask them for the spec. The situation here is analogous to Xen support for the Processor Properties Topology Table referenced in a commit that you Acked [1] just a few weeks ago. I presume you Acked that commit not because it is based on an official, open spec of the Processor Properties Topology Table that is available to the public, but because it is based on Linux kernel code that supports the Processor Properties Topology Table. [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=99794c8a8ff8b1d277c09d4736384fd5bb94f2d6 So it was acceptable to use a spec of the Processor Properties Topology Table derived from Linux kernel code as the basis for a commit to the Xen codebase just a few weeks ago. Why would it not also be acceptable to use an updated spec for the Intel IGD OpRegion and VBT derived from Linux kernel code in the code for tools/hvmloader in the Xen codebase that already has code that is based on the spec for older versions of the Intel IGD OpRegion and VBT? > >> --- >>[...] >> >> tools/firmware/hvmloader/Makefile | 1 + >> tools/firmware/hvmloader/config.h | 15 +- >> tools/firmware/hvmloader/e820.c | 4 +- >> tools/firmware/hvmloader/intel_opregion.c | 297 ++++++++++++++++++++++ > > Nit: Please use dashes in favor of underscores in new files' names. Ok. > >> --- a/tools/firmware/hvmloader/Makefile >> +++ b/tools/firmware/hvmloader/Makefile >> @@ -35,6 +35,7 @@ OBJS += smp.o cacheattr.o xenbus.o vnuma.o >> OBJS += e820.o pci.o pir.o ctype.o >> OBJS += hvm_param.o >> OBJS += ovmf.o seabios.o >> +OBJS += intel_opregion.o > > While this list isn't well sorted, I think your addition still wants to move > up by a line. Ok. > >> --- 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. Perhaps the best solution would be to presume there are so few current users of this feature that we do not need to worry about backward compatibility and breaking existing setups. I say this because the code here in hvmloader and in Qemu upstream to support Intel IGD passthrough is very badly bit rotten and I doubt there are very many, if any, working implementations currently in the wild based on unpatched vanilla Xen/Qemu upstream code, particularly with more modern Intel IGD devices and more recent versions of Qemu. If you give your blessing, then I can rework the patch without worrying so much about backward compatibility and about what happens when a guest is configured with a version of hvmloader that has this patch and a version of the DM that lacks the compatible patch, and vice versa, that is, when hvmloader lacks support instead of the DM lacking support. Then we could completely remove this oddity of setting IGD_OPREGION_PAGES to 3 in the current implementation in both hvmloader and the Qemu DM as well as many other oddities that result from the current implementation. > >> +#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. > >> --- a/tools/firmware/hvmloader/e820.c >> +++ b/tools/firmware/hvmloader/e820.c >> @@ -243,11 +243,11 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> >> e820[nr].addr = igd_opregion_base; >> - e820[nr].size = IGD_OPREGION_PAGES * PAGE_SIZE; >> + e820[nr].size = igd_opregion_e820_pages * PAGE_SIZE; >> e820[nr].type = E820_NVS; >> nr++; >> >> - e820[nr].addr = igd_opregion_base + IGD_OPREGION_PAGES * PAGE_SIZE; >> + e820[nr].addr = igd_opregion_base + igd_opregion_e820_pages * >> PAGE_SIZE; > > Are these new multiplications at risk of overflowing? I.e. how many pages can > there be in an extreme case? We are allocating down, so as igd_opregion_e820_pages grows, igd_opregion_base will shrink. The danger is that igd_opregion_base will go below the minimum possible value that is compatible with our memory map. I could add a check for that. I think our memory map allows for tens if not hundreds of pages in the region where the OpRegion and VBT are located, and typically the OpRegion + VBT is only about 4 or 5 pages. It should probably be a BUG() if somehow we detected a VBT whose size was large enough to cause this problem. > >> --- /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. > >> + * Copyright (c) 2026, Charles Zmudzinski. >> + * -- snip -- >> + * You should have received a copy of the GNU General Public License along >> with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + */ > > Please use an SPDX line instead in new files. OK. > >> +#include "util.h" >> +#include "config.h" >> +#include "pci_regs.h" >> + >> +unsigned long igd_opregion_pgbase = 0; >> +uint32_t igd_opregion_e820_pages = IGD_OPREGION_PAGES; >> + >> +static bool verify_opregion(const uint32_t addr) >> +{ >> + const char *opregion_signature = IGD_OPREGION_SIGNATURE; >> + if ( memcmp((const void *)addr, (const void *)opregion_signature, 16) ) >> + return false; >> + return true; >> +} > > Style: Blank line please between declaration(s) and statement(s) as well as > ahead of the main "return" of a function. There further isn't really a need > for an if() or two return statements here. Also please avoid casts wherever > possible. Finally, the local variable isn't really needed here either - the > string literal can be passed directly to memcmp(). All of this helps > readability as well. OK. > >> +static bool verify_vbt(const uint32_t addr) >> +{ >> + const char *vbt_signature = IGD_VBT_SIGNATURE; >> + if ( memcmp((const void *)addr, (const void *)vbt_signature, 4) ) >> + return false; >> + return true; >> +} > > Same comments here, obviously (and potentially elsewhere). OK. > >> +void intel_opregion_setup(uint32_t vga_devfn) >> +{ >> + uint32_t igd_guest_opregion; >> + uint32_t pages_needed; /* for OpRegion + VBT */ > > The former probably wants to be fixed-width, but for the latter I see no need. OK. > >> + void *opregion_scratch; >> + void *vbt_scratch; >> + void *vbt_source; >> + /* >> + * absolute value in the host/guest except >> + * as noted in the comments >> + */ > > Nit: Comment style (see ./CODING_STYLE). OK. > >> + 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. > >> + 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. > >> + 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. Also, I will more carefully read CODING_STYLE and try to fix all those issues you have pointed out (and any others I might find). > >> + /* >> + * 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); >> + return; >> + } else { > > No need for "else" after an unconditional "return". Ok. > >> + printf("host OpRegion address: 0x%x\n", > > The shorter %#x please (also elsewhere). Ok. > >> + igd_host_opregion); >> + } >> + >> + 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." > >> + igd_guest_opregion = (igd_opregion_pgbase << PAGE_SHIFT) | >> + igd_host_opregion_page_offset; >> + >> + /* >> + * We know at this point the device model supports >> + * OpRegion 2. >> + * >> + * Indicate to the device model that we support >> + * OpRegion 2 by setting the least significant bit >> + * of the address we give to the device model. >> + * The device model will notice this bit set and >> + * respond appropriately to our writes to the >> + * register where the OpRegion address is stored. >> + */ > > Specifically noticeable here: Please make better use of line length in > long(ish) comments. Ok. > >> + 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. > >> + 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? > >> + BUG(); >> + } >> + --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. Actually, I only use rvds_page_offset below to help decide whether or not to retain the host OpRegion page offset in the guest. I don't know if this is necessary, though, and I could test without retaining the same page offset in the guest and always place the both the OpRegion and the VBT on a page boundary in the guest (if I place the OpRegion on a page boundary and also always place the VBT contiguous after the OpRegion, the VBT will always be placed exactly two pages after the base of the OpRegion and thus also on a page boundary). All the devices I test have enough room to retain the page offset of the host in the guest without requiring allocation of an extra page, so if there are regressions I will notice them in my testing. If always placing OpRegion and VBT on a page boundary works with no regressions, then I could completely remove rvds_page_offset from the code. I will still need rvda_host_page_offset though, because it is needed to get the exact location of the VBT in the guest when the DM temporarily maps the host VBT to the guest. > >> + 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. The KVM/vfio solution is to have the kernel vfio driver provide rvda to Qemu, and I think it would be possible for the xen-pciback kernel driver to also expose rvda to the DM, but that would likely require patches to the kernel xen-pciback driver and probably also to libxl or other toolstack which uses QMP to plug the Xen PCI passthrough devices into the PCI bus provided by the DM. This solution avoids needing to touch libxl and kernel drivers. > >> + /* In this case, we use the mapped OpRegion */ >> + if ( !rvda_host ) >> + return; >> + >> + /* >> + * 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. > >> --- a/tools/firmware/hvmloader/pci.c >> +++ b/tools/firmware/hvmloader/pci.c >> @@ -43,7 +43,6 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0; >> #define BAR_RELOC_THRESH GB(1) >> >> enum virtual_vga virtual_vga = VGA_none; >> -unsigned long igd_opregion_pgbase = 0; >> >> /* Check if the specified range conflicts with any reserved device memory. >> */ >> static bool check_overlap_all(uint64_t start, uint64_t size) >> @@ -190,14 +189,7 @@ void pci_setup(void) >> virtual_vga = VGA_pt; >> 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); >> + intel_opregion_setup(vga_devfn); >> } > > With this preferably also drop the figure braces. Ok. > > Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |