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

Michael Kelley
2026-05-06T14:36:33Z

From: Naman Jain <namjain@linux.microsoft.com> Sent: Tuesday, May 5, 2026 10:50 PM
>
> On 5/4/2026 9:36 PM, Michael Kelley wrote:
> > From: Naman Jain <namjain@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@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@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.
> >
>
> 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