RE: [RFC PATCH V2 05/18] x86/hyperv: Get Virtual Trust Level via hvcall

[Michael Kelley (LINUX)]

From: Tianyu Lan <ltykernel@xxxxxxxxx> Sent: Friday, November 18, 2022 7:46 PM
> 
> sev-snp guest provides vtl(Virtual Trust Level) and get it from
> hyperv hvcall via HVCALL_GET_VP_REGISTERS.

Two general comments:

1) Could this patch be combined with Patch 9 of the series?  It seems
like they go together since Patch 9 is the consumer of the VTL.

2) What is the bigger picture motivation for this patch and Patch 9
being part of the patch series for support fully enlightened SEV-SNP
guests?  Won't the VTL always be 0 in such a guest?  The code currently
assumes VTL 0, so it seems like this patch doesn't change anything.  Or
maybe there's a scenario that I'm not aware of where the VTL is
other than 0.

> 
> Signed-off-by: Tianyu Lan <tiala@xxxxxxxxxxxxx>
> ---
>  arch/x86/hyperv/hv_init.c      | 35 ++++++++++++++++++++++++++++++++++
>  include/asm-generic/mshyperv.h |  2 ++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 4600c5941957..5b919d4d24c0 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -390,6 +390,39 @@ static void __init hv_get_partition_id(void)
>  	local_irq_restore(flags);
>  }
> 
> +static u8 __init get_current_vtl(void)

The name get_current_vtl() seems to imply that there might be a
"previous" VTL, or that the VTL might change over time.  I'm not aware
that either is the case.  Couldn't this just be get_vtl()?

> +{
> +	u64 control = ((u64)1 << HV_HYPERCALL_REP_COMP_OFFSET) | HVCALL_GET_VP_REGISTERS;

Simplify by using HV_HYPERCALL_REP_COMP_1.

> +	struct hv_get_vp_registers_input *input = NULL;
> +	struct hv_get_vp_registers_output *output = NULL;

It doesn't seem like the above two initializations to NULL are needed.

> +	u8 vtl = 0;
> +	int ret;

The result of hv_do_hypercall() should always be a u64.

> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	input = *(struct hv_get_vp_registers_input **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +	output = (struct hv_get_vp_registers_output *)input;
> +	if (!input || !output) {

Don't need to check both values since one is assigned from the other. :-)

> +		pr_err("Hyper-V: cannot allocate a shared page!");

Error message text isn't correct.

> +		goto done;

Need to do local_irq_restore() before goto done.

> +	}
> +
> +	memset(input, 0, sizeof(*input) + sizeof(input->element[0]));
> +	input->header.partitionid = HV_PARTITION_ID_SELF;
> +	input->header.inputvtl = 0;
> +	input->element[0].name0 = 0x000D0003;

This constant should go in one of the hyperv-tlfs.h header files.  If I
recall correctly, we're currently treating VTLs as x86-specific, so should
go in arch/x86/include/asm/hyperv-tlfs.h.

> +
> +	ret = hv_do_hypercall(control, input, output);
> +	if (ret == 0)

Use hv_result_success(ret).

> +		vtl = output->as64.low & 0xf;

The 0xF mask should be defined in the hyperv-tlfs.h per
above.

> +	else
> +		pr_err("Hyper-V: failed to get the current VTL!");

Again, drop the word "current".  And the exclamation mark isn't needed. :-)

> +	local_irq_restore(flags);
> +
> +done:
> +	return vtl;
> +}
> +
>  /*
>   * This function is to be invoked early in the boot sequence after the
>   * hypervisor has been detected.
> @@ -527,6 +560,8 @@ void __init hyperv_init(void)
>  	if (hv_is_isolation_supported())
>  		swiotlb_update_mem_attributes();
>  #endif
> +	/* Find the current VTL */
> +	ms_hyperv.vtl = get_current_vtl();

Drop "current" in the comment and function name.

> 
>  	return;
> 
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index bfb9eb9d7215..68133de044ec 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -46,6 +46,7 @@ struct ms_hyperv_info {
>  		};
>  	};
>  	u64 shared_gpa_boundary;
> +	u8 vtl;
>  };
>  extern struct ms_hyperv_info ms_hyperv;
> 
> @@ -55,6 +56,7 @@ extern void * __percpu *hyperv_pcpu_output_arg;
>  extern u64 hv_do_hypercall(u64 control, void *inputaddr, void *outputaddr);
>  extern u64 hv_do_fast_hypercall8(u16 control, u64 input8);
>  extern bool hv_isolation_type_snp(void);
> +extern bool hv_isolation_type_en_snp(void);

This declaration of hv_isolation_type_en_snp() shouldn't be needed here
as it has already been added to arch/x86/include/asm/mshyperv.h.

The declaration of hv_isolation_type_snp() occurs both places, but I
think that's some sloppiness from the past that could be fixed.  In fact,
hv_isolation_type_snp() occurs *twice* in include/asm-generic/mshyperv.h.

> 
>  /* Helper functions that provide a consistent pattern for checking Hyper-V hypercall
> status. */
>  static inline int hv_result(u64 status)
> --
> 2.25.1