[PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86

[Michael Kelley]

From: Naman Jain <namjain at linux.microsoft.com> Sent: Wednesday, April 29, 2026 2:58 AM
> 
> On 4/27/2026 11:10 AM, Michael Kelley wrote:
> > From: Naman Jain <namjain at linux.microsoft.com> 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 <namjain at linux.microsoft.com>
> >> ---
> >>   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/68143eb0-e6a7-4579-bedb-4c2ec5aaef6b@linux.microsoft.com/
> 
> 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