Message-ID:
 <SN6PR02MB4157F92B7DB9369F23D2F35FD45DA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 21 Jul 2025 19:30:42 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, Easwar Hariharan
	<eahariha@...ux.microsoft.com>, Nuno Das Neves
	<nunodasneves@...ux.microsoft.com>, Naman Jain <namjain@...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>, "mani@...nel.org"
	<mani@...nel.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 v4 0/7] hyperv: Introduce new way to manage hypercall args

From: Roman Kisel <romank@...ux.microsoft.com> Sent: Monday, July 21, 2025 10:15 AM
> 
> On 7/20/2025 7:19 PM, Michael Kelley wrote:
> > From: Roman Kisel <romank@...ux.microsoft.com> Sent: Friday, July 18, 2025 6:16
> PM
> 
> [...]
> 
> >
> > Thanks for any testing you can do on standalone test machines without
> > needing test clusters in Azure. It will be hard to get test coverage on
> > *every* hypercall call site that is modified by the patch set, but doing
> > basic smoke testing of running in the root partition and in VTL2 will
> > cover more than I can cover running in a VTL0 guest on my laptop or
> > in Azure. Fortunately, the changes overall in this patch set are pretty
> > straightforward, and my testing of VTL0 guests didn’t turn up any bugs.
> > I'm hoping that additional smoke testing is more about gaining
> > confidence than finding actual bugs.  (Famous last words ....)
> >
> 
> Thank you a million times for pushing the bar higher and supporting the
> code :)
> 
> >> VTL2 currently uses a limited number hypercalls that are set as enabled
> >> in the OpenVMM code (`set_allowed_hypercalls`). You could take a look
> >> and conclude if these hypercalls require any adjustments in the patches.
> >
> > My patch set already covers all the hypercall call sites that originate in
> > VTL2 code. Again, a basic smoke test should help gain confidence, or
> > show that any confidence is misplaced :-)
> >
> 
> Very nice, should be smooth sailing then :)
> 
> >> My opinion has been to have two pages (input and output ones). As the
> >> new code introduces just one page I do feel a bit apprehensive, got no
> >> hard evidence that this is a bad approach though. If we tweak the code
> >> to have 2 pages, perhaps there would be no need to run a full-blown
> >> validation, and even smoke tests will suffice?
> >
> > My view is that the 1 page vs. 2 pages is much less of a risk than just
> > some coding error in introducing the new interfaces. The 1 page vs.
> > 2 pages should only affect the batch size for rep hypercalls, and the
> > existing code already handles different batch sizes. So I'm not as
> > concerned about that risk. Wei Liu in the maintainer here, so I'll
> > certainly follow his judgment and guidance on what is needed to
> > be confident in this patch set.
> >
> 
> I agree with your risk assessment. Perhaps I am playing too much of
> a spec lawyer yet it states
> 
> 1) Input and output area may not intersect,
> 2) Either can be up to 4KiB of size.
> 
> Hence, one (be that for feature development or one-off debugging) would
> be within their right to implement a hypercall that accepts 4KiB of
> data and returns 4KiB of data. My understanding that after this patch,
> that won't work out-of-the-box, and would need some fixing in the
> kernel.
> 
> Perhaps, we could have a KConfig option to let the user choose if they
> need 2 pages instead of making the user figure out what needs to be
> fixed in the kernel?
> 

I'm not seeing the scenario where a Kconfig option is useful. Here's
my thinking:

A putative new hypercall that requires more than 4K for the sum of
the input and output would not be a rep hypercall. So the hypercall
has some large structure for the input and/or the output. Such a
hypercall would be handled one of two ways in the kernel:

(1) New code must be added in the kernel that includes a hypercall
call site to invoke this hypercall. The new code populates the large
input structure, and/or processes the large output structure in order
to accomplish whatever the Linux kernel needs done that the
hypercall helps with.

(2) The hypercall is invoked for the root partition or VTL2 via one
of the existing ioctl's that allow user space in the root partition or
VTL2 to invoke arbitrary hypercalls and get the results.

For (1), new code is being added to the kernel. So changing the
#define HV_HVCALL_ARG_PAGES from "1" to "2" is just part
of the code changes needed for the new hypercall.

For (2), the ioctls, such as mshv_ioctl_passthru_hvcall(), don't
use the pre-allocated hyperv_pcpu_input/output_arg. They do
a separate allocation of a full page for input and a full page for
output because of the complexities of copying the input
and output args from/to user space. So if the putative new
hypercall is invoked via the ioctl, it will just work.

I don't see a scenario where a Kconfig option provides any
value to someone building the kernel. Linux kernel code also
generally eschews adding mechanism to support future
scenarios that might or might not happen. Sure, we design
code to be extensible, but we don't generally add options or
APIs that aren't currently needed. I've consistently gotten
feedback to add such when/if the need actually arises. I
think the #define HV_HVCALL_ARG_PAGES 1 provides the
right balance.

Michael

Powered by blists - more mailing lists