Re: [PATCH v5 3/5] hyperv: Enable the hypercall output page for the VTL mode

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

On 1/3/2025 11:20 AM, Stanislav Kinsburskii wrote:
> On Mon, Dec 30, 2024 at 10:09:39AM -0800, Roman Kisel wrote:
>> Due to the hypercall page not being allocated in the VTL mode,
>> the code resorts to using a part of the input page.
>>
>> Allocate the hypercall output page in the VTL mode thus enabling
>> it to use it for output and share code with dom0.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   drivers/hv/hv_common.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index c6ed3ba4bf61..c983cfd4d6c0 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -340,7 +340,7 @@ int __init hv_common_init(void)
>>   	BUG_ON(!hyperv_pcpu_input_arg);
>>   
>>   	/* Allocate the per-CPU state for output arg for root */
>> -	if (hv_root_partition) {
>> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
> 
> This check doesn't look nice.
I read that as you don't like it. Neither do I (see below), the change
enables what's needed for the rest, and poses no harm imo.

> First of all, IS_ENABLED(CONFIG_HYPERV_VTL_MODE) doesn't mean that this
> particular kernel is being booted in VTL other that VTL0. > Second, current approach is that a VTL1+ kernel is a different build 
from VTL0
> kernel and thus relying on the config option looks reasonable. However,
> this is not true in general case.
"First" and "Second" appear to be saying that the approach is good in
your opinion. What is that general case you're alluding to which is
going to be broken by adding IS_ENABLED() here, how do I repro the
possible borkage?

> 
> I'd suggest one of the following three options:
> 
> 1. Always allocate per-cpu output pages. This is wasteful for the VTL0
> guest case, but it might worth it for overall simplification.
As outlined above, the justification for the changes you're requesting
isn't clear. Yet, if no objections from others, I'd happily weed out
these if's and #ifdef's, on that we're in agreement.

> 
> 2. Don't touch this code and keep the cnage in the Underhill tree.
So, leave get_vtl() broken iiuc? Please suggest what would be the fix
you prefer more. The patch set regularizes the common case and makes
get_vtl() look as hv_get_vp_register which it is so get_vtl() can be
replaced with it once it appears in the tree.

All in all, strong disagree. I cannot seem to see how "don't touch"
and "keep" is going to work with upstreaming the VTL mode patches.

> 
> 3. Introduce a configuration option (command line or device tree or
> both) and use it instead of the kernel config option.
That looks to me as messy and complicated compared to adding
IS_ENABLED(). Why defer the decision to runtime, what are the benefits
in your opinion?

> 
> Thanks,
> Stas
> 
>>   		hyperv_pcpu_output_arg = alloc_percpu(void *);
>>   		BUG_ON(!hyperv_pcpu_output_arg);
>>   	}
>> @@ -435,7 +435,7 @@ int hv_common_cpu_init(unsigned int cpu)
>>   	void **inputarg, **outputarg;
>>   	u64 msr_vp_index;
>>   	gfp_t flags;
>> -	int pgcount = hv_root_partition ? 2 : 1;
>> +	const int pgcount = (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) ? 2 : 1;
>>   	void *mem;
>>   	int ret;
>>   
>> @@ -453,7 +453,7 @@ int hv_common_cpu_init(unsigned int cpu)
>>   		if (!mem)
>>   			return -ENOMEM;
>>   
>> -		if (hv_root_partition) {
>> +		if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>>   			outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
>>   			*outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
>>   		}
>> -- 
>> 2.34.1
>>

-- 
Thank you,
Roman