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.
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 */"
Please let me know your thoughts on this.
Regards,
Naman
