From: Naman Jain <[email protected]> Sent: Tuesday, May 5, 2026 10:50 PM > > On 5/4/2026 9:36 PM, Michael Kelley wrote: > > From: Naman Jain <[email protected]> Sent: Wednesday, April 29, > > 2026 2:58 AM > >> > >> On 4/27/2026 11:10 AM, Michael Kelley wrote: > >>> From: Naman Jain <[email protected]> Sent: Thursday, April 23, > >>> 2026 5:42 AM > >>>> > >>>> Move hv_vtl_configure_reg_page() from drivers/hv/mshv_vtl_main.c to > >>>> arch/x86/hyperv/hv_vtl.c. The register page overlay is an x86-specific > >>>> feature that uses HV_X64_REGISTER_REG_PAGE, so its configuration belongs > >>>> in architecture-specific code. > >>>> > >>>> Move struct mshv_vtl_per_cpu and union hv_synic_overlay_page_msr to > >>>> include/asm-generic/mshyperv.h so they are visible to both arch and > >>>> driver code. > >>>> > >>>> Change the return type from void to bool so the caller can determine > >>>> whether the register page was successfully configured and set > >>>> mshv_has_reg_page accordingly. > >>>> > >>>> Signed-off-by: Naman Jain <[email protected]> > >>>> --- > >>>> arch/x86/hyperv/hv_vtl.c | 32 ++++++++++++++++++++++ > >>>> drivers/hv/mshv_vtl_main.c | 49 +++------------------------------- > >>>> include/asm-generic/mshyperv.h | 17 ++++++++++++ > >>>> 3 files changed, 53 insertions(+), 45 deletions(-) > >>>> > >> > >> <snip> > >> > >>>> #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) > >>>> +/* SYNIC_OVERLAY_PAGE_MSR - internal, identical to hv_synic_simp */ > >>> > >>> This comment pre-dates your patch, but I don't understand the point > >>> it is trying to make. The comment is factually true, but I don't know > >>> why calling that out is relevant. The REG_PAGE MSR seems to be > >>> conceptually separate and distinct from the SIMP MSR, so the fact > >>> that the layouts are the same is just a coincidence. Or is there some > >>> relationship between the two MSRs that I'm not aware of, and the > >>> comment is trying (and failing?) to point out? > >> > >> This was added as per suggestion from Nuno in my initial series for > >> MSHV_VTL. If the reference in "identical to" is misleading, I should > >> remove it. > >> > >> https://lore.kernel.org/all/[email protected]/ > >> > >> Quoting: > >> """ > >> it is a generic structure that > >> appears to be used for several overlay page MSRs (SIMP, SIEF, etc). > >> > >> But, the type doesn't appear in the hv*dk headers explicitly; it's just > >> used internally by the hypervisor. > >> > >> I think it should be renamed with a hv_ prefix to indicate it's part of > >> the hypervisor ABI, and a brief comment with the provenance: > >> > >> /* SYNIC_OVERLAY_PAGE_MSR - internal, identical to hv_synic_simp */ > >> union hv_synic_overlay_page_msr { > >> /* <snip> */ > >> }; > > > > OK, so this union is not associated *only* with the REG_PAGE MSR > > (though that MSR is the only current user). Instead, it is intended to > > be a more generic description of MSRs that set up overlay pages. I > > don't think I had previously noticed Nuno's comment on the topic. > > > > Looking through hvgdk_mini.h and hvhdk.h, I see 6 definitions that > > are exactly the same: > > > > * union hv_reference_tsc_msr > > * union hv_x64_msr_hypercall_contents > > * union hv_vp_assist_msr_contents > > * union hv_synic_simp > > * union hv_synic_siefp > > * union hv_synic_sirbp > > > > There's an argument to be made for removing these 6 unique definitions > > and using union hv_synic_overlay_page_msr instead (though "synic" > > would need to be removed from the name). I would not object to such > > an approach. It's a small extra layer of conceptual indirection, but saves > > some lines of code for duplicative definitions. The alternative is to drop > > the idea of a generic overlay page MSR layout, and replace union > > hv_synic_overlay_page_msr with a definition that is specific to the > > REG_PAGE MSR, like the other six above. > > > > Hi Michael, > > While having a generic definition looks good to have here, I can see two > reasons for not going ahead with generic overlay page definition: > 1. All of the above definitions are present in Hyper-V headers and > generalizing them would deviate from the strategy of keeping the kernel > headers in line with Hyper-V headers. > 2. For any of these definitions, if the use-case requires using some of > these reserved bits, then it would be a problem. I can actually see that > happening in "hv_x64_msr_hypercall_contents" in the corresponding > variant in the Hyper-V header.
Your points are certainly valid, and I'm good with not going the generic route. > > > I could go either way. If we want to use a generic overlay page definition, > > then that approach should be applied everywhere. With the current > > state of your patch set, we're halfway in between -- the generic definition > > is used one place, but duplicative specific MSR definitions are used other > > places. That's probably the least desirable approach. > > > > Michael > > > Now, coming back to the hv_synic_overlay_page_msr definition. While > Nuno's comment hinted at it being "generic", the same is not documented > in the name of this structure or its comments. So it should be safe to > assume that it is specific to synic_overlay_page_msr usage. But since it > is not part of Hyper-V header as such, we needed that comment: > "/* SYNIC_OVERLAY_PAGE_MSR - internal, identical to hv_synic_simp */" > An "overlay page" is a generic concept in the Hyper-V world, and it is used in multiple places in the guest<->hypervisor interface. The old PDF version of the Hyper-V TLFS describes overlay pages in the section 5.2.1 entitled "GPA Overlay Pages". See [1]. Unfortunately, this material about overlay pages doesn't seem to have been carried over to the web page version of the TLFS. So in my thinking, the name "hv_synic_overlay_page_msr" is inherently a generic definition that could be applied to multiple MSRs that are used to specify overlay pages. Your patch is about a specific MSR, HV_X64_REGISTER_REG_PAGE, which happens to be used to define an overlay page. But if the decision is to *not* go the generic route, I would expect to see something like "union hv_x64_reg_page_msr" that is specific to the REG_PAGE MSR, and to have that type used in hv_vtl_configure_reg_page(). The definition of hv_x64_reg_page_msr would not have a comment referencing the SIMP or any other MSR because it would be a standalone definition that is specific to HV_X64_REGISTER_REG_PAGE. Then the pattern would be the same as the other six cases that I listed above. When not using the generic approach, hv_synic_overlay_page_msr really has no purpose, and could go away. Michael [1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf
