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

[Fuad Tabba]

Hi Mostaf, Seb,

On Fri, 7 Aug 2026 at 17:43, Sebastian Ene <sebastianene@xxxxxxxxxx> wrote:
>
> From: Mostafa Saleh <smostafa@xxxxxxxxxx>
...
> +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.

...

> +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?

...

> @@ -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.

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?

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

Cheers,
/fuad

[1] https://lore.kernel.org/all/20260908110713.1540304-1-fuad.tabba@xxxxxxxxx/

>
>         if (kvm_pte_table(ctx->old, ctx->level)) {
>                 childp = kvm_pte_follow(ctx->old, mm_ops);
> --
> 2.55.0.654.g21b8a5bc05-goog
>