Re: [PATCH v6 08/25] KVM: arm64: iommu: Shadow host stage-2 page table

[Vincent Donnefort]

On Mon, Jul 13, 2026 at 02:00:31PM +0000, Mostafa Saleh wrote:
> On Mon, Jul 13, 2026 at 02:24:19PM +0100, Vincent Donnefort wrote:
> > On Fri, May 01, 2026 at 11:19:10AM +0000, Mostafa Saleh wrote:
> > > Create a page-table for the IOMMU that shadows the host CPU stage-2
> > > to establish DMA isolation.
> > > 
> > > An initial snapshot is created after the driver init, then
> > > on every permission change a callback would be called for
> > > the IOMMU driver to update the page table.
> > > 
> 
> [...]
> 
> > > +	 */
> > > +	if (pte && !kvm_pte_valid(pte))
> > > +		return 0;
> > > +
> > > +	if (kvm_pte_valid(pte)) {
> > > +		prot = pkvm_to_iommu_prot(kvm_pgtable_stage2_pte_prot(pte));
> > > +		/* If the range is mapped in a single PTE, it must be the same type.*/
> > > +		if (!addr_is_memory(start))
> > > +			prot |= IOMMU_MMIO;
> > > +
> > > +		return kvm_iommu_ops->host_stage2_idmap(start, end, prot);
> > 
> > Do we really need to do that when is_memory()? 
> > 
> > fix_host_ownership_walker() by calling host_stage2_idmap_locked() and
> > host_stage2_set_owner_locked() should already handle the memory region. That
> > would also get rid of kvm_idmap_initialized.
> > 
> > So this one here could only take care of the MMIO?
> > 
> > Overall we would have a common point of synchro which is
> > fix_host_ownership_walker() after which the host ownership is ready for both
> > CPU stage-2 and the IOMMU?
> > 
> 
> I am not sure I understand, this is another empty page table, so we
> have to walk all of the host CPU stage-2 page table to shadow it in the
> IOMMU. if you are refering to the case where it handle zero ptes for
> memory, I can drop that but it will not change much in this logic.

In fixup_host_ownership() we already walk the hyp pgtable to know what needs to be
map/unmapped from the host stage-2. Can't we rely on that for the IOMMU
page-table as well?

As of, fix_host_ownership() could handle the host stage-2 __and__ the iommu? 

> 
> > > +	}
> > > +
> > > +	/* In case of invalid PTE, we need to figure out which part of it is MMIO */
> 
> [...]
> 
> > >  #include <nvhe/mm.h>
> > > @@ -481,6 +482,14 @@ static int check_range_allowed_memory(u64 start, u64 end)
> > >  	return 0;
> > >  }
> > >  
> > > +u64 find_mem_range_from(u64 start, bool *is_memory)
> > > +{
> > > +	struct kvm_mem_range r;
> > > +
> > > +	*is_memory = !!find_mem_range(start, &r);
> > > +	return r.end;
> > > +}
> > > +
> > >  static bool range_is_memory(u64 start, u64 end)
> > >  {
> > >  	struct kvm_mem_range r;
> > > @@ -577,8 +586,34 @@ int host_stage2_idmap_locked(phys_addr_t addr, u64 size,
> > >  
> > >  static void __host_update_page_state(phys_addr_t addr, u64 size, enum pkvm_page_state state)
> > 
> > I would really split this. I know that this is convinient, but as the function
> > says, it only update the page state so it shouldn't hide an update to the IOMMU.
> 
> I mention a couple of alternatives in the commit message, I tried to
> implement it differently which was harder to reason about as the calls
> was scattered everywhere and any small refactor will possibly break it.
> 
> Did you have a split in my mind? I open to rework it.

Yes, I meant the option #2.

> 
> > 
> > Beside, we have examples already in Android where we want to update the
> > page-state but not the IOMMU, so it doesn't feel future-proof...
> > 
> > >  {
> > > +	enum pkvm_page_state old = get_host_state(hyp_phys_to_page(addr));
> > > +	enum kvm_pgtable_prot prot = 0;
> > > +
> > >  	for_each_hyp_page(page, addr, size)
> > >  		set_host_state(page, state);
> > > +
> > > +	/*
> > > +	 * Any transition to PKVM_NOPAGE, unmaps the page from the host
> > > +	 * Any transition to PKVM_PAGE_SHARED_BORROWED, maps the page in the host
> > > +	 * Any transition to PKVM_PAGE_SHARED_OWNED is ignored as page is already mapped.
> > > +	 * Transitions to PKVM_PAGE_OWNED from anything but PKVM_NOPAGE are ignored.
> > > +	 * Transitions to PKVM_PAGE_OWNED from PKVM_NOPAGE will map the page.
> > > +	 */
> > > +	if ((state == PKVM_PAGE_SHARED_OWNED) ||
> > > +		((state == PKVM_PAGE_OWNED) && (old != PKVM_NOPAGE)))
> > > +		return;
> > > +
> > > +	if ((state == PKVM_PAGE_SHARED_BORROWED) ||
> > > +		(state == PKVM_PAGE_OWNED))
> > > +		prot = PKVM_HOST_MEM_PROT;
> > 
> > ... and that would avoid that sort of things here. The caller decides if the IOMMU
> > is updated or not.
> 
> Typically, the IOMMU is updated if the CPU is.
> 
> > 
> > And as the patch says, we "shadow" the host stage2. So probably modifying
> > host_stage2_idmap and host_stage2_set_owner_metdata() sounds really a better
> > approach.
> 
> Initially, before the pKVM merge upstream I was doing something similar
> as that only required one hook [1]. However, after rebasing I found that
> would be too complicated and I have to add many more (as mentioned in
> the commit message). But I can re-visit this approach in v7.

What bit would be more complicated? As you've wrote in the commit message, in
option#2 you only need two calls really:

  * host_stage2_set_owner_locked()
  * host_stage2_set_owner_metadata_locked().

This fits better the narative for the "shadow" page-table: when we map the host
stage-2, we map the iommu and when we unmap the host stage-2, we unmap the
iommu.

> 
> [1] https://lore.kernel.org/all/20250819215156.2494305-11-smostafa@xxxxxxxxxx/
> 
> Thanks,
> Mostafa
>