Message-ID: <33062e8a-27dc-a623-6b12-c92713298369@linux.microsoft.com>
Date: Mon, 25 Aug 2025 18:46:43 -0700
From: Mukesh R <mrathor@...ux.microsoft.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
 Michael Kelley <mhklinux@...look.com>, "kys@...rosoft.com"
 <kys@...rosoft.com>, "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
 "wei.liu@...nel.org" <wei.liu@...nel.org>,
 "decui@...rosoft.com" <decui@...rosoft.com>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "hpa@...or.com" <hpa@...or.com>,
 "lpieralisi@...nel.org" <lpieralisi@...nel.org>, "kw@...ux.com"
 <kw@...ux.com>,
 "manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
 "robh@...nel.org" <robh@...nel.org>,
 "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "arnd@...db.de" <arnd@...db.de>
Cc: "x86@...nel.org" <x86@...nel.org>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
 "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: [PATCH v3 1/7] Drivers: hv: Introduce hv_hvcall_*() functions for
 hypercall arguments

On 8/25/25 17:13, Nuno Das Neves wrote:
> On 8/21/2025 7:16 PM, Michael Kelley wrote:
>> From: Mukesh R <mrathor@...ux.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@...ux.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@...il.com wrote:
>>>>>>>> From: Michael Kelley <mhklinux@...look.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; \
> })

Ugh! What is the problem that we are trying to solve? The code is
simple and clear today, tells the reader exactly what is being used and
for how many bytes etc. What if the input to hyp is a list of pfns, maybe
a void *? And if we want to do any complex stuff, we'll just keep adding
parameters to the macro. IMO, complex macros just obfuscate code! I think
this is just not worth it right now. We'll look ways to enhance hcall params
in future, perhaps we can address it then if there are any real issues.

Thanks,
-Mukesh


> (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

Powered by blists - more mailing lists