linux-kernel - Re: [PATCH v3 1/7] Drivers: hv: Introduce hv_hvcall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1643d6a8-7d4f-4d6e-aeab-f43963644a1f@linux.microsoft.com>
Date: Mon, 21 Apr 2025 16:27:40 -0700
From: Easwar Hariharan <eahariha@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>
Cc: eahariha@...ux.microsoft.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>, "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 4/21/2025 2:24 PM, Michael Kelley wrote:
> From: Easwar Hariharan <eahariha@...ux.microsoft.com> Sent: Monday, April 21, 2025 1:41 PM
>>>

<snip>

>>>
>>
>> This is very cool, thanks for taking the time! I think the function naming
>> could be more intuitive, e.g. hv_setup_*_args(). I'd not block it for that reason,
>> but would be super happy if you would update it. What do you think?
>>
> 
> I'm not particularly enamored with my naming scheme, but it was the
> best I could come up with. My criteria were:
> 
> * Keep the length reasonably short to not make line length problems
>    any worse
> * Distinguish the input args only, input & output args, and array versions

I think the in/inout/array scheme you have does this nicely

> * Use the standard "hv_" prefix for Hyper-V related code
> 
> Using "setup" instead of "hvcall" seems like an improvement to me, and
> it is 1 character shorter.  The "hv" prefix would be there, but they wouldn't
> refer specifically to hypercalls. I would not add "_args" on the end because
> that's another 5 characters in length. So we would have:
> 
> * hv_setup_in()
> * hv_setup_inout()
> * hv_setup_in_array()
> * hv_setup_inout_array()
> * hv_setup_in_batch_size() [??]
> 
> Or maybe, something like this, or similar, which picks up the "args" string,
> but not "setup":
> 
> * hv_hcargs_in()
> * hv_hcargs_inout()
> * hv_hcargs_in_array()
> * hv_hcargs_inout_array()
> * hv_hcargs_in_batch_size() [??]
> 
> I'm very open to any other ideas because I'm not particularly
> happy with the hv_hvcall_* approach.

Between the two presented here, I prefer option 1, with the "setup" verb because it tells you
inline what the function will do. I agree that the "args" is unnecessary because most
hypercall args are named hv_{input, output}_* and are clearly arguments to hv_do_hypercall()
and friends.

Since hv_setup*() will normally be followed shortly after by hv_do_hypercall(), I don't
see a problem with not referring specifically to hypercalls, it should be clear in context.

For hv_hvcall_in_batch_size(), I think it serves a fundamentally different function than the
other wrappers and doesn't need to follow the "setup" pattern. Instead it could be named 
hv_get_input_batch_size() for the same length and similarly tell you its purpose inline.

I am continuing to review the rest of the series, sorry for the delay, and thank you for your
patience!

Thanks,
Easwar (he/him)