Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Saurabh Sengar <ssengar@linux.microsoft.com>
Cc:     ssengar@microsoft.com, kys@microsoft.com, haiyangz@microsoft.com,
        wei.liu@kernel.org, decui@microsoft.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, linux-hyperv@vger.kernel.org,
        linux-kernel@vger.kernel.org, mikelley@microsoft.com
Subject: Re: [PATCH v2] x86/hyperv: Restrict get_vtl to only VTL platforms
In-Reply-To: <1695101408-22432-1-git-send-email-ssengar@linux.microsoft.com>
References: <1695101408-22432-1-git-send-email-ssengar@linux.microsoft.com>
Date:   Tue, 19 Sep 2023 13:04:45 +0200
Message-ID: <87jzsme84y.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.

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 <ssengar@linux.microsoft.com>

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

> ---
> [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