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
