Re: [PATCH v2 3/6] drivers/hv: arch-neutral implementation of get_vtl()

[Roman Kisel <romank@linux.microsoft.com>]

On 5/15/2024 6:38 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> To run in the VTL mode, Hyper-V drivers have to know what
>> VTL the system boots in, and the arm64/hyperv code does not
>> have the means to compute that.
>>
>> Refactor the code to hoist the function that detects VTL,
>> make it arch-neutral to be able to employ it to get the VTL
>> on arm64.
> 
> Nit:  In patches that just refactor and move some code around,
> patch authors will often include a statement like "No functional
> changes" (or the slightly more doubtful "No functional changes
> intended") as a separate line at the end of the commit message.
> It's just something the reader/reviewer can cross-check against.
> 
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/x86/hyperv/hv_init.c          | 34 -----------------------
>>   arch/x86/include/asm/hyperv-tlfs.h |  7 -----
>>   drivers/hv/hv_common.c             | 43 ++++++++++++++++++++++++++++++
>>   include/asm-generic/hyperv-tlfs.h  |  7 +++++
>>   include/asm-generic/mshyperv.h     |  6 +++++
>>   5 files changed, 56 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 17a71e92a343..c350fa05ee59 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -413,40 +413,6 @@ 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;
>> -	struct hv_get_vp_registers_input *input;
>> -	struct hv_get_vp_registers_output *output;
>> -	unsigned long flags;
>> -	u64 ret;
>> -
>> -	local_irq_save(flags);
>> -	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> -	output = (struct hv_get_vp_registers_output *)input;
>> -
>> -	memset(input, 0, struct_size(input, element, 1));
>> -	input->header.partitionid = HV_PARTITION_ID_SELF;
>> -	input->header.vpindex = HV_VP_INDEX_SELF;
>> -	input->header.inputvtl = 0;
>> -	input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS;
>> -
>> -	ret = hv_do_hypercall(control, input, output);
>> -	if (hv_result_success(ret)) {
>> -		ret = output->as64.low & HV_X64_VTL_MASK;
>> -	} else {
>> -		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
>>    * hypervisor has been detected.
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index 3787d26810c1..9ee68eb8e6ff 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -309,13 +309,6 @@ enum hv_isolation_type {
>>   #define HV_MSR_STIMER0_CONFIG	(HV_X64_MSR_STIMER0_CONFIG)
>>   #define HV_MSR_STIMER0_COUNT	(HV_X64_MSR_STIMER0_COUNT)
>>
>> -/*
>> - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and
>> - * there is not associated MSR address.
>> - */
>> -#define	HV_X64_REGISTER_VSM_VP_STATUS	0x000D0003
>> -#define	HV_X64_VTL_MASK			GENMASK(3, 0)
>> -
>>   /* Hyper-V memory host visibility */
>>   enum hv_mem_host_visibility {
>>   	VMBUS_PAGE_NOT_VISIBLE		= 0,
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index dde3f9b6871a..d4cf477a4d0c 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64
>> param2)
>>   	return HV_STATUS_INVALID_PARAMETER;
>>   }
>>   EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
>> +
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void)
>> +{
>> +	u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>> +	struct hv_get_vp_registers_input *input;
>> +	struct hv_get_vp_registers_output *output;
>> +	unsigned long flags;
>> +	u64 ret;
>> +
>> +	local_irq_save(flags);
>> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> +	output = (struct hv_get_vp_registers_output *)input;
> 
> I gave some bad advice on the original version of the above code.  Rather
> than allocating a separate hypercall output area, I had suggested just setting
> the hypercall output to be the same as the input area, which is done
> above.  While the overlap doesn't cause any problems for a hypercall returning
> the value of single register, the TLFS says to never set up such an overlap.
> 
> Since this code is being moved anyway, there's an opportunity to fix
> this by setting output to "input" + "struct_size(input, element, 1)".  Or
> put the output at the start of the second half of the page (note that the
> size is HY_HYP_PAGE_SIZE, not PAGE_SIZE).  The input and output for 1
> register are relatively small and both easily fit with either approach. They
> just shouldn't overlap.
> 
> Some might argue this tweak should be made in a separate patch, but
> in my judgment, it's OK to do it here if you note it in the commit
> message.  Your call.  Of course, if you make this change, my previous
> comment about "No functional changes" no longer applies. :-)
I'll update the code to adhere to the TLFS requirements, thank you :)

> 
> Michael
> 
>> +
>> +	memset(input, 0, struct_size(input, element, 1));
>> +	input->header.partitionid = HV_PARTITION_ID_SELF;
>> +	input->header.vpindex = HV_VP_INDEX_SELF;
>> +	input->header.inputvtl = 0;
>> +	input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS;
>> +
>> +	ret = hv_do_hypercall(control, input, output);
>> +	if (hv_result_success(ret)) {
>> +		ret = output->as64.low & HV_VTL_MASK;
>> +	} else {
>> +		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>> +
>> +		/*
>> +		 * This is a dead end, something fundamental is broken.
>> +		 *
>> +		 * There is no sensible way of continuing as the Hyper-V drivers
>> +		 * transitively depend via the vmbus driver on knowing which VTL
>> +		 * they run in to establish communication with the host. The kernel
>> +		 * is going to be worse off if continued booting than a panicked one,
>> +		 * just hung and stuck, producing second-order failures, with neither
>> +		 * a way to recover nor to provide expected services.
>> +		 */
>> +		BUG();
>> +	}
>> +
>> +	local_irq_restore(flags);
>> +	return ret;
>> +}
>> +#endif
>> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
>> index 87e3d49a4e29..682bcda3124f 100644
>> --- a/include/asm-generic/hyperv-tlfs.h
>> +++ b/include/asm-generic/hyperv-tlfs.h
>> @@ -75,6 +75,13 @@
>>   /* AccessTscInvariantControls privilege */
>>   #define HV_ACCESS_TSC_INVARIANT			BIT(15)
>>
>> +/*
>> + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS
>> + * hvcall, and there is no an associated MSR on x86.
>> + */
>> +#define	HV_REGISTER_VSM_VP_STATUS	0x000D0003
>> +#define	HV_VTL_MASK			GENMASK(3, 0)
>> +
>>   /*
>>    * Group B features.
>>    */
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 99935779682d..ea434186d765 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -301,4 +301,10 @@ static inline enum hv_isolation_type
>> hv_get_isolation_type(void)
>>   }
>>   #endif /* CONFIG_HYPERV */
>>
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void);
>> +#else
>> +static inline u8 get_vtl(void) { return 0; }
>> +#endif
>> +
>>   #endif
>> --
>> 2.45.0
>>

-- 
Thank you,
Roman