| Message ID | 20240514224508.212318-4-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | arm64/hyperv: Support Virtual Trust Level Boot | expand |
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM > > To run in the VTL mode, Hyper-V drivers have to know what > VTL the system boots in, and the arm64/hyperv code does not > have the means to compute that. > > Refactor the code to hoist the function that detects VTL, > make it arch-neutral to be able to employ it to get the VTL > on arm64. Nit: In patches that just refactor and move some code around, patch authors will often include a statement like "No functional changes" (or the slightly more doubtful "No functional changes intended") as a separate line at the end of the commit message. It's just something the reader/reviewer can cross-check against. > > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > --- > arch/x86/hyperv/hv_init.c | 34 ----------------------- > arch/x86/include/asm/hyperv-tlfs.h | 7 ----- > drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++ > include/asm-generic/hyperv-tlfs.h | 7 +++++ > include/asm-generic/mshyperv.h | 6 +++++ > 5 files changed, 56 insertions(+), 41 deletions(-) > > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c > index 17a71e92a343..c350fa05ee59 100644 > --- a/arch/x86/hyperv/hv_init.c > +++ b/arch/x86/hyperv/hv_init.c > @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void) > local_irq_restore(flags); > } > > -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) > -static u8 __init get_vtl(void) > -{ > - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; > - struct hv_get_vp_registers_input *input; > - struct hv_get_vp_registers_output *output; > - unsigned long flags; > - u64 ret; > - > - local_irq_save(flags); > - input = *this_cpu_ptr(hyperv_pcpu_input_arg); > - output = (struct hv_get_vp_registers_output *)input; > - > - memset(input, 0, struct_size(input, element, 1)); > - input->header.partitionid = HV_PARTITION_ID_SELF; > - input->header.vpindex = HV_VP_INDEX_SELF; > - input->header.inputvtl = 0; > - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS; > - > - ret = hv_do_hypercall(control, input, output); > - if (hv_result_success(ret)) { > - ret = output->as64.low & HV_X64_VTL_MASK; > - } else { > - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); > - BUG(); > - } > - > - local_irq_restore(flags); > - return ret; > -} > -#else > -static inline u8 get_vtl(void) { return 0; } > -#endif > - > /* > * This function is to be invoked early in the boot sequence after the > * hypervisor has been detected. > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > index 3787d26810c1..9ee68eb8e6ff 100644 > --- a/arch/x86/include/asm/hyperv-tlfs.h > +++ b/arch/x86/include/asm/hyperv-tlfs.h > @@ -309,13 +309,6 @@ enum hv_isolation_type { > #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG) > #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT) > > -/* > - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and > - * there is not associated MSR address. > - */ > -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003 > -#define HV_X64_VTL_MASK GENMASK(3, 0) > - > /* Hyper-V memory host visibility */ > enum hv_mem_host_visibility { > VMBUS_PAGE_NOT_VISIBLE = 0, > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c > index dde3f9b6871a..d4cf477a4d0c 100644 > --- a/drivers/hv/hv_common.c > +++ b/drivers/hv/hv_common.c > @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 > param2) > return HV_STATUS_INVALID_PARAMETER; > } > EXPORT_SYMBOL_GPL(hv_tdx_hypercall); > + > +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) > +u8 __init get_vtl(void) > +{ > + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; > + struct hv_get_vp_registers_input *input; > + struct hv_get_vp_registers_output *output; > + unsigned long flags; > + u64 ret; > + > + local_irq_save(flags); > + input = *this_cpu_ptr(hyperv_pcpu_input_arg); > + output = (struct hv_get_vp_registers_output *)input; I gave some bad advice on the original version of the above code. Rather than allocating a separate hypercall output area, I had suggested just setting the hypercall output to be the same as the input area, which is done above. While the overlap doesn't cause any problems for a hypercall returning the value of single register, the TLFS says to never set up such an overlap. Since this code is being moved anyway, there's an opportunity to fix this by setting output to "input" + "struct_size(input, element, 1)". Or put the output at the start of the second half of the page (note that the size is HY_HYP_PAGE_SIZE, not PAGE_SIZE). The input and output for 1 register are relatively small and both easily fit with either approach. They just shouldn't overlap. Some might argue this tweak should be made in a separate patch, but in my judgment, it's OK to do it here if you note it in the commit message. Your call. Of course, if you make this change, my previous comment about "No functional changes" no longer applies. :-) Michael > + > + memset(input, 0, struct_size(input, element, 1)); > + input->header.partitionid = HV_PARTITION_ID_SELF; > + input->header.vpindex = HV_VP_INDEX_SELF; > + input->header.inputvtl = 0; > + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS; > + > + ret = hv_do_hypercall(control, input, output); > + if (hv_result_success(ret)) { > + ret = output->as64.low & HV_VTL_MASK; > + } else { > + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); > + > + /* > + * This is a dead end, something fundamental is broken. > + * > + * There is no sensible way of continuing as the Hyper-V drivers > + * transitively depend via the vmbus driver on knowing which VTL > + * they run in to establish communication with the host. The kernel > + * is going to be worse off if continued booting than a panicked one, > + * just hung and stuck, producing second-order failures, with neither > + * a way to recover nor to provide expected services. > + */ > + BUG(); > + } > + > + local_irq_restore(flags); > + return ret; > +} > +#endif > diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h > index 87e3d49a4e29..682bcda3124f 100644 > --- a/include/asm-generic/hyperv-tlfs.h > +++ b/include/asm-generic/hyperv-tlfs.h > @@ -75,6 +75,13 @@ > /* AccessTscInvariantControls privilege */ > #define HV_ACCESS_TSC_INVARIANT BIT(15) > > +/* > + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS > + * hvcall, and there is no an associated MSR on x86. > + */ > +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003 > +#define HV_VTL_MASK GENMASK(3, 0) > + > /* > * Group B features. > */ > diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h > index 99935779682d..ea434186d765 100644 > --- a/include/asm-generic/mshyperv.h > +++ b/include/asm-generic/mshyperv.h > @@ -301,4 +301,10 @@ static inline enum hv_isolation_type > hv_get_isolation_type(void) > } > #endif /* CONFIG_HYPERV */ > > +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) > +u8 __init get_vtl(void); > +#else > +static inline u8 get_vtl(void) { return 0; } > +#endif > + > #endif > -- > 2.45.0 >
On 5/15/2024 6:38 AM, Michael Kelley wrote: > From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM >> >> To run in the VTL mode, Hyper-V drivers have to know what >> VTL the system boots in, and the arm64/hyperv code does not >> have the means to compute that. >> >> Refactor the code to hoist the function that detects VTL, >> make it arch-neutral to be able to employ it to get the VTL >> on arm64. > > Nit: In patches that just refactor and move some code around, > patch authors will often include a statement like "No functional > changes" (or the slightly more doubtful "No functional changes > intended") as a separate line at the end of the commit message. > It's just something the reader/reviewer can cross-check against. > >> >> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> >> --- >> arch/x86/hyperv/hv_init.c | 34 ----------------------- >> arch/x86/include/asm/hyperv-tlfs.h | 7 ----- >> drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++ >> include/asm-generic/hyperv-tlfs.h | 7 +++++ >> include/asm-generic/mshyperv.h | 6 +++++ >> 5 files changed, 56 insertions(+), 41 deletions(-) >> >> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c >> index 17a71e92a343..c350fa05ee59 100644 >> --- a/arch/x86/hyperv/hv_init.c >> +++ b/arch/x86/hyperv/hv_init.c >> @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void) >> local_irq_restore(flags); >> } >> >> -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) >> -static u8 __init get_vtl(void) >> -{ >> - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; >> - struct hv_get_vp_registers_input *input; >> - struct hv_get_vp_registers_output *output; >> - unsigned long flags; >> - u64 ret; >> - >> - local_irq_save(flags); >> - input = *this_cpu_ptr(hyperv_pcpu_input_arg); >> - output = (struct hv_get_vp_registers_output *)input; >> - >> - memset(input, 0, struct_size(input, element, 1)); >> - input->header.partitionid = HV_PARTITION_ID_SELF; >> - input->header.vpindex = HV_VP_INDEX_SELF; >> - input->header.inputvtl = 0; >> - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS; >> - >> - ret = hv_do_hypercall(control, input, output); >> - if (hv_result_success(ret)) { >> - ret = output->as64.low & HV_X64_VTL_MASK; >> - } else { >> - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); >> - BUG(); >> - } >> - >> - local_irq_restore(flags); >> - return ret; >> -} >> -#else >> -static inline u8 get_vtl(void) { return 0; } >> -#endif >> - >> /* >> * This function is to be invoked early in the boot sequence after the >> * hypervisor has been detected. >> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h >> index 3787d26810c1..9ee68eb8e6ff 100644 >> --- a/arch/x86/include/asm/hyperv-tlfs.h >> +++ b/arch/x86/include/asm/hyperv-tlfs.h >> @@ -309,13 +309,6 @@ enum hv_isolation_type { >> #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG) >> #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT) >> >> -/* >> - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and >> - * there is not associated MSR address. >> - */ >> -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003 >> -#define HV_X64_VTL_MASK GENMASK(3, 0) >> - >> /* Hyper-V memory host visibility */ >> enum hv_mem_host_visibility { >> VMBUS_PAGE_NOT_VISIBLE = 0, >> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c >> index dde3f9b6871a..d4cf477a4d0c 100644 >> --- a/drivers/hv/hv_common.c >> +++ b/drivers/hv/hv_common.c >> @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 >> param2) >> return HV_STATUS_INVALID_PARAMETER; >> } >> EXPORT_SYMBOL_GPL(hv_tdx_hypercall); >> + >> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) >> +u8 __init get_vtl(void) >> +{ >> + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; >> + struct hv_get_vp_registers_input *input; >> + struct hv_get_vp_registers_output *output; >> + unsigned long flags; >> + u64 ret; >> + >> + local_irq_save(flags); >> + input = *this_cpu_ptr(hyperv_pcpu_input_arg); >> + output = (struct hv_get_vp_registers_output *)input; > > I gave some bad advice on the original version of the above code. Rather > than allocating a separate hypercall output area, I had suggested just setting > the hypercall output to be the same as the input area, which is done > above. While the overlap doesn't cause any problems for a hypercall returning > the value of single register, the TLFS says to never set up such an overlap. > > Since this code is being moved anyway, there's an opportunity to fix > this by setting output to "input" + "struct_size(input, element, 1)". Or > put the output at the start of the second half of the page (note that the > size is HY_HYP_PAGE_SIZE, not PAGE_SIZE). The input and output for 1 > register are relatively small and both easily fit with either approach. They > just shouldn't overlap. > > Some might argue this tweak should be made in a separate patch, but > in my judgment, it's OK to do it here if you note it in the commit > message. Your call. Of course, if you make this change, my previous > comment about "No functional changes" no longer applies. :-) I'll update the code to adhere to the TLFS requirements, thank you :) > > Michael > >> + >> + memset(input, 0, struct_size(input, element, 1)); >> + input->header.partitionid = HV_PARTITION_ID_SELF; >> + input->header.vpindex = HV_VP_INDEX_SELF; >> + input->header.inputvtl = 0; >> + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS; >> + >> + ret = hv_do_hypercall(control, input, output); >> + if (hv_result_success(ret)) { >> + ret = output->as64.low & HV_VTL_MASK; >> + } else { >> + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); >> + >> + /* >> + * This is a dead end, something fundamental is broken. >> + * >> + * There is no sensible way of continuing as the Hyper-V drivers >> + * transitively depend via the vmbus driver on knowing which VTL >> + * they run in to establish communication with the host. The kernel >> + * is going to be worse off if continued booting than a panicked one, >> + * just hung and stuck, producing second-order failures, with neither >> + * a way to recover nor to provide expected services. >> + */ >> + BUG(); >> + } >> + >> + local_irq_restore(flags); >> + return ret; >> +} >> +#endif >> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h >> index 87e3d49a4e29..682bcda3124f 100644 >> --- a/include/asm-generic/hyperv-tlfs.h >> +++ b/include/asm-generic/hyperv-tlfs.h >> @@ -75,6 +75,13 @@ >> /* AccessTscInvariantControls privilege */ >> #define HV_ACCESS_TSC_INVARIANT BIT(15) >> >> +/* >> + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS >> + * hvcall, and there is no an associated MSR on x86. >> + */ >> +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003 >> +#define HV_VTL_MASK GENMASK(3, 0) >> + >> /* >> * Group B features. >> */ >> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h >> index 99935779682d..ea434186d765 100644 >> --- a/include/asm-generic/mshyperv.h >> +++ b/include/asm-generic/mshyperv.h >> @@ -301,4 +301,10 @@ static inline enum hv_isolation_type >> hv_get_isolation_type(void) >> } >> #endif /* CONFIG_HYPERV */ >> >> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) >> +u8 __init get_vtl(void); >> +#else >> +static inline u8 get_vtl(void) { return 0; } >> +#endif >> + >> #endif >> -- >> 2.45.0 >>
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c index 17a71e92a343..c350fa05ee59 100644 --- a/arch/x86/hyperv/hv_init.c +++ b/arch/x86/hyperv/hv_init.c @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void) local_irq_restore(flags); } -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) -static u8 __init get_vtl(void) -{ - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; - struct hv_get_vp_registers_input *input; - struct hv_get_vp_registers_output *output; - unsigned long flags; - u64 ret; - - local_irq_save(flags); - input = *this_cpu_ptr(hyperv_pcpu_input_arg); - output = (struct hv_get_vp_registers_output *)input; - - memset(input, 0, struct_size(input, element, 1)); - input->header.partitionid = HV_PARTITION_ID_SELF; - input->header.vpindex = HV_VP_INDEX_SELF; - input->header.inputvtl = 0; - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS; - - ret = hv_do_hypercall(control, input, output); - if (hv_result_success(ret)) { - ret = output->as64.low & HV_X64_VTL_MASK; - } else { - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); - BUG(); - } - - local_irq_restore(flags); - return ret; -} -#else -static inline u8 get_vtl(void) { return 0; } -#endif - /* * This function is to be invoked early in the boot sequence after the * hypervisor has been detected. diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index 3787d26810c1..9ee68eb8e6ff 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h @@ -309,13 +309,6 @@ enum hv_isolation_type { #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG) #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT) -/* - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and - * there is not associated MSR address. - */ -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003 -#define HV_X64_VTL_MASK GENMASK(3, 0) - /* Hyper-V memory host visibility */ enum hv_mem_host_visibility { VMBUS_PAGE_NOT_VISIBLE = 0, diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c index dde3f9b6871a..d4cf477a4d0c 100644 --- a/drivers/hv/hv_common.c +++ b/drivers/hv/hv_common.c @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 param2) return HV_STATUS_INVALID_PARAMETER; } EXPORT_SYMBOL_GPL(hv_tdx_hypercall); + +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) +u8 __init get_vtl(void) +{ + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS; + struct hv_get_vp_registers_input *input; + struct hv_get_vp_registers_output *output; + unsigned long flags; + u64 ret; + + local_irq_save(flags); + input = *this_cpu_ptr(hyperv_pcpu_input_arg); + output = (struct hv_get_vp_registers_output *)input; + + memset(input, 0, struct_size(input, element, 1)); + input->header.partitionid = HV_PARTITION_ID_SELF; + input->header.vpindex = HV_VP_INDEX_SELF; + input->header.inputvtl = 0; + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS; + + ret = hv_do_hypercall(control, input, output); + if (hv_result_success(ret)) { + ret = output->as64.low & HV_VTL_MASK; + } else { + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret); + + /* + * This is a dead end, something fundamental is broken. + * + * There is no sensible way of continuing as the Hyper-V drivers + * transitively depend via the vmbus driver on knowing which VTL + * they run in to establish communication with the host. The kernel + * is going to be worse off if continued booting than a panicked one, + * just hung and stuck, producing second-order failures, with neither + * a way to recover nor to provide expected services. + */ + BUG(); + } + + local_irq_restore(flags); + return ret; +} +#endif diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h index 87e3d49a4e29..682bcda3124f 100644 --- a/include/asm-generic/hyperv-tlfs.h +++ b/include/asm-generic/hyperv-tlfs.h @@ -75,6 +75,13 @@ /* AccessTscInvariantControls privilege */ #define HV_ACCESS_TSC_INVARIANT BIT(15) +/* + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS + * hvcall, and there is no an associated MSR on x86. + */ +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003 +#define HV_VTL_MASK GENMASK(3, 0) + /* * Group B features. */ diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h index 99935779682d..ea434186d765 100644 --- a/include/asm-generic/mshyperv.h +++ b/include/asm-generic/mshyperv.h @@ -301,4 +301,10 @@ static inline enum hv_isolation_type hv_get_isolation_type(void) } #endif /* CONFIG_HYPERV */ +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE) +u8 __init get_vtl(void); +#else +static inline u8 get_vtl(void) { return 0; } +#endif + #endif
To run in the VTL mode, Hyper-V drivers have to know what VTL the system boots in, and the arm64/hyperv code does not have the means to compute that. Refactor the code to hoist the function that detects VTL, make it arch-neutral to be able to employ it to get the VTL on arm64. Signed-off-by: Roman Kisel <romank@linux.microsoft.com> --- arch/x86/hyperv/hv_init.c | 34 ----------------------- arch/x86/include/asm/hyperv-tlfs.h | 7 ----- drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++ include/asm-generic/hyperv-tlfs.h | 7 +++++ include/asm-generic/mshyperv.h | 6 +++++ 5 files changed, 56 insertions(+), 41 deletions(-)