Re: [PATCH v3 1/7] Drivers: hv: Introduce hv_hvcall_*() functions for hypercall arguments

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

On 8/21/2025 7:16 PM, Michael Kelley wrote:
> From: Mukesh R <mrathor@linux.microsoft.com> Sent: Thursday, August 21, 2025 2:16 PM
>>
>> On 8/21/25 13:49, Mukesh R wrote:
>>> On 8/21/25 12:24, Michael Kelley wrote:
>>>> From: Mukesh R <mrathor@linux.microsoft.com> Sent: Wednesday, August 20, 2025 7:58 PM
>>>>>
>>>>> On 8/20/25 17:31, Mukesh R wrote:
>>>>>> On 4/15/25 11:07, mhkelley58@gmail.com wrote:
>>>>>>> From: Michael Kelley <mhklinux@outlook.com>
>>>>>>>
>>>>>>>
>>> <snip>
>>>>>>
>>>>>>
>>>>>> IMHO, this is unnecessary change that just obfuscates code. With status quo
>>>>>> one has the advantage of seeing what exactly is going on, one can use the
>>>>>> args any which way, change batch size any which way, and is thus flexible.
>>>>
>>>> I started this patch set in response to some errors in open coding the
>>>> use of hyperv_pcpu_input/output_arg, to see if helper functions could
>>>> regularize the usage and reduce the likelihood of future errors. Balancing
>>>> the pluses and minuses of the result, in my view the helper functions are
>>>> an improvement, though not overwhelmingly so. Others may see the
>>>> tradeoffs differently, and as such I would not go to the mat in arguing the
>>>> patches must be taken. But if we don't take them, we need to go back and
>>>> clean up minor errors and inconsistencies in the open coding at some
>>>> existing hypercall call sites.
>>>
>>> Yes, definitely. Assuming Nuno knows what issues you are referring to,
>>> I'll work with him to get them addressed asap. Thanks for noticing them.
>>> If Nuno is not aware, I'll ping you for more info.
>>
>> Talked to Nuno, he's not aware of anything pending or details. So if you
>> can kindly list them out here, I will make sure it gets addressed right
>> away.
>>
> 
> I didn't catalog the issues as I came across them when doing this patch
> set. :-(   I don't think any are bugs that could break things now. They were
> things like not ensuring that all hypercall input fields are initialized to zero,
> duplicate initialization to zero, and unnecessary initialization of hypercall
> output memory. In general, how the hypercall args are set up is inconsistent
> across different hypercall call sites, and that inconsistency can lead to errors,
> which is what I was trying to address.
> 
> But I can go back and come up with a list if that's where we're headed.

Hi Michael and Mukesh,

Just a suggestion, how about a simpler set of macros that doesn't really change
the existing paradigm, but can be used to improve the consistency across the
various hypercall sites.

e.g. for getting and zeroing the input page:

#define hv_get_input_ptr(in_ptr) \
({ \
        static_assert(sizeof(*in_ptr) <= HV_HYP_PAGE_SIZE); \
        void *__arg = *this_cpu_ptr(hyperv_pcpu_input_arg); \
        memset(__arg, 0, sizeof(*in_ptr)); \
        __arg; \
})

(And something similar for the output arg which doesn't need memset())

And for batch size, it can be very simple, although there's both the case
of argument + array elements, and just array elements:

#define hv_arg_get_batch_size(arg_ptr, element_ptr) \
        ((HV_HYP_PAGE_SIZE - sizeof(*arg_ptr)) / sizeof(*element_ptr))

#define hv_get_batch_size(element_ptr) (HV_HYP_PAGE_SIZE / sizeof(*element_ptr))

Usage:

struct hv_input_map_gpa_pages *input_page = hv_get_input_ptr(input_page);
int batch_size = hv_arg_get_batch_size(input_page, &input_page->source_gpa_page_list[0]);



Nuno

> 
> Michael