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

[Mostafa Saleh]

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

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

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

Thanks,
Mostafa