Re: [PATCH 09/17] KVM: arm64: Move VMA-related information to kvm_s2_fault_vma_info

[Marc Zyngier <maz@kernel.org>]

On Wed, 18 Mar 2026 16:14:19 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Joey,
> 
> First, thanks for the reviews and the comments on my series. You're
> right about my changes wrongly editing "page". I wanted it to be as
> mechanical as possible to make it easy to review, but it ended up
> being too mechanical.
> 
> <snip>
> 
> > > -     /* Mark the fault->page dirty only if the fault is handled successfully */
> > > -     if (fault->writable && !ret)
> > > -             mark_page_dirty_in_slot(kvm, s2fd->memslot, get_canonical_gfn(s2fd, fault));
> > > +     /* Mark the page dirty only if the fault is handled successfully */
> > > +     if (fault->writable && !ret) {
> > > +             phys_addr_t ipa = gfn_to_gpa(get_canonical_gfn(s2fd, s2vi));
> > > +             ipa &= ~(mapping_size - 1);
> > > +             mark_page_dirty_in_slot(kvm, s2fd->memslot, gpa_to_gfn(ipa));
> >
> > I don't understand this change, why do we need to mask stuff now?
> 
> Let me see if _I_ understand it (Marc, please correct me if I'm wrong).
> 
> Before this patch, fault->gfn and fault->vma_pagesize were mutable,
> and transparent_hugepage_adjust() modified both directly. In addition
> to this being confusing (which gfn is this: the host /canonical or the
> nested one?), it made it more difficult to separate the logic.
> 
> So, to mark a dirty page, it did this:
> -             mark_page_dirty_in_slot(kvm, s2fd->memslot,
> get_canonical_gfn(s2fd, fault));
> 
> which relied on the old struct fault to calculate the canonical gfn
> using the (magically) THP adjusted fault->vma_pagesize.
> 
> Now that fault (or s2vi, its successor in this case) isn't mutable, we
> need to get the canonical gfn using the host mapping size.

It's exactly that, and it is slightly clearer if you look at how
mapping_size is updated:

	mapping_size = transparent_hugepage_adjust(kvm, s2fd->memslot,
						   s2fd->hva, &fault->pfn,
						   &gfn);

The faulting IPA is represented by 'gfn', and gets correctly updated
by the helper. But that doesn't adjust the 'canonical' IPA, which is
used for any memslot related update.

So if we need to call into mark_page_dirty_in_slot(), we really need
to pick the base of the region we are actually marking dirty, hence
the masking of the bottom bits.

Does this make sense? This is one of the area where the constification
results in slightly more complicated code, as we can't update things
in place anymore.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.