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/6/2026 8:06 PM, Michael Kelley wrote:
> 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


Thanks for suggesting this, I was not aware of it. I'll change it to 
hv_x64_reg_page_msr and remove the comment.

Regards,
Naman


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv