Message-ID:
 <SN6PR02MB4157FEE08571B84B6CEBFC92D4B82@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 21 Apr 2025 21:24:58 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Easwar Hariharan <eahariha@...ux.microsoft.com>
CC: "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

From: Easwar Hariharan <eahariha@...ux.microsoft.com> Sent: Monday, April 21, 2025 1:41 PM
> >
> > Current code allocates the "hyperv_pcpu_input_arg", and in
> > some configurations, the "hyperv_pcpu_output_arg". Each is a 4 KiB
> > page of memory allocated per-vCPU. A hypercall call site disables
> > interrupts, then uses this memory to set up the input parameters for
> > the hypercall, read the output results after hypercall execution, and
> > re-enable interrupts. The open coding of these steps leads to
> > inconsistencies, and in some cases, violation of the generic
> > requirements for the hypercall input and output as described in the
> > Hyper-V Top Level Functional Spec (TLFS)[1].
> >
> > To reduce these kinds of problems, introduce a family of inline
> > functions to replace the open coding. The functions provide a new way
> > to manage the use of this per-vCPU memory that is usually the input and
> > output arguments to Hyper-V hypercalls. The functions encapsulate
> > key aspects of the usage and ensure that the TLFS requirements are
> > met (max size of 1 page each for input and output, no overlap of
> > input and output, aligned to 8 bytes, etc.). Conceptually, there
> > is no longer a difference between the "per-vCPU input page" and
> > "per-vCPU output page". Only a single per-vCPU page is allocated, and
> > it provides both hypercall input and output memory. All current
> > hypercalls can fit their input and output within that single page,
> > though the new code allows easy changing to two pages should a future
> > hypercall require a full page for each of the input and output.
> >
> > The new functions always zero the fixed-size portion of the hypercall
> > input area so that uninitialized memory is not inadvertently passed
> > to the hypercall. Current open-coded hypercall call sites are
> > inconsistent on this point, and use of the new functions addresses
> > that inconsistency. The output area is not zero'ed by the new code
> > as it is Hyper-V's responsibility to provide legal output.
> >
> > When the input or output (or both) contain an array, the new functions
> > calculate and return how many array entries fit within the per-vCPU
> > memory page, which is effectively the "batch size" for the hypercall
> > processing multiple entries. This batch size can then be used in the
> > hypercall control word to specify the repetition count. This
> > calculation of the batch size replaces current open coding of the
> > batch size, which is prone to errors. Note that the array portion of
> > the input area is *not* zero'ed. The arrays are almost always 64-bit
> > GPAs or something similar, and zero'ing that much memory seems
> > wasteful at runtime when it will all be overwritten. The hypercall
> > call site is responsible for ensuring that no part of the array is
> > left uninitialized (just as with current code).
> >
> > The new functions are realized as a single inline function that
> > handles the most complex case, which is a hypercall with input
> > and output, both of which contain arrays. Simpler cases are mapped to
> > this most complex case with #define wrappers that provide zero or NULL
> > for some arguments. Several of the arguments to this new function
> > must be compile-time constants generated by "sizeof()"
> > expressions. As such, most of the code in the new function can be
> > evaluated by the compiler, with the result that the code paths are
> > no longer than with the current open coding. The one exception is
> > new code generated to zero the fixed-size portion of the input area
> > in cases where it is not currently done.
> >
> > [1]
> https://learn.microsoft/.
> com%2Fen-us%2Fvirtualization%2Fhyper-v-on-
> windows%2Ftlfs%2Ftlfs&data=05%7C02%7C%7Ceefaa97bb91c4d5c9dfb08dd8114da
> b3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638808648755643707%
> 7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCI
> sIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=1S2
> 9jKMjSZgciblHrJzH1rVbPuIORh%2FrU1vFcviBBHE%3D&reserved=0
> >
> > Signed-off-by: Michael Kelley <mhklinux@...look.com>
> > Reviewed-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
> > ---
> >
> > Notes:
> >     Changes in v3:
> >     * Added wrapper #define hv_hvcall_in_batch_size() to get the batch size
> >       without setting up hypercall input/output parameters. This call can be
> >       used when the batch size is needed for validation checks or memory
> >       allocations prior to disabling interrupts.
> >
> >     Changes in v2:
> >     * Added comment that hv_hvcall_inout_array() should always be called with
> >       interrupts disabled because it is returning pointers to per-cpu memory
> >       [Nuno Das Neves]
> >
> >  include/asm-generic/mshyperv.h | 106 +++++++++++++++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >
>
> 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
* 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.

Michael

Powered by blists - more mailing lists