Re: [RFC PATCH 15/45] KVM: arm64: pkvm: Add __pkvm_host_share/unshare_dma()

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Tue, Feb 07, 2023 at 12:53:56PM +0000, Mostafa Saleh wrote:
> > +static int __host_check_page_dma_shared(phys_addr_t phys_addr)
> > +{
> > +	int ret;
> > +	u64 hyp_addr;
> > +
> > +	/*
> > +	 * The page is already refcounted. Make sure it's owned by the host, and
> > +	 * not part of the hyp pool.
> > +	 */
> > +	ret = __host_check_page_state_range(phys_addr, PAGE_SIZE,
> > +					    PKVM_PAGE_SHARED_OWNED);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Refcounted and owned by host, means it's either mapped in the
> > +	 * SMMU, or it's some VM/VCPU state shared with the hypervisor.
> > +	 * The host has no reason to use a page for both.
> > +	 */
> > +	hyp_addr = (u64)hyp_phys_to_virt(phys_addr);
> > +	return __hyp_check_page_state_range(hyp_addr, PAGE_SIZE, PKVM_NOPAGE);
> 
> This works for hyp-host sharing, I am worried about scalability of this.
> For example FFA(still on the list) adds a new entity that can share data
> with host, and we would need an extra check for it.
> https://lore.kernel.org/kvmarm/20221116170335.2341003-1-qperret@google.com/

Right, although it looks like the ff-a support doesn't need a refcount at
the moment so passing such a page to iommu_map() would fail at
check_share() (but I may be wrong, I'll need another look). Even if it did
use a refcount, I think it may be fine to share it between different
entities: the refcount ensures that every sharing is undone before the
page can be donated to another entity, and a separate tracking ensures
that the host undoes the right thing (the io-pgtable mappings for pages
shared with the IOMMU, and the secure world for ff-a)

Regardless I agree that this doesn't scale, it gets too complex and I'd
prefer if tracking the page state was less subtle. I'll try to find
something more generic.

> 
> One way I can think about this, is to use the SW bits in SMMU page table to
> represent ownership, for example for PKVM_ID_IOMMU
> do_share:
> -completer: set SW bits in the SMMU page table to
> PKVM_PAGE_SHARED_BORROWED

I'm not sure I understand, would we add this in order to share a page that
is already mapped in the SMMU with another entity (e.g. ff-a)?  Doing it
this way would be difficult because there may be a lot of different SMMU
page tables mapping the page.

The fact that a page is mapped in the SMMU page table already indicates
that it's borrowed from the host so the SW bits seem redundant.

> 
> do_unshare:
> -completer: set SW bit back to PKVM_PAGE_OWNED(I think this is good
> enough for host ownership)
> 
> In __pkvm_host_share_dma_page
> If page refcount > 1, it succeeds if only the page was borrowed in the
> SMMU page table and shared in the host page table.

We could also move more information into the vmemmap, because struct
hyp_page still has some space for page state. It has six free bits at the
moment, which is enough for both owner ID and share state, and maybe we
could steal a couple more from the refcount and order fields if necessary.

With that the iommu_map() and unmap() path could also be made a lot more
scalable because they wouldn't need to take any page table locks when
updating page state, a cmpxchg may be sufficient.

Thanks,
Jean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel