2023-09-19 07:25:20

by Saurabh Sengar

Subject: [PATCH v2] x86/hyperv: Restrict get_vtl to only VTL platforms

For non VTL platforms vtl is always 0, and there is no need of
get_vtl function. For VTL platforms get_vtl should always succeed
and should return the correct VTL.

Signed-off-by: Saurabh Sengar <[email protected]>
---
[V2]
- Put the if else at function definition rather then at the caller

arch/x86/hyperv/hv_init.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 783ed339f341..f0128fd4031d 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -394,6 +394,7 @@ 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;
@@ -416,13 +417,16 @@ static u8 __init get_vtl(void)
if (hv_result_success(ret)) {
ret = output->as64.low & HV_X64_VTL_MASK;
} else {
- pr_err("Failed to get VTL(%lld) and set VTL to zero by default.\n", ret);
- ret = 0;
+ 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
@@ -604,8 +608,7 @@ void __init hyperv_init(void)
hv_query_ext_cap(0);

/* Find the VTL */
- if (!ms_hyperv.paravisor_present && hv_isolation_type_snp())
- ms_hyperv.vtl = get_vtl();
+ ms_hyperv.vtl = get_vtl();

return;

--
2.34.1

2023-09-19 15:15:25

by Vitaly Kuznetsov

Subject: Re: [PATCH v2] x86/hyperv: Restrict get_vtl to only VTL platforms

Saurabh Sengar <[email protected]> writes:

> For non VTL platforms vtl is always 0, and there is no need of
> get_vtl function. For VTL platforms get_vtl should always succeed
> and should return the correct VTL.

Nitpicking, an alternative summary:

"""
When Linux runs in a non-default VTL (CONFIG_HYPERV_VTL_MODE=y),
get_vtl() must never fail as the result it returns is used in
negotiations with the host. In the more generic case,
(CONFIG_HYPERV_VTL_MODE=n) the VTL is always zero so there's no need to
do the hypercall.

Make get_vtl() BUG() in case of failure and put the implementation under
"if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)" to avoid the call altogether in
the most generic use case.
"""

>
> Signed-off-by: Saurabh Sengar <[email protected]>

Reviewed-by: Vitaly Kuznetsov <[email protected]>

> ---
> [V2]
> - Put the if else at function definition rather then at the caller
>
> arch/x86/hyperv/hv_init.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 783ed339f341..f0128fd4031d 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -394,6 +394,7 @@ 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;
> @@ -416,13 +417,16 @@ static u8 __init get_vtl(void)
> if (hv_result_success(ret)) {
> ret = output->as64.low & HV_X64_VTL_MASK;
> } else {
> - pr_err("Failed to get VTL(%lld) and set VTL to zero by default.\n", ret);
> - ret = 0;
> + 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
> @@ -604,8 +608,7 @@ void __init hyperv_init(void)
> hv_query_ext_cap(0);
>
> /* Find the VTL */
> - if (!ms_hyperv.paravisor_present && hv_isolation_type_snp())
> - ms_hyperv.vtl = get_vtl();
> + ms_hyperv.vtl = get_vtl();
>
> return;

--
Vitaly