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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Saurabh Sengar <ssengar@linux.microsoft.com> 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.
>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 783ed339f341..e589c240565a 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -416,8 +416,8 @@ 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);

Nitpick: arch/x86/hyperv/hv_init.c lacks pr_fmt so the message won't get
prefixed with "Hyper-V". I'm not sure 'VTL' abbreviation has the only,
Hyper-V specific meaning. I'd suggest we add 

#define pr_fmt(fmt)  "Hyper-V: " fmt

to the beginning of the file.

> +		BUG();
>  	}
>  
>  	local_irq_restore(flags);
> @@ -604,8 +604,10 @@ void __init hyperv_init(void)
>  	hv_query_ext_cap(0);
>  
>  	/* Find the VTL */
> -	if (!ms_hyperv.paravisor_present && hv_isolation_type_snp())
> +	if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))
>  		ms_hyperv.vtl = get_vtl();
> +	else
> +		ms_hyperv.vtl = 0;

Is 'else' branch really needed? 'ms_hyperv' seems to be a statically
allocated global. But instead of doing this, what about putting the
whole get_vtl() funtion under '#if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))', i.e.:

#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
static u8 __init get_vtl(void)
{
        u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
...
}
#else
static inline get_vtl(void) { return 0; }
#endif

and then we can always do

      ms_hyperv.vtl = get_vtl();

unconditionally?

>  
>  	return;

-- 
Vitaly