Re: [PATCH v2 01/13] KVM: arm64: Donate MMIO to the hypervisor

[Mostafa Saleh <smostafa@google.com>]

Hi Fuad,

On Wed, Sep 09, 2026 at 04:39:03PM +0100, Fuad Tabba wrote:
> Hi Mostaf, Seb,
> 
> On Fri, 7 Aug 2026 at 17:43, Sebastian Ene <sebastianene@google.com> wrote:
> >
> > From: Mostafa Saleh <smostafa@google.com>
> ...
> > +int __pkvm_host_donate_hyp_mmio(phys_addr_t addr, size_t size)
> > +{
> ...
> > +       /*
> > +        * We set HYP as the owner of the MMIO pages in the host stage-2, for:
> > +        * - host aborts: host_stage2_adjust_range() would fail for invalid non zero PTEs.
> > +        * - recycle under memory pressure: host_stage2_unmap_dev_all() would call
> > +        *   kvm_pgtable_stage2_unmap() which will not clear non zero invalid ptes (counted).
> > +        * - other MMIO donation: Would fail as we check that the PTE is valid or empty.
> > +        */
> > +       ret = host_stage2_try(kvm_pgtable_stage2_annotate, &host_mmu.pgt,
> > +                             addr, size, &host_s2_pool,
> > +                             KVM_HOST_INVALID_PTE_TYPE_DONATION,
> > +                             FIELD_PREP(KVM_HOST_DONATION_PTE_OWNER_MASK, PKVM_ID_HYP));
> 
> Before this patch the hyp linear map held nothing but memory, and this
> adds the first device mapping to it. That places it outside
> `fix_host_ownership()`, whose scope is the memblock list, so nothing
> at init verifies the ownership. It's correct as written: the donation
> writes the host stage-2 annotation itself.
> 
> This has already gone wrong once with the hyp stacks. The fix [1] adds
> `pkvm_check_host_ownership()` over the private range, which fails init
> on a leaf that isn't hyp-owned. I'd move this mapping there rather
> than leave it outside any ownership walk.
> 

This patch is not the latest, the latest one uses the private range:
https://lore.kernel.org/all/20260715115906.2664882-3-smostafa@google.com/

> ...
> 
> > +int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size)
> > +{
> ...
> > +               virt = __hyp_va(addr + offset);
> > +               if (kvm_pgtable_hyp_unmap(&pkvm_pgtable, (u64)virt, PAGE_SIZE) != PAGE_SIZE)
> > +                       goto err_with_unmap;
> 
> Sashiko is right, even though this is benign for now. `ret` is still 0
> from the `kvm_pgtable_get_leaf()` above, so a short unmap runs the
> rollback and returns success. Could it set an error before the goto?
> 

True, also fixed in the latest version.

> ...
> 
> > @@ -1161,13 +1161,12 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >         kvm_pte_t *childp = NULL;
> >         bool need_flush = false;
> >
> > -       if (!kvm_pte_valid(ctx->old)) {
> > -               if (stage2_pte_is_counted(ctx->old)) {
> > -                       kvm_clear_pte(ctx->ptep);
> > -                       mm_ops->put_page(ctx->ptep);
> > -               }
> > +       /*
> > +        * That also ignores stage2_pte_is_counted() instead of clearing
> > +        * the PTE as the MMIO can be owned by the hypervisor.
> > +        */
> > +       if (!kvm_pte_valid(ctx->old))
> >                 return 0;
> > -       }
> 
> This changes `kvm_pgtable_stage2_unmap()` for every caller, for a
> reason specific to `host_stage2_unmap_dev_all()`. The others are
> `__unmap_stage2_range()` and the two guest unmaps in `mem_protect.c`;
> as far as I can tell none is affected today, but
> `stage2_pte_is_counted()`'s comment still describes the old behaviour.

I digged more into this check and I am not sure why is it here in
the first place, as invalid counted PTEs are pKVM specific anyway.
And there is no place were pKVM clear them with an unmap call.
So this is more solid IMHO to avoid accidentally losing annotations.

> 
> What's changing isn't what the walk does, it's what counts as a
> counted entry, and that isn't the same question for the host stage-2
> as for a guest's. Could that be stated at the call site instead?

My understanding is that the old check never hits in guest (invalid
and counted)

> 
> I couldn't work out what "That" refers to in the new comment.

Sorry that was unclear, I mean't "the check", as it only checks
for validity now, it will ignore stage2_pte_is_counted() PTEs,
I just wanted to make that clear.


Thanks,
Mostafa

> 
> Cheers,
> /fuad
> 
> [1] https://lore.kernel.org/all/20260908110713.1540304-1-fuad.tabba@linux.dev/
> 
> >
> >         if (kvm_pte_table(ctx->old, ctx->level)) {
> >                 childp = kvm_pte_follow(ctx->old, mm_ops);
> > --
> > 2.55.0.654.g21b8a5bc05-goog
> >