On Tue, Feb 03, 2026 at 06:35:40PM +0000, Michael Kelley wrote: > From: Stanislav Kinsburskii <[email protected]> Sent: Monday, > February 2, 2026 10:56 AM > > > > On Mon, Feb 02, 2026 at 06:26:42PM +0000, Michael Kelley wrote: > > > From: Stanislav Kinsburskii <[email protected]> Sent: > > > Monday, February 2, 2026 9:18 AM > > > > > > > > On Mon, Feb 02, 2026 at 08:51:01AM -0800, [email protected] wrote: > > > > > From: Michael Kelley <[email protected]> > > > > > > > > > > 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/[email protected]/T/#m0871d2cae9b297fd397ddb8459e534981307c7dc > > > > > Signed-off-by: Michael Kelley <[email protected]> > > > > > --- > > > > > 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
