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

Mostafa Saleh
2023-02-07T12:55:12Z

Hi Jean,

On Wed, Feb 01, 2023 at 12:52:59PM +0000, Jean-Philippe Brucker wrote:
> Host pages mapped in the SMMU must not be donated to the guest or
> hypervisor, since the host could then use DMA to break confidentiality.
> Mark them shared in the host stage-2 page tables, and keep a refcount in
> the hyp vmemmap.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 3 +
> arch/arm64/kvm/hyp/nvhe/mem_protect.c | 185 ++++++++++++++++++
> 2 files changed, 188 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> index 021825aee854..a363d58a998b 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> @@ -58,6 +58,7 @@ enum pkvm_component_id {
> PKVM_ID_HOST,
> PKVM_ID_HYP,
> PKVM_ID_GUEST,
> + PKVM_ID_IOMMU,
> };
>
> extern unsigned long hyp_nr_cpus;
> @@ -72,6 +73,8 @@ int __pkvm_host_share_guest(u64 pfn, u64 gfn, struct pkvm_hyp_vcpu *vcpu);
> int __pkvm_host_donate_guest(u64 pfn, u64 gfn, struct pkvm_hyp_vcpu *vcpu);
> int __pkvm_guest_share_host(struct pkvm_hyp_vcpu *hyp_vcpu, u64 ipa);
> int __pkvm_guest_unshare_host(struct pkvm_hyp_vcpu *hyp_vcpu, u64 ipa);
> +int __pkvm_host_share_dma(u64 phys_addr, size_t size, bool is_ram);
> +int __pkvm_host_unshare_dma(u64 phys_addr, size_t size);
>
> bool addr_is_memory(phys_addr_t phys);
> int host_stage2_idmap_locked(phys_addr_t addr, u64 size, enum kvm_pgtable_prot prot);
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 856673291d70..dcf08ce03790 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -1148,6 +1148,9 @@ static int check_share(struct pkvm_mem_share *share)
> case PKVM_ID_GUEST:
> ret = guest_ack_share(completer_addr, tx, share->completer_prot);
> break;
> + case PKVM_ID_IOMMU:
> + ret = 0;
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -1185,6 +1188,9 @@ static int __do_share(struct pkvm_mem_share *share)
> case PKVM_ID_GUEST:
> ret = guest_complete_share(completer_addr, tx, share->completer_prot);
> break;
> + case PKVM_ID_IOMMU:
> + ret = 0;
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -1239,6 +1245,9 @@ static int check_unshare(struct pkvm_mem_share *share)
> case PKVM_ID_HYP:
> ret = hyp_ack_unshare(completer_addr, tx);
> break;
> + case PKVM_ID_IOMMU:
> + ret = 0;
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -1273,6 +1282,9 @@ static int __do_unshare(struct pkvm_mem_share *share)
> case PKVM_ID_HYP:
> ret = hyp_complete_unshare(completer_addr, tx);
> break;
> + case PKVM_ID_IOMMU:
> + ret = 0;
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -1633,6 +1645,179 @@ void hyp_unpin_shared_mem(void *from, void *to)
> host_unlock_component();
> }
>
> +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/

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

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.

Thanks,
Mostafa