Re: [PATCH v6 02/25] KVM: arm64: Donate MMIO to the hypervisor

[Mostafa Saleh <smostafa@google.com>]

On Fri, Jul 10, 2026 at 06:08:47PM +0100, Vincent Donnefort wrote:
> On Fri, May 01, 2026 at 11:19:04AM +0000, Mostafa Saleh wrote:
> > Add a function to donate MMIO to the hypervisor so IOMMU hypervisor
> > drivers can protect and access the MMIO of IOMMUs.
> > 
> > As donating MMIO is very rare, and we don’t need to encode the full
> > state, it’s reasonable to have a separate function to do this.
> > It will init the host s2 page table with an invalid leaf with the owner ID
> > to prevent the host from mapping the page on faults.
> > 
> > Also, prevent kvm_pgtable_stage2_unmap() from removing owner ID from
> > stage-2 PTEs, as this can be triggered from recycle logic under memory
> > pressure. There is no code relying on this, as all ownership changes is
> > done via kvm_pgtable_stage2_set_owner()
> > 
> > For the error path in IOMMU drivers, add a function to donate MMIO
> > back from hyp to host. However, that leaks the hypervisor virtual
> > address range which should be acceptable as this is quite rare and
> > it matches the behaviour of fix_map/block.
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  arch/arm64/kvm/hyp/include/nvhe/mem_protect.h |   2 +
> >  arch/arm64/kvm/hyp/nvhe/mem_protect.c         | 119 +++++++++++++++++-
> >  arch/arm64/kvm/hyp/pgtable.c                  |   9 +-
> >  3 files changed, 121 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > index 3cbfae0e3dda..ff440204d2c7 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > @@ -36,6 +36,8 @@ int __pkvm_guest_share_host(struct pkvm_hyp_vcpu *vcpu, u64 gfn);
> >  int __pkvm_guest_unshare_host(struct pkvm_hyp_vcpu *vcpu, u64 gfn);
> >  int __pkvm_host_unshare_hyp(u64 pfn);
> >  int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages);
> > +int __pkvm_host_donate_hyp_mmio(phys_addr_t addr, size_t size, unsigned long *haddr);
> > +int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size);
> 
> Can we have u64 pfn, u64 nr_pages here to match all the other functions?
> 

I see the convention is that phys_addr_t is used for physical addresses
as (addr_is_memory(), host_stage2_idmap_locked(()...) while u64 is used
for pfn.

> Also, as this is used by a "module" perhaps a sort of kerneldoc would be nice,
> especially to explain this isn't mapped into the hyp idmap range.

Sure I can add a comment.

> 
> >  int __pkvm_hyp_donate_host(u64 pfn, u64 nr_pages);
> >  int __pkvm_host_share_ffa(u64 pfn, u64 nr_pages);
> >  int __pkvm_host_unshare_ffa(u64 pfn, u64 nr_pages);
> > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > index 28a471d1927c..2fb20a63a417 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> > @@ -353,6 +353,38 @@ int __pkvm_prot_finalize(void)
> >  	return 0;
> >  }
> >  
> > +/* Unmap MMIO region while skipping donated PTEs. */
> > +static int host_stage2_unmap_mmio_region(u64 start, u64 size)
> > +{
> > +	struct kvm_pgtable *pgt = &host_mmu.pgt;
> > +	u64 unmap_start = start;
> > +	u64 addr = start;
> > +	kvm_pte_t pte;
> > +	int ret = 0;
> > +	u8 level;
> > +
> > +	while (addr < start + size) {
> > +		ret = kvm_pgtable_get_leaf(pgt, addr, &pte, &level);
> > +		if (ret)
> > +			return ret;
> > +		if (!kvm_pte_valid(pte) && pte != 0) {
> > +			if (addr > unmap_start) {
> > +				ret = kvm_pgtable_stage2_unmap(pgt, unmap_start,
> > +							       addr - unmap_start);
> > +				if (ret)
> > +					return ret;
> > +			}
> > +			addr += kvm_granule_size(level);
> > +			unmap_start = addr;
> > +		} else {
> > +			addr += kvm_granule_size(level);
> > +		}
> > +	}
> > +	if (addr > unmap_start)
> > +		ret = kvm_pgtable_stage2_unmap(pgt, unmap_start, addr - unmap_start);
> 
> Ah shame, that's a lot of table walking. I guess we could:
> 
>  * create our own custom walker here but that would duplicate a lot of logic
>    from pgtable.c. So doesn't look great.

Yes, that seems too complicated.

> 
>  * Retry kvm_pgtable_stage2_unmap() as long as it return -EPERM... but then we
>    it needs to return where it has stopped?

Yes, that is not possible with its current interface.

> 
>  * Modify kvm_pgtable_stage2_unmap() to just skip the refcounted entries?
>    There's already a flag something that looks like this: KVM_PGTABLE_WALK_IGNORE_EAGAIN
> 

I thought about this, my main concern is that now any unmap will fail
silently if it hits a refcounted page. Propably not too bad as that only
is used in recycling.

> I am not so convinced by the first two points but the last one is probably worth
> exploring.
>  
> > +	return ret;
> > +}
> > +
> >  static int host_stage2_unmap_dev_all(void)
> >  {
> >  	struct kvm_pgtable *pgt = &host_mmu.pgt;
> > @@ -363,11 +395,11 @@ static int host_stage2_unmap_dev_all(void)
> >  	/* Unmap all non-memory regions to recycle the pages */
> >  	for (i = 0; i < hyp_memblock_nr; i++, addr = reg->base + reg->size) {
> >  		reg = &hyp_memory[i];
> > -		ret = kvm_pgtable_stage2_unmap(pgt, addr, reg->base - addr);
> > +		ret = host_stage2_unmap_mmio_region(addr, reg->base - addr);
> >  		if (ret)
> >  			return ret;
> >  	}
> > -	return kvm_pgtable_stage2_unmap(pgt, addr, BIT(pgt->ia_bits) - addr);
> > +	return host_stage2_unmap_mmio_region(addr, BIT(pgt->ia_bits) - addr);
> >  }
> >  
> >  /*
> > @@ -1087,6 +1119,89 @@ int __pkvm_host_donate_hyp(u64 pfn, u64 nr_pages)
> >  	return ret;
> >  }
> >  
> > +int __pkvm_host_donate_hyp_mmio(phys_addr_t addr, size_t size, unsigned long *haddr)
> > +{
> > +	kvm_pte_t pte;
> > +	u64 offset;
> > +	int ret;
> > +
> > +	/* Only before de-privilege. */
> > +	if (static_branch_unlikely(&kvm_protected_mode_initialized))
> > +		return -EPERM;
> > +
> > +	if (!PAGE_ALIGNED(addr | size))
> > +		return -EINVAL;
> 
> Would be nice here to have pfn_range_is_valid() even though this is only
> used by modules? (ditto __pkvm_hyp_donate_host_mmio())

Will do.

> 
> > +
> > +	ret = __pkvm_create_private_mapping(addr, size, PAGE_HYP_DEVICE, haddr);
> > +	if (ret)
> > +		return ret;
> 
> Perhaps better to do it once it's been annotated into the host s2, so that we
> don't burn the private range in case of failure.

This really should not fail, or the system is already have bigger
problems, and that also does not allocate much space (128K), I see
that is easier to do this way than having to fix the host stage-2
if the allocation fails.

> 
> > +
> > +	host_lock_component();
> > +	for (offset = 0; offset < size; offset += PAGE_SIZE) {
> > +		if (addr_is_memory(addr + offset)) {
> > +			ret = -EINVAL;
> > +			goto unlock;
> > +		}
> > +		ret = kvm_pgtable_get_leaf(&host_mmu.pgt, addr + offset, &pte, NULL);
> > +		if (ret)
> > +			goto unlock;
> > +		if (pte && !kvm_pte_valid(pte)) {
> > +			ret = -EPERM;
> > +			goto unlock;
> > +		}
> > +	}
> > +	/*
> > +	 * 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));
> > +unlock:
> > +	host_unlock_component();
> > +	return ret;
> > +}
> > +
> > +int __pkvm_hyp_donate_host_mmio(phys_addr_t addr, size_t size)
> > +{
> > +	kvm_pte_t pte;
> > +	u64 offset;
> > +	int ret = 0;
> > +
> > +	if (static_branch_unlikely(&kvm_protected_mode_initialized))
> > +		return -EPERM;
> > +
> > +	if (!PAGE_ALIGNED(addr | size))
> > +		return -EINVAL;
> > +
> > +	host_lock_component();
> > +	for (offset = 0; offset < size; offset += PAGE_SIZE) {
> > +		if (addr_is_memory(addr + offset)) {
> > +			ret = -EINVAL;
> > +			goto unlock;
> > +		}
> > +		ret = kvm_pgtable_get_leaf(&host_mmu.pgt, addr + offset, &pte, NULL);
> > +		if (ret)
> > +			goto unlock;
> > +		if (!pte || kvm_pte_valid(pte)) {
> > +			ret = -EINVAL;
> > +			goto unlock;
> > +		}
> 
> It should probably verify it is also mapped as we expect into the hyp.

I thought about that, but the caller is be trusted and I was aiming
for simplicity, so the check for PKVM_ID_HYP seemed enough, but no
strong opinion.

> 
> 
> > +		if (FIELD_GET(KVM_HOST_DONATION_PTE_OWNER_MASK, pte) != PKVM_ID_HYP) {
> > +			ret = -EPERM;
> > +			goto unlock;
> > +		}
> > +	}
> > +	WARN_ON(host_stage2_idmap_locked(addr, size, PKVM_HOST_MMIO_PROT));
> 
> Perhaps we are better of simply zapping the entry and not map it proactively?

I do not think it matters that much, this is a failure path, and the
space is too small anyway. But no strong opinion also.

> 
> And even if we can't "reclaim" the VA-space from the private range, it would be
> good to unmap it. I have made a function for that as part of the allocator
> series:
> 
> https://lore.kernel.org/all/20260706175415.2604046-3-vdonnefort@google.com/
>
I do not think that is a big problem at the moment all the other
users of the VA space leak it on failure, I can re-use this logic
when the allocator seires lands, otherwise I'd avoid adding more code.

Thanks,
Mostafa