Re: [PATCH v2 2/3] dma: Introduce generic dma_decrypted/dma_encrypted helpers

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Robin

Thanks for your comments, response below.

On 25/02/2025 12:49, Robin Murphy wrote:
> On 2025-02-19 10:07 pm, Suzuki K Poulose wrote:
>> AMD SME added __sme_set/__sme_clr primitives to modify the DMA address 
>> for
>> encrypted/decrypted traffic. However this doesn't fit in with other 
>> models,
>> e.g., Arm CCA where the meanings are the opposite. i.e., "decrypted" 
>> traffic
>> has a bit set and "encrypted" traffic has the top bit cleared.
>>
>> In preparation for adding the support for Arm CCA DMA conversions, 
>> convert the
>> existing primitives to more generic ones that can be provided by the 
>> backends.
>> i.e., add helpers to
>>   1. dma_encrypted - Convert a DMA address to "encrypted" [ == 
>> __sme_set() ]
>>   2. dma_decrypted - Convert a DMA address to "decrypted" [ None 
>> exists today ]
>>   3. dma_clear_encryption - Clear any "encryption"/"decryption" bits 
>> from DMA
>>      address [ SME uses __sme_clr() ]
> 
> Nit: I'd still prefer to have as much distinction as possible between 
> manipulation of the address representation itself, and any other aspects 
> such as the manipulation of underlying pagetable attributes also implied 
> by force_dma_unencrypted() on x86. So how about:
> 
> dma_addr_encrypted
> dma_addr_unencrypted
> dma_addr_clear_encryption
> 
> ?

Sure, sounds better. I am still not convinced about the 
dma_addr_clear_encryption() as it really applies to "clear" decryption
too.

The options we have are :

dma_addr_canonical(), dma_addr_normal() or any others.

> 
> Note that we intentionally avoided "decrypted" in the existing APIs to 
> minimise confusion, the only time any actual decryption is involved is 
> for an "encrypted" address.

Agreed.

> 
> (Stuff like that being why I personally would prefer to generalise away 
> from encryption as an implementation detail at all, but since everyone 
> else seems to be accustomed to the existing terminology and not 
> complaining, I'm prepared to compromise that far!)
> 
> Otherwise, the overall shape looks good to me.

I will respin this, with the changes.

Suzuki

> 
> Thanks,
> Robin.
> 
>> Since the original __sme_xxx helpers come from linux/mem_encrypt.h, 
>> use that
>> as the home for the new definitions and provide dummy ones when none 
>> is provided
>> by the architectures.
>>
>> With the above, phys_to_dma_unencrypted() uses the newly added 
>> dma_decrypted()
>> helper and to make it a bit more easier to read and avoid double 
>> conversion,
>> provide __phys_to_dma().
>>
>> No functional changes intended. Compile tested on x86 defconfig with
>> CONFIG_AMD_MEM_ENCRYPT.
>>
>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Steven Price <steven.price@arm.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   include/linux/dma-direct.h  | 12 ++++++++----
>>   include/linux/mem_encrypt.h | 23 +++++++++++++++++++++++
>>   2 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
>> index d20ecc24cb0f..9b5cc0ee86d5 100644
>> --- a/include/linux/dma-direct.h
>> +++ b/include/linux/dma-direct.h
>> @@ -78,14 +78,18 @@ static inline dma_addr_t dma_range_map_max(const 
>> struct bus_dma_region *map)
>>   #define phys_to_dma_unencrypted        phys_to_dma
>>   #endif
>>   #else
>> -static inline dma_addr_t phys_to_dma_unencrypted(struct device *dev,
>> -        phys_addr_t paddr)
>> +static inline dma_addr_t __phys_to_dma(struct device *dev, 
>> phys_addr_t paddr)
>>   {
>>       if (dev->dma_range_map)
>>           return translate_phys_to_dma(dev, paddr);
>>       return paddr;
>>   }
>> +static inline dma_addr_t phys_to_dma_unencrypted(struct device *dev,
>> +                        phys_addr_t paddr)
>> +{
>> +    return dma_decrypted(__phys_to_dma(dev, paddr));
>> +}
>>   /*
>>    * If memory encryption is supported, phys_to_dma will set the 
>> memory encryption
>>    * bit in the DMA address, and dma_to_phys will clear it.
>> @@ -94,14 +98,14 @@ static inline dma_addr_t 
>> phys_to_dma_unencrypted(struct device *dev,
>>    */
>>   static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t 
>> paddr)
>>   {
>> -    return __sme_set(phys_to_dma_unencrypted(dev, paddr));
>> +    return dma_encrypted(__phys_to_dma(dev, paddr));
>>   }
>>   static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t 
>> dma_addr)
>>   {
>>       phys_addr_t paddr;
>> -    dma_addr = __sme_clr(dma_addr);
>> +    dma_addr = dma_clear_encryption(dma_addr);
>>       if (dev->dma_range_map)
>>           paddr = translate_dma_to_phys(dev, dma_addr);
>>       else
>> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h
>> index ae4526389261..c8dcc1be695a 100644
>> --- a/include/linux/mem_encrypt.h
>> +++ b/include/linux/mem_encrypt.h
>> @@ -26,11 +26,34 @@
>>    */
>>   #define __sme_set(x)        ((x) | sme_me_mask)
>>   #define __sme_clr(x)        ((x) & ~sme_me_mask)
>> +
>> +#define dma_encrypted(x)    __sme_set(x)
>> +#define dma_clear_encryption(x)    __sme_clr(x)
>> +
>>   #else
>>   #define __sme_set(x)        (x)
>>   #define __sme_clr(x)        (x)
>>   #endif
>> +/*
>> + * dma_encrypted() and dma_decrypted() are for converting a given DMA
>> + * address to the respective type of addressing.
>> + *
>> + * dma_clear_encryption() is used to reverse the conversion back to 
>> "normal"
>> + * DMA address.
>> + */
>> +#ifndef dma_encrypted
>> +#define dma_encrypted(x)    (x)
>> +#endif
>> +
>> +#ifndef dma_decrypted
>> +#define dma_decrypted(x)    (x)
>> +#endif
>> +
>> +#ifndef dma_clear_encryption
>> +#define dma_clear_encryption(x)    (x)
>> +#endif
>> +
>>   #endif    /* __ASSEMBLY__ */
>>   #endif    /* __MEM_ENCRYPT_H__ */
>