2023-09-13 19:56:11

by Saurabh Sengar

Subject: [PATCH] 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]>
---
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);
+ 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;

return;

--
2.34.1

2023-09-14 16:11:01

by Vitaly Kuznetsov

Subject: Re: [PATCH] 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.
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> 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

2023-09-15 08:50:33

by Saurabh Sengar

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

On Thu, Sep 14, 2023 at 04:49:08PM +0200, Vitaly Kuznetsov wrote:
> 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.
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > 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.

Good suggestion, I can do this as a separate patch.

>
> > + 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?


Fine with me, can update in v2.

- Saurabh

>
> >
> > return;
>
> --
> Vitaly
>