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

[Naman Jain <namjain@linux.microsoft.com>]

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.

> 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