Re: [PATCH v2 4/6] Drivers: hv: Use hv_hvcall_*() to set up hypercall arguments

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 3/12/2025 11:19 PM, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
> 
> Update hypercall call sites to use the new hv_hvcall_*() functions
> to set up hypercall arguments. Since these functions zero the
> fixed portion of input memory, remove now redundant zero'ing of
> input fields.
> 
> hv_post_message() requires additional updates. The payload area is
> treated as an array to avoid wasting cycles on zero'ing it and
> then overwriting with memcpy(). To allow treatment as an array,
> the corresponding payload[] field is updated to have zero size.
> 
I'd prefer to leave the payload field as a fixed-sized array.
Changing it to a flexible array makes it look like that input is
for a variable-sized or rep hypercall, and it makes the surrounding
code in hv_post_message() more complex and inscrutable as a result.

I suggest leaving hv_post_message() alone, except for changing
hyperv_pcpu_input_arg -> hyperv_pcpu_arg, and perhaps a comment
explaining why hv_hvcall_input() isn't used there.

> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
>  drivers/hv/hv.c           | 9 ++++++---
>  drivers/hv/hv_balloon.c   | 4 ++--
>  drivers/hv/hv_common.c    | 2 +-
>  drivers/hv/hv_proc.c      | 8 ++++----
>  drivers/hv/hyperv_vmbus.h | 2 +-
>  5 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index a38f84548bc2..e2dcbc816fc5 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -66,7 +66,8 @@ int hv_post_message(union hv_connection_id connection_id,
>  	if (hv_isolation_type_tdx() && ms_hyperv.paravisor_present)
>  		aligned_msg = this_cpu_ptr(hv_context.cpu_context)->post_msg_page;
>  	else
> -		aligned_msg = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +		hv_hvcall_in_array(&aligned_msg, sizeof(*aligned_msg),
> +				   sizeof(aligned_msg->payload[0]));
>  
>  	aligned_msg->connectionid = connection_id;
>  	aligned_msg->reserved = 0;
> @@ -80,8 +81,10 @@ int hv_post_message(union hv_connection_id connection_id,
>  						  virt_to_phys(aligned_msg), 0);
>  		else if (hv_isolation_type_snp())
>  			status = hv_ghcb_hypercall(HVCALL_POST_MESSAGE,
> -						   aligned_msg, NULL,
> -						   sizeof(*aligned_msg));
> +						   aligned_msg,
> +						   NULL,
> +						   struct_size(aligned_msg, payload,
> +							       HV_MESSAGE_PAYLOAD_QWORD_COUNT));

See my comment above, I'd prefer to leave this function mostly
alone to maintain readability.

>  		else
>  			status = HV_STATUS_INVALID_PARAMETER;
>  	} else {
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index fec2f18679e3..2def8b8794ee 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1582,14 +1582,14 @@ static int hv_free_page_report(struct page_reporting_dev_info *pr_dev_info,
>  	WARN_ON_ONCE(nents > HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES);
>  	WARN_ON_ONCE(sgl->length < (HV_HYP_PAGE_SIZE << page_reporting_order));
>  	local_irq_save(flags);
> -	hint = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> +	hv_hvcall_in_array(&hint, sizeof(*hint), sizeof(hint->ranges[0]));

We should ensure the returned batch size is large enough for
"nents".

>  	if (!hint) {
>  		local_irq_restore(flags);
>  		return -ENOSPC;
>  	}
>  
>  	hint->heat_type = HV_EXTMEM_HEAT_HINT_COLD_DISCARD;
> -	hint->reserved = 0;
>  	for_each_sg(sgl, sg, nents, i) {
>  		union hv_gpa_page_range *range;
>  
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 9804adb4cc56..a6b1cdfbc8d4 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -293,7 +293,7 @@ void __init hv_get_partition_id(void)
>  	u64 status, pt_id;
>  
>  	local_irq_save(flags);
> -	output = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	hv_hvcall_inout(NULL, 0, &output, sizeof(*output));
>  	status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, &output);
>  	pt_id = output->partition_id;
>  	local_irq_restore(flags);
> diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c
> index 2fae18e4f7d2..5c580ee1c23f 100644
> --- a/drivers/hv/hv_proc.c
> +++ b/drivers/hv/hv_proc.c
> @@ -73,7 +73,8 @@ int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>  
>  	local_irq_save(flags);
>  
> -	input_page = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	hv_hvcall_in_array(&input_page, sizeof(*input_page),
> +			   sizeof(input_page->gpa_page_list[0]));

We should ensure the returned batch size is large enough.

>  
>  	input_page->partition_id = partition_id;
>  
> @@ -124,9 +125,8 @@ int hv_call_add_logical_proc(int node, u32 lp_index, u32 apic_id)
>  	do {
>  		local_irq_save(flags);
>  
> -		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>  		/* We don't do anything with the output right now */
> -		output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +		hv_hvcall_inout(&input, sizeof(*input), &output, sizeof(*output));
>  
>  		input->lp_index = lp_index;
>  		input->apic_id = apic_id;
> @@ -167,7 +167,7 @@ int hv_call_create_vp(int node, u64 partition_id, u32 vp_index, u32 flags)
>  	do {
>  		local_irq_save(irq_flags);
>  
> -		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +		hv_hvcall_in(&input, sizeof(*input));
>  
>  		input->partition_id = partition_id;
>  		input->vp_index = vp_index;
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 29780f3a7478..44b5e8330d9d 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -101,7 +101,7 @@ struct hv_input_post_message {
>  	u32 reserved;
>  	u32 message_type;
>  	u32 payload_size;
> -	u64 payload[HV_MESSAGE_PAYLOAD_QWORD_COUNT];
> +	u64 payload[];

See my comment above, I'd prefer to keep this how it is.

>  };
>  
>  

Thanks
Nuno