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
I'm happy to approve such a version of this patch. Also, if you want to limit yourself to the map behavior and not unmap, you could also place this in the description of mshv_map_user_memory(). I would happily approve such a patch as well. Overall, I think your comment is very useful and points out things that are easy to miss while reading, modifying or reviewing this code in the future. I also believe that this information is better as a comment here than a commit message as has been suggested elsewhere in this thread. Thanks, Anirudh.
