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. 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
