* RE: [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 [not found] ` <567cf73b-6a48-4fc3-b312-9be6d71e2f22@linux.microsoft.com> @ 2026-05-04 16:06 ` Michael Kelley 2026-05-06 5:50 ` Naman Jain 0 siblings, 1 reply; 10+ messages in thread From: Michael Kelley @ 2026-05-04 16:06 UTC (permalink / raw) To: Naman Jain, Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com 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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 2026-05-04 16:06 ` [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 Michael Kelley @ 2026-05-06 5:50 ` Naman Jain 2026-05-06 14:36 ` Michael Kelley 0 siblings, 1 reply; 10+ messages in thread From: Naman Jain @ 2026-05-06 5:50 UTC (permalink / raw) To: Michael Kelley Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com, Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 2026-05-06 5:50 ` Naman Jain @ 2026-05-06 14:36 ` Michael Kelley 2026-05-07 3:43 ` Naman Jain 0 siblings, 1 reply; 10+ messages in thread From: Michael Kelley @ 2026-05-06 14:36 UTC (permalink / raw) To: Naman Jain, Michael Kelley Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com, Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 2026-05-06 14:36 ` Michael Kelley @ 2026-05-07 3:43 ` Naman Jain 0 siblings, 0 replies; 10+ messages in thread From: Naman Jain @ 2026-05-07 3:43 UTC (permalink / raw) To: Michael Kelley Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20260423124206.2410879-8-namjain@linux.microsoft.com>]
[parent not found: <SN6PR02MB4157C147A1B915F9B45D3B74D4362@SN6PR02MB4157.namprd02.prod.outlook.com>]
[parent not found: <516a4e65-3a4d-4e09-b445-28cb740b5653@linux.microsoft.com>]
* RE: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call [not found] ` <516a4e65-3a4d-4e09-b445-28cb740b5653@linux.microsoft.com> @ 2026-05-04 16:06 ` Michael Kelley 2026-05-06 5:11 ` Naman Jain 0 siblings, 1 reply; 10+ messages in thread From: Michael Kelley @ 2026-05-04 16:06 UTC (permalink / raw) To: Naman Jain, Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, April 29, 2026 2:57 AM > > On 4/27/2026 11:08 AM, Michael Kelley wrote: > > From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, April 23, 2026 5:42 AM > >> [snip] > >> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > >> index 08278547b84c..b4d80c9a673a 100644 > >> --- a/arch/x86/include/asm/mshyperv.h > >> +++ b/arch/x86/include/asm/mshyperv.h > >> @@ -286,7 +286,6 @@ struct mshv_vtl_cpu_context { > >> #ifdef CONFIG_HYPERV_VTL_MODE > >> void __init hv_vtl_init_platform(void); > >> int __init hv_vtl_early_init(void); > >> -void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); > >> void mshv_vtl_return_call_init(u64 vtl_return_offset); > >> void mshv_vtl_return_hypercall(void); > >> void __mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); > >> @@ -294,7 +293,6 @@ int hv_vtl_get_set_reg(struct hv_register_assoc *regs, bool set, bool shared); > >> #else > >> static inline void __init hv_vtl_init_platform(void) {} > >> static inline int __init hv_vtl_early_init(void) { return 0; } > >> -static inline void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} > >> static inline void mshv_vtl_return_call_init(u64 vtl_return_offset) {} > >> static inline void mshv_vtl_return_hypercall(void) {} > >> static inline void __mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} > >> diff --git a/drivers/hv/mshv_vtl.h b/drivers/hv/mshv_vtl.h > >> index a6eea52f7aa2..103f07371f3f 100644 > >> --- a/drivers/hv/mshv_vtl.h > >> +++ b/drivers/hv/mshv_vtl.h > >> @@ -22,4 +22,7 @@ struct mshv_vtl_run { > >> char vtl_ret_actions[MSHV_MAX_RUN_MSG_SIZE]; > >> }; > >> > >> +static_assert(sizeof(struct mshv_vtl_cpu_context) <= 1024, > >> + "struct mshv_vtl_cpu_context exceeds reserved space in struct mshv_vtl_run"); > >> + > >> #endif /* _MSHV_VTL_H */ > >> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h > >> index db183c8cfb95..8cdf2a9fbdfb 100644 > >> --- a/include/asm-generic/mshyperv.h > >> +++ b/include/asm-generic/mshyperv.h > >> @@ -396,8 +396,10 @@ static inline int hv_deposit_memory(u64 partition_id, u64 status) > >> > >> #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) > >> u8 __init get_vtl(void); > >> +void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); > >> #else > >> static inline u8 get_vtl(void) { return 0; } > >> +static inline void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} > > > > Is this stub needed? Maybe I missed something, but it looks to me like none > > of the code that calls this gets built unless CONFIG_HYPERV_VTL_MODE is set. > > See further comments about stubs in Patch 8 of this series. > > > > Config dependencies would handle such cases, and this is not required. I > saw similar stubs added in the code, so I thought this is a norm that > should be followed, and not rely on config dependencies. > I can remove it. > Others might disagree with me, but I don't think it's the norm to add stubs when they aren't truly needed. As you can see from some of my other comments, I look for ways to eliminate stubs. Stubs are indicative of a boundary between separately built components, and I generally try to minimize the surface area of such boundaries. A large surface area often means that the overall design could be improved by re-thinking which code goes with which component. Michael ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call 2026-05-04 16:06 ` [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call Michael Kelley @ 2026-05-06 5:11 ` Naman Jain 0 siblings, 0 replies; 10+ messages in thread From: Naman Jain @ 2026-05-06 5:11 UTC (permalink / raw) To: Michael Kelley Cc: Marc Zyngier, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org, vdso@mailbox.org, ssengar@linux.microsoft.com, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86@kernel.org, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti On 5/4/2026 9:36 PM, Michael Kelley wrote: > From: Naman Jain <namjain@linux.microsoft.com> Sent: Wednesday, April 29, 2026 2:57 AM >> >> On 4/27/2026 11:08 AM, Michael Kelley wrote: >>> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, April 23, 2026 5:42 AM >>>> > > [snip] > >>>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h >>>> index 08278547b84c..b4d80c9a673a 100644 >>>> --- a/arch/x86/include/asm/mshyperv.h >>>> +++ b/arch/x86/include/asm/mshyperv.h >>>> @@ -286,7 +286,6 @@ struct mshv_vtl_cpu_context { >>>> #ifdef CONFIG_HYPERV_VTL_MODE >>>> void __init hv_vtl_init_platform(void); >>>> int __init hv_vtl_early_init(void); >>>> -void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); >>>> void mshv_vtl_return_call_init(u64 vtl_return_offset); >>>> void mshv_vtl_return_hypercall(void); >>>> void __mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); >>>> @@ -294,7 +293,6 @@ int hv_vtl_get_set_reg(struct hv_register_assoc *regs, bool set, bool shared); >>>> #else >>>> static inline void __init hv_vtl_init_platform(void) {} >>>> static inline int __init hv_vtl_early_init(void) { return 0; } >>>> -static inline void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} >>>> static inline void mshv_vtl_return_call_init(u64 vtl_return_offset) {} >>>> static inline void mshv_vtl_return_hypercall(void) {} >>>> static inline void __mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} >>>> diff --git a/drivers/hv/mshv_vtl.h b/drivers/hv/mshv_vtl.h >>>> index a6eea52f7aa2..103f07371f3f 100644 >>>> --- a/drivers/hv/mshv_vtl.h >>>> +++ b/drivers/hv/mshv_vtl.h >>>> @@ -22,4 +22,7 @@ struct mshv_vtl_run { >>>> char vtl_ret_actions[MSHV_MAX_RUN_MSG_SIZE]; >>>> }; >>>> >>>> +static_assert(sizeof(struct mshv_vtl_cpu_context) <= 1024, >>>> + "struct mshv_vtl_cpu_context exceeds reserved space in struct mshv_vtl_run"); >>>> + >>>> #endif /* _MSHV_VTL_H */ >>>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h >>>> index db183c8cfb95..8cdf2a9fbdfb 100644 >>>> --- a/include/asm-generic/mshyperv.h >>>> +++ b/include/asm-generic/mshyperv.h >>>> @@ -396,8 +396,10 @@ static inline int hv_deposit_memory(u64 partition_id, u64 status) >>>> >>>> #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) >>>> u8 __init get_vtl(void); >>>> +void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0); >>>> #else >>>> static inline u8 get_vtl(void) { return 0; } >>>> +static inline void mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {} >>> >>> Is this stub needed? Maybe I missed something, but it looks to me like none >>> of the code that calls this gets built unless CONFIG_HYPERV_VTL_MODE is set. >>> See further comments about stubs in Patch 8 of this series. >>> >> >> Config dependencies would handle such cases, and this is not required. I >> saw similar stubs added in the code, so I thought this is a norm that >> should be followed, and not rely on config dependencies. >> I can remove it. >> > > Others might disagree with me, but I don't think it's the norm to add > stubs when they aren't truly needed. As you can see from some of my > other comments, I look for ways to eliminate stubs. Stubs are indicative > of a boundary between separately built components, and I generally > try to minimize the surface area of such boundaries. A large surface area > often means that the overall design could be improved by re-thinking > which code goes with which component. > > Michael I agree. I'll remove these in next version. Thanks, Naman ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <aeolHwXHFH4AnX_n@J2N7QTR9R3.cambridge.arm.com>]
[parent not found: <f4059f5d-a82b-40c2-942e-3e24cefab94f@linux.microsoft.com>]
* Re: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call [not found] ` <f4059f5d-a82b-40c2-942e-3e24cefab94f@linux.microsoft.com> @ 2026-05-06 7:52 ` Mark Rutland 2026-05-08 4:26 ` Naman Jain 2026-05-08 9:25 ` Marc Zyngier 1 sibling, 1 reply; 10+ messages in thread From: Mark Rutland @ 2026-05-06 7:52 UTC (permalink / raw) To: Naman Jain Cc: Marc Zyngier, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Michael Kelley, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv, linux-arm-kernel, linux-kernel, linux-arch, linux-riscv, vdso, ssengar On Wed, Apr 29, 2026 at 03:26:11PM +0530, Naman Jain wrote: > On 4/23/2026 7:26 PM, Mark Rutland wrote: > > On Thu, Apr 23, 2026 at 12:41:57PM +0000, Naman Jain wrote: [ non-SMMC hypercall code omitted for brevity ] > > NAK to this. > > > > * This is a non-SMCCC hypercall, which we have NAK'd in general in the > > past for various reasons that I am not going to rehash here. > > > > * It's not clear how this is going to be extended with necessary > > architecture state in future (e.g. SVE, SME). This is not > > future-proof, and I don't believe this is maintainable. > > > > * This breaks general requirements for reliable stacktracing by > > clobbering state (e.g. x29) that we depend upon being valid AT ALL > > TIMES outside of entry code. > > > > * IMO, if this needs to be saved/restored, that should happen in > > whatever you are calling. > > > > Mark. > > Merging threads for addressing comments from Mark Rutland and Marc Zyngier > on this patch. > > Thanks for reviewing the changes. Please allow me to briefly explain the use > case here and then address your comments. > > Hyper-V's Virtual Trust Levels (VTLs) provide hardware-enforced isolation > within a single VM, analogous to ARM TrustZone. The kernel runs in VTL2 > (higher privilege) as a "paravisor", a security monitor that handles > intercepts for the primary OS in VTL0 (lower privilege). The VTL switch > (mshv_vtl_return_call) is functionally equivalent to KVM's guest enter/exit. It's worth noting that for KVM, the KVM hyp code is *tightly* coupled with the host kernel (they are one single binary object), and the calling convention between the two is an implementation detail that can change at any time without any ABI concerns. While I appreciate this might be trying to do the same thing from a *functional* perspective, it's certainly different from a maintainability perspective, and can't be treated in the same way. > It saves VTL2 state, loads VTL0's GPRs other registers from a shared context > structure, issues hvc #3 to let VTL0 run, and on return saves VTL0's updated > state back. > > Coming to the problems with the code, I have identified a few ways to > address them. > > I can put the assembly code in a separate .S file with > SYM_FUNC_START/SYM_FUNC_END and marked as noinstr, to prevent ftrace/kprobes > from instrumenting between the GPR load and the hvc, which could have > corrupted VTL0 register state. This should solve x29 clobbering, stack > tracing problems. My point was that you must not clobber those registers. Looking at the TLFS document you linked below, it says: | Note: X29 (FP/frame pointer), X30 (LR/link register), and SP are private | per-VTL ... so clobbering those doesn't seem to be necessary anyway. Clearly having an arbitrary calling convention is confusing for everyone. > I should use kernel_neon_begin()/kernel_neon_end() to save/restore the full > extended FP state of the current task in VTL2. VTL0's Q0-Q31 can be > loaded/saved separately via fpsimd_load_state()/fpsimd_save_state(). This > way, the assembly touches none of the SIMD registers. This is SVE/SME-safe > for VTL2's task state. VTL0 still only carries Q0-Q31 in the context struct, > and extending to SVE, SME is a future context struct change, which will need > Hyper-V arm64 ABI support. > This way, VTL2's callee-saved regs (x19-x28, x29, x30) are explicitly saved > to the stack frame at the top and restored at the bottom of assembly code. > The C caller (in hv_vtl.c) is a clean function call. That doesn't really address my concerns here. I do not think that Linux should have to save/restore anything here; that should be the job of the real hypervisor. The arbitrary separation of PE state into private and shared (with shred state being directly exposed to Linux) is a problem for maintainability and forward compatibility. Looking at the TLFS document you linked below, I see: | Note: SVE state (Z0-Z31, P0-P15, FFR) and SME state are VTL-private. | The lower 128-bit portion (Q registers) is shared, but the upper bits | of Z registers may be corrupted on VTL transitions. Software should | not rely on Z register contents being preserved across VTL switches. ... which is certainly going to be a pain to manage. Note in particular "SME state" is not an architectural term. I don't know which state in particular that is intended to cover (e.g. ZA, ZT0, SVCR, all streaming mode state)? There's no mention of SVCR, so I don't know how this is going to interact with management of ZA state (ZA and ZT0, which are dependent upon SVCR.ZA) or streaming mode (dependent upon SVCR.SM). That state has been *incredibly* painful for us to manage generally. Regardless of the SMCCC concerns, that needs to be specified better. > Regarding Non-SMCCC "hvc #3" call, I have a limitation here owing to the ABI > that is defined by the Hyper-V hypervisor. Fixing this requires a > hypervisor-side change to support SMCCC-style dispatch for VTL return. Until > then, hvc #3 is the only working interface. Moreover there would be backward > compatibility issues with this new ABI interface, if at all it is added. To be clear, that's Microsoft's problem, not the Linux kernel community's problem. My NAK still stands. Multiple years ago now, we made it clear that we would not accept a non-SMCCC calling convention. Ignoring the substance of that feedback, and inventing a new calling convention after that point is a self-inflicted problem. [...] > Link to TLFS: https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/vsm#on-arm64-platforms-3 For shared state, aside fomr GPRs and FPSIMD/SVE/SME state, that says: | * System Information Registers (read-only or non-security-critical): | * System identification and feature registers | * Cache and TLB type information It's *implied* that some of those registers might be writable, but as the specific set of registers is not described I cannot tell. Are there any writable system registers which are shared? I don't see how we can know which registers we might need to save/restore without that being explicitly documented. I also see: | Note: SPE (Statistical Profiling Extension) state is shared across VTLs, | except for PMBSR_EL1 which is VTL-private. If "SPE state" includes PMBPTR or PMBLIMITR (which is the obvious reading), this would be a security problem, as a lower-privileged VTL could clobber those and cause SPE to write to arbitrary memory immediately upon return to the higher-privileged VTL. Having PMBSR be private on its own isn't sufficient to prevent that (e.g. since the higher-privileged VTL could have its own active SPE profiling session). I'm not keen on requiring hyper-v specific hooks in the SPE driver to achieve that, and I'm also not keen on having hyper-v support code poke SPE registers behind the SPE driver's back. This does not give me confidence that any future PE state (e.g. things like TRBE) will be managed in a safe way either. Mark. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call 2026-05-06 7:52 ` Mark Rutland @ 2026-05-08 4:26 ` Naman Jain 0 siblings, 0 replies; 10+ messages in thread From: Naman Jain @ 2026-05-08 4:26 UTC (permalink / raw) To: Mark Rutland Cc: Marc Zyngier, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Michael Kelley, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv, linux-arm-kernel, linux-kernel, linux-arch, linux-riscv, vdso, ssengar On 5/6/2026 1:22 PM, Mark Rutland wrote: > On Wed, Apr 29, 2026 at 03:26:11PM +0530, Naman Jain wrote: >> On 4/23/2026 7:26 PM, Mark Rutland wrote: >>> On Thu, Apr 23, 2026 at 12:41:57PM +0000, Naman Jain wrote: > > [ non-SMMC hypercall code omitted for brevity ] > >>> NAK to this. >>> >>> * This is a non-SMCCC hypercall, which we have NAK'd in general in the >>> past for various reasons that I am not going to rehash here. >>> >>> * It's not clear how this is going to be extended with necessary >>> architecture state in future (e.g. SVE, SME). This is not >>> future-proof, and I don't believe this is maintainable. >>> >>> * This breaks general requirements for reliable stacktracing by >>> clobbering state (e.g. x29) that we depend upon being valid AT ALL >>> TIMES outside of entry code. >>> >>> * IMO, if this needs to be saved/restored, that should happen in >>> whatever you are calling. >>> >>> Mark. >> >> Merging threads for addressing comments from Mark Rutland and Marc Zyngier >> on this patch. >> >> Thanks for reviewing the changes. Please allow me to briefly explain the use >> case here and then address your comments. >> >> Hyper-V's Virtual Trust Levels (VTLs) provide hardware-enforced isolation >> within a single VM, analogous to ARM TrustZone. The kernel runs in VTL2 >> (higher privilege) as a "paravisor", a security monitor that handles >> intercepts for the primary OS in VTL0 (lower privilege). The VTL switch >> (mshv_vtl_return_call) is functionally equivalent to KVM's guest enter/exit. > > It's worth noting that for KVM, the KVM hyp code is *tightly* coupled > with the host kernel (they are one single binary object), and the > calling convention between the two is an implementation detail that can > change at any time without any ABI concerns. > > While I appreciate this might be trying to do the same thing from a > *functional* perspective, it's certainly different from a > maintainability perspective, and can't be treated in the same way. > >> It saves VTL2 state, loads VTL0's GPRs other registers from a shared context >> structure, issues hvc #3 to let VTL0 run, and on return saves VTL0's updated >> state back. >> >> Coming to the problems with the code, I have identified a few ways to >> address them. >> >> I can put the assembly code in a separate .S file with >> SYM_FUNC_START/SYM_FUNC_END and marked as noinstr, to prevent ftrace/kprobes >> from instrumenting between the GPR load and the hvc, which could have >> corrupted VTL0 register state. This should solve x29 clobbering, stack >> tracing problems. > > My point was that you must not clobber those registers. > > Looking at the TLFS document you linked below, it says: > > | Note: X29 (FP/frame pointer), X30 (LR/link register), and SP are private > | per-VTL > > ... so clobbering those doesn't seem to be necessary anyway. Clearly > having an arbitrary calling convention is confusing for everyone. > >> I should use kernel_neon_begin()/kernel_neon_end() to save/restore the full >> extended FP state of the current task in VTL2. VTL0's Q0-Q31 can be >> loaded/saved separately via fpsimd_load_state()/fpsimd_save_state(). This >> way, the assembly touches none of the SIMD registers. This is SVE/SME-safe >> for VTL2's task state. VTL0 still only carries Q0-Q31 in the context struct, >> and extending to SVE, SME is a future context struct change, which will need >> Hyper-V arm64 ABI support. >> This way, VTL2's callee-saved regs (x19-x28, x29, x30) are explicitly saved >> to the stack frame at the top and restored at the bottom of assembly code. >> The C caller (in hv_vtl.c) is a clean function call. > > That doesn't really address my concerns here. > > I do not think that Linux should have to save/restore anything here; > that should be the job of the real hypervisor. The arbitrary separation > of PE state into private and shared (with shred state being directly > exposed to Linux) is a problem for maintainability and forward > compatibility. > > Looking at the TLFS document you linked below, I see: > > | Note: SVE state (Z0-Z31, P0-P15, FFR) and SME state are VTL-private. > | The lower 128-bit portion (Q registers) is shared, but the upper bits > | of Z registers may be corrupted on VTL transitions. Software should > | not rely on Z register contents being preserved across VTL switches. > > ... which is certainly going to be a pain to manage. > > Note in particular "SME state" is not an architectural term. I don't > know which state in particular that is intended to cover (e.g. ZA, ZT0, > SVCR, all streaming mode state)? > > There's no mention of SVCR, so I don't know how this is going to > interact with management of ZA state (ZA and ZT0, which are dependent > upon SVCR.ZA) or streaming mode (dependent upon SVCR.SM). That state has > been *incredibly* painful for us to manage generally. Regardless of the > SMCCC concerns, that needs to be specified better. > >> Regarding Non-SMCCC "hvc #3" call, I have a limitation here owing to the ABI >> that is defined by the Hyper-V hypervisor. Fixing this requires a >> hypervisor-side change to support SMCCC-style dispatch for VTL return. Until >> then, hvc #3 is the only working interface. Moreover there would be backward >> compatibility issues with this new ABI interface, if at all it is added. > > To be clear, that's Microsoft's problem, not the Linux kernel > community's problem. My NAK still stands. > > Multiple years ago now, we made it clear that we would not accept a > non-SMCCC calling convention. Ignoring the substance of that feedback, > and inventing a new calling convention after that point is a > self-inflicted problem. > > [...] > >> Link to TLFS: https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/vsm#on-arm64-platforms-3 > > For shared state, aside fomr GPRs and FPSIMD/SVE/SME state, that says: > > | * System Information Registers (read-only or non-security-critical): > | * System identification and feature registers > | * Cache and TLB type information > > It's *implied* that some of those registers might be writable, but as > the specific set of registers is not described I cannot tell. Are there > any writable system registers which are shared? > > I don't see how we can know which registers we might need to > save/restore without that being explicitly documented. > > I also see: > > | Note: SPE (Statistical Profiling Extension) state is shared across VTLs, > | except for PMBSR_EL1 which is VTL-private. > > If "SPE state" includes PMBPTR or PMBLIMITR (which is the obvious > reading), this would be a security problem, as a lower-privileged VTL > could clobber those and cause SPE to write to arbitrary memory > immediately upon return to the higher-privileged VTL. Having PMBSR be > private on its own isn't sufficient to prevent that (e.g. since the > higher-privileged VTL could have its own active SPE profiling session). > > I'm not keen on requiring hyper-v specific hooks in the SPE driver to > achieve that, and I'm also not keen on having hyper-v support code poke > SPE registers behind the SPE driver's back. > > This does not give me confidence that any future PE state (e.g. things > like TRBE) will be managed in a safe way either. > > Mark. Thanks for sharing this, I'll discuss it internally and come up with a plan. Regards, Naman ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call [not found] ` <f4059f5d-a82b-40c2-942e-3e24cefab94f@linux.microsoft.com> 2026-05-06 7:52 ` Mark Rutland @ 2026-05-08 9:25 ` Marc Zyngier 2026-05-08 9:56 ` Naman Jain 1 sibling, 1 reply; 10+ messages in thread From: Marc Zyngier @ 2026-05-08 9:25 UTC (permalink / raw) To: Naman Jain Cc: Mark Rutland, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Michael Kelley, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv, linux-arm-kernel, linux-kernel, linux-arch, linux-riscv, vdso, ssengar On Wed, 29 Apr 2026 10:56:11 +0100, Naman Jain <namjain@linux.microsoft.com> wrote: > [...] > Merging threads for addressing comments from Mark Rutland and Marc > Zyngier on this patch. > > Thanks for reviewing the changes. Please allow me to briefly explain > the use case here and then address your comments. > > Hyper-V's Virtual Trust Levels (VTLs) provide hardware-enforced > isolation within a single VM, analogous to ARM TrustZone. The kernel > runs in VTL2 (higher privilege) as a "paravisor", a security monitor > that handles intercepts for the primary OS in VTL0 (lower > privilege). The VTL switch (mshv_vtl_return_call) is functionally > equivalent to KVM's guest enter/exit. It saves VTL2 state, loads > VTL0's GPRs other registers from a shared context structure, issues > hvc #3 to let VTL0 run, and on return saves VTL0's updated state back. No, this is fundamentally different. KVM is purely architectural, doesn't try to "sanitise" anything, and context switches *all* of the guest state. No ifs, no buts, no "reserved registers". [...] > Regarding Non-SMCCC "hvc #3" call, I have a limitation here owing to > the ABI that is defined by the Hyper-V hypervisor. Fixing this > requires a hypervisor-side change to support SMCCC-style dispatch for > VTL return. Until then, hvc #3 is the only working interface. Moreover > there would be backward compatibility issues with this new ABI > interface, if at all it is added. Left hand, please talk to right hand. This is not the first time we push back on this, and we already had this annoying discussion back when arm64 as a Hyper-V guest was first proposed (6, 7 years ago?). What has changed since? Absolutely nothing. If the Hyper-V folks decide to ignore the standard and go their own way, that's fine. They only get to keep the pieces. Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call 2026-05-08 9:25 ` Marc Zyngier @ 2026-05-08 9:56 ` Naman Jain 0 siblings, 0 replies; 10+ messages in thread From: Naman Jain @ 2026-05-08 9:56 UTC (permalink / raw) To: Marc Zyngier Cc: Mark Rutland, K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H . Peter Anvin, Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Michael Kelley, Timothy Hayes, Lorenzo Pieralisi, Sascha Bischoff, mrigendrachaubey, linux-hyperv, linux-arm-kernel, linux-kernel, linux-arch, linux-riscv, vdso, ssengar On 5/8/2026 2:55 PM, Marc Zyngier wrote: > On Wed, 29 Apr 2026 10:56:11 +0100, > Naman Jain <namjain@linux.microsoft.com> wrote: >> > > [...] > >> Merging threads for addressing comments from Mark Rutland and Marc >> Zyngier on this patch. >> >> Thanks for reviewing the changes. Please allow me to briefly explain >> the use case here and then address your comments. >> >> Hyper-V's Virtual Trust Levels (VTLs) provide hardware-enforced >> isolation within a single VM, analogous to ARM TrustZone. The kernel >> runs in VTL2 (higher privilege) as a "paravisor", a security monitor >> that handles intercepts for the primary OS in VTL0 (lower >> privilege). The VTL switch (mshv_vtl_return_call) is functionally >> equivalent to KVM's guest enter/exit. It saves VTL2 state, loads >> VTL0's GPRs other registers from a shared context structure, issues >> hvc #3 to let VTL0 run, and on return saves VTL0's updated state back. > > No, this is fundamentally different. KVM is purely architectural, > doesn't try to "sanitise" anything, and context switches *all* of the > guest state. No ifs, no buts, no "reserved registers". > > [...] Acked. > >> Regarding Non-SMCCC "hvc #3" call, I have a limitation here owing to >> the ABI that is defined by the Hyper-V hypervisor. Fixing this >> requires a hypervisor-side change to support SMCCC-style dispatch for >> VTL return. Until then, hvc #3 is the only working interface. Moreover >> there would be backward compatibility issues with this new ABI >> interface, if at all it is added. > > Left hand, please talk to right hand. This is not the first time we > push back on this, and we already had this annoying discussion back > when arm64 as a Hyper-V guest was first proposed (6, 7 years ago?). > > What has changed since? Absolutely nothing. > > If the Hyper-V folks decide to ignore the standard and go their own > way, that's fine. They only get to keep the pieces. > > Thanks, > > M. > Thanks for the feedback. I understand your and Mark’s concerns with this approach now, and I’ve initiated internal discussions with the team to explore potential solutions. Regards, Naman ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-05-08 9:56 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260423124206.2410879-1-namjain@linux.microsoft.com>
[not found] ` <20260423124206.2410879-10-namjain@linux.microsoft.com>
[not found] ` <SN6PR02MB4157467FDBC0203C67A67042D4362@SN6PR02MB4157.namprd02.prod.outlook.com>
[not found] ` <567cf73b-6a48-4fc3-b312-9be6d71e2f22@linux.microsoft.com>
2026-05-04 16:06 ` [PATCH v2 09/15] Drivers: hv: mshv_vtl: Move hv_vtl_configure_reg_page() to x86 Michael Kelley
2026-05-06 5:50 ` Naman Jain
2026-05-06 14:36 ` Michael Kelley
2026-05-07 3:43 ` Naman Jain
[not found] ` <20260423124206.2410879-8-namjain@linux.microsoft.com>
[not found] ` <SN6PR02MB4157C147A1B915F9B45D3B74D4362@SN6PR02MB4157.namprd02.prod.outlook.com>
[not found] ` <516a4e65-3a4d-4e09-b445-28cb740b5653@linux.microsoft.com>
2026-05-04 16:06 ` [PATCH v2 07/15] arm64: hyperv: Add support for mshv_vtl_return_call Michael Kelley
2026-05-06 5:11 ` Naman Jain
[not found] ` <aeolHwXHFH4AnX_n@J2N7QTR9R3.cambridge.arm.com>
[not found] ` <f4059f5d-a82b-40c2-942e-3e24cefab94f@linux.microsoft.com>
2026-05-06 7:52 ` Mark Rutland
2026-05-08 4:26 ` Naman Jain
2026-05-08 9:25 ` Marc Zyngier
2026-05-08 9:56 ` Naman Jain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox