Re: [PATCH v5 1/2] dma-mapping: introduce DMA_ATTR_CC_SHARED for shared memory

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Jiri Pirko <jiri@resnulli.us> writes:

> From: Jiri Pirko <jiri@nvidia.com>
>
> Current CC designs don't place a vIOMMU in front of untrusted devices.
> Instead, the DMA API forces all untrusted device DMA through swiotlb
> bounce buffers (is_swiotlb_force_bounce()) which copies data into
> shared memory on behalf of the device.
>
> When a caller has already arranged for the memory to be shared
> via set_memory_decrypted(), the DMA API needs to know so it can map
> directly using the unencrypted physical address rather than bounce
> buffering. Following the pattern of DMA_ATTR_MMIO, add
> DMA_ATTR_CC_SHARED for this purpose. Like the MMIO case, only the
> caller knows what kind of memory it has and must inform the DMA API
> for it to work correctly.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v4->v5:
> - rebased on top od dma-mapping-for-next
> - s/decrypted/shared/
> v3->v4:
> - added some sanity checks to dma_map_phys and dma_unmap_phys
> - enhanced documentation of DMA_ATTR_CC_DECRYPTED attr
> v1->v2:
> - rebased on top of recent dma-mapping-fixes
> ---
>  include/linux/dma-mapping.h | 10 ++++++++++
>  include/trace/events/dma.h  |  3 ++-
>  kernel/dma/direct.h         | 14 +++++++++++---
>  kernel/dma/mapping.c        | 13 +++++++++++--
>  4 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 677c51ab7510..db8ab24a54f4 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -92,6 +92,16 @@
>   * flushing.
>   */
>  #define DMA_ATTR_REQUIRE_COHERENT	(1UL << 12)
> +/*
> + * DMA_ATTR_CC_SHARED: Indicates the DMA mapping is shared (decrypted) for
> + * confidential computing guests. For normal system memory the caller must have
> + * called set_memory_decrypted(), and pgprot_decrypted must be used when
> + * creating CPU PTEs for the mapping. The same shared semantic may be passed
> + * to the vIOMMU when it sets up the IOPTE. For MMIO use together with
> + * DMA_ATTR_MMIO to indicate shared MMIO. Unless DMA_ATTR_MMIO is provided
> + * a struct page is required.
> + */
> +#define DMA_ATTR_CC_SHARED	(1UL << 13)
>  
>  /*
>   * A dma_addr_t can hold any valid DMA or bus address for the platform.  It can
> diff --git a/include/trace/events/dma.h b/include/trace/events/dma.h
> index 63597b004424..31c9ddf72c9d 100644
> --- a/include/trace/events/dma.h
> +++ b/include/trace/events/dma.h
> @@ -34,7 +34,8 @@ TRACE_DEFINE_ENUM(DMA_NONE);
>  		{ DMA_ATTR_PRIVILEGED, "PRIVILEGED" }, \
>  		{ DMA_ATTR_MMIO, "MMIO" }, \
>  		{ DMA_ATTR_DEBUGGING_IGNORE_CACHELINES, "CACHELINES_OVERLAP" }, \
> -		{ DMA_ATTR_REQUIRE_COHERENT, "REQUIRE_COHERENT" })
> +		{ DMA_ATTR_REQUIRE_COHERENT, "REQUIRE_COHERENT" }, \
> +		{ DMA_ATTR_CC_SHARED, "CC_SHARED" })
>  
>  DECLARE_EVENT_CLASS(dma_map,
>  	TP_PROTO(struct device *dev, phys_addr_t phys_addr, dma_addr_t dma_addr,
> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index b86ff65496fc..7140c208c123 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -89,16 +89,24 @@ static inline dma_addr_t dma_direct_map_phys(struct device *dev,
>  	dma_addr_t dma_addr;
>  
>  	if (is_swiotlb_force_bounce(dev)) {
> -		if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
> -			return DMA_MAPPING_ERROR;
> +		if (!(attrs & DMA_ATTR_CC_SHARED)) {
> +			if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
> +				return DMA_MAPPING_ERROR;
>  
> -		return swiotlb_map(dev, phys, size, dir, attrs);
> +			return swiotlb_map(dev, phys, size, dir, attrs);
> +		}
> +	} else if (attrs & DMA_ATTR_CC_SHARED) {
> +		return DMA_MAPPING_ERROR;
>  	}
>

What is this check for? If we are requesting a DMA mapping with
DMA_ATTR_CC_SHARED, shouldn’t it be allowed? If not, how would we reach
the conditional below where we convert the physical address to a DMA
address using phys_to_dma_unencrypted()?. Also, how is this supposed to
interact with is_swiotlb_force_bounce()?”

>  
>  	if (attrs & DMA_ATTR_MMIO) {
>  		dma_addr = phys;
>  		if (unlikely(!dma_capable(dev, dma_addr, size, false)))
>  			goto err_overflow;
> +	} else if (attrs & DMA_ATTR_CC_SHARED) {
> +		dma_addr = phys_to_dma_unencrypted(dev, phys);
> +		if (unlikely(!dma_capable(dev, dma_addr, size, false)))
> +			goto err_overflow;
>  	} else {
>  		dma_addr = phys_to_dma(dev, phys);
>  		if (unlikely(!dma_capable(dev, dma_addr, size, true)) ||

-aneesh