Message-ID: <aYKY0JmcnadPqwXK@skinsburskii.localdomain>
Date: Tue, 3 Feb 2026 16:54:40 -0800
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.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

On Tue, Feb 03, 2026 at 06:35:40PM +0000, Michael Kelley wrote:
> 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?

I’d rather suggest something simpler for the function header:

* What regions are created
* What pages sizes are supported

I.e. describe what the function does, not the rationale or the
architecture behind it.

For example, something like this (suggested by AI, feel free to rewrite
completly):

 * Depending on the request, the region is created as pinned RAM, movable RAM,
 * or MMIO. PMD-sized huge page mappings are supported when the userspace
 * address and guest physical address (guest_pfn << PAGE_SHIFT) have matching
 * alignment modulo PMD_SIZE; otherwise the mapping is established using base
 * pages.

The rationale and architecture can be put into the commit message.

Thanks,
Stanislav

> Michael

Powered by blists - more mailing lists