Message-ID:
 <SN6PR02MB41575CA65B0A07C935F85665D49BA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 3 Feb 2026 18:35:40 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
CC: "mhkelley58@...il.com" <mhkelley58@...il.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>, "longli@...rosoft.com" <longli@...rosoft.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] mshv: Add comment about huge page mappings in guest
 physical address space

From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com> Sent: Monday, February 2, 2026 10:56 AM
> 
> On Mon, Feb 02, 2026 at 06:26:42PM +0000, Michael Kelley wrote:
> > From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com> Sent: Monday, February 2, 2026 9:18 AM
> > >
> > > On Mon, Feb 02, 2026 at 08:51:01AM -0800, mhkelley58@...il.com wrote:
> > > > From: Michael Kelley <mhklinux@...look.com>
> > > >
> > > > Huge page mappings in the guest physical address space depend on having
> > > > matching alignment of the userspace address in the parent partition and
> > > > of the guest physical address. Add a comment that captures this
> > > > information. See the link to the mailing list thread.
> > > >
> > > > No code or functional change.
> > > >
> > > > Link: https://lore.kernel.org/linux-hyperv/aUrC94YvscoqBzh3@skinsburskii.localdomain/T/#m0871d2cae9b297fd397ddb8459e534981307c7dc
> > > > Signed-off-by: Michael Kelley <mhklinux@...look.com>
> > > > ---
> > > >  drivers/hv/mshv_root_main.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> > > > index 681b58154d5e..bc738ff4508e 100644
> > > > --- a/drivers/hv/mshv_root_main.c
> > > > +++ b/drivers/hv/mshv_root_main.c
> > > > @@ -1389,6 +1389,20 @@ mshv_partition_ioctl_set_memory(struct mshv_partition *partition,
> > > >  	if (mem.flags & BIT(MSHV_SET_MEM_BIT_UNMAP))
> > > >  		return mshv_unmap_user_memory(partition, mem);
> > > >
> > > > +	/*
> > > > +	 * If the userspace_addr and the guest physical address (as derived
> > > > +	 * from the guest_pfn) have the same alignment modulo PMD huge page
> > > > +	 * size, the MSHV driver can map any PMD huge pages to the guest
> > > > +	 * physical address space as PMD huge pages. If the alignments do
> > > > +	 * not match, PMD huge pages must be mapped as single pages in the
> > > > +	 * guest physical address space. The MSHV driver does not enforce
> > > > +	 * that the alignments match, and it invokes the hypervisor to set
> > > > +	 * up correct functional mappings either way. See mshv_chunk_stride().
> > > > +	 * The caller of the ioctl is responsible for providing userspace_addr
> > > > +	 * and guest_pfn values with matching alignments if it wants the guest
> > > > +	 * to get the performance benefits of PMD huge page mappings of its
> > > > +	 * physical address space to real system memory.
> > > > +	 */
> > >
> > > Thanks. However, I'd suggest to reduce this commet a lot and put the
> > > details into the commit message instead. Also, why this place? Why not a
> > > part of the function description instead, for example?
> >
> > In general, I'm very much an advocate of putting a bit more detail into code
> > comments, so that someone new reading the code has a chance of figuring
> > out what's going on without having to search through the commit history
> > and read commit messages. The commit history is certainly useful for the
> > historical record, and especially how things have changed over time. But for
> > "how non-obvious things work now", I like to see that in the code comments.
> >
> 
> This approach is not well aligned with the existing kernel coding style.
> It is common to answer the "why" question in the commit message.
> Code comments should focus on "what" the code does.
> 
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> 

Which says "Instead, put the comments at the head of the function,
telling people what it does, and possibly WHY it does it." I'm good with
that approach.

> For more details, it is common to use `git blame` to learn the context
> of a change when needed.

Yep, I use that all the time for the historical record.

> 
> > As for where to put the comment, I'm flexible. I thought about placing it
> > outside the function as a "header" (which is what I think you mean by the
> > "function description"), but the function handles both "map" and "unmap"
> > operations, and this comment applies only to "map".  Hence I put it after
> > the test for whether we're doing "map" vs. "unmap".  But I wouldn't object
> > to it being placed as a function description, though the text would need to be
> > enhanced to more broadly be a function description instead of just a comment
> > about a specific aspect of "map" behavior.
> >
> 
> As for the location, since this documents the userspace API, I would
> rather place it above the function as part of the function description.
> Even though the function handles both map and unmap, unmap also deals
> with huge pages.

I'll do a version written as the function description. But the full function
description will be more extensive to cover all the "what" that this function
implements:
* input parameters, and their valid values
* map and unmap
* when pinned vs. movable vs. mmio regions are created
* what is done with huge pages in the above cases (i.e., a massaged version
   of what I've already written)
* populating and pinning of pages for pinned regions

Does that match with your expectations?

Michael

Powered by blists - more mailing lists