Re: [PATCH v4 07/15] arm64: Enforce bounce buffers for realm DMA

[Steven Price <steven.price@arm.com>]

On 09/07/2024 12:56, Will Deacon wrote:
> On Mon, Jul 01, 2024 at 10:54:57AM +0100, Steven Price wrote:
>> Within a realm guest it's not possible for a device emulated by the VMM
>> to access arbitrary guest memory. So force the use of bounce buffers to
>> ensure that the memory the emulated devices are accessing is in memory
>> which is explicitly shared with the host.
>>
>> This adds a call to swiotlb_update_mem_attributes() which calls
>> set_memory_decrypted() to ensure the bounce buffer memory is shared with
>> the host. For non-realm guests or hosts this is a no-op.
>>
>> Co-developed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> v3: Simplify mem_init() by using a 'flags' variable.
>> ---
>>  arch/arm64/kernel/rsi.c |  2 ++
>>  arch/arm64/mm/init.c    | 10 +++++++++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
>> index 7ac5fc4a27d0..918db258cd4a 100644
>> --- a/arch/arm64/kernel/rsi.c
>> +++ b/arch/arm64/kernel/rsi.c
>> @@ -6,6 +6,8 @@
>>  #include <linux/jump_label.h>
>>  #include <linux/memblock.h>
>>  #include <linux/psci.h>
>> +#include <linux/swiotlb.h>
>> +
>>  #include <asm/rsi.h>
>>  
>>  struct realm_config config;
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index 9b5ab6818f7f..1d595b63da71 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -41,6 +41,7 @@
>>  #include <asm/kvm_host.h>
>>  #include <asm/memory.h>
>>  #include <asm/numa.h>
>> +#include <asm/rsi.h>
>>  #include <asm/sections.h>
>>  #include <asm/setup.h>
>>  #include <linux/sizes.h>
>> @@ -369,8 +370,14 @@ void __init bootmem_init(void)
>>   */
>>  void __init mem_init(void)
>>  {
>> +	unsigned int flags = SWIOTLB_VERBOSE;
>>  	bool swiotlb = max_pfn > PFN_DOWN(arm64_dma_phys_limit);
>>  
>> +	if (is_realm_world()) {
>> +		swiotlb = true;
>> +		flags |= SWIOTLB_FORCE;
>> +	}
>> +
>>  	if (IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) && !swiotlb) {
>>  		/*
>>  		 * If no bouncing needed for ZONE_DMA, reduce the swiotlb
>> @@ -382,7 +389,8 @@ void __init mem_init(void)
>>  		swiotlb = true;
>>  	}
>>  
>> -	swiotlb_init(swiotlb, SWIOTLB_VERBOSE);
>> +	swiotlb_init(swiotlb, flags);
>> +	swiotlb_update_mem_attributes();
> 
> Why do we have to call this so early? Certainly, we won't have probed
> the hypercalls under pKVM yet and I think it would be a lot cleaner if
> you could defer your RSI discovery too.

I don't think we *need* the swiotlb up so early, it was more of a case
of this seemed a convenient place. We can probably move this later if
pKVM has a requirement for this.

In terms of RSI discovery then see my reply on patch 2.

> Looking forward to the possibility of device assignment in future, how
> do you see DMA_BOUNCE_UNALIGNED_KMALLOC interacting with a decrypted
> SWIOTLB buffer? I'm struggling to wrap my head around how to fix that
> properly.

Device assignment generally causes problems with bounce buffers. The
assumption has mostly been that any device which is clever enough to
deal with device assignment (which in our case means enlightened enough
to understand the extra bit on the bus) wouldn't need bounce buffers.

Suzuki: Do you know if DMA_BOUNCE_UNALIGNED_KMALLOC has been thought
about? Can we make the assumption that an assigned device will be coherent?

I have to admit to being a bit worried about current assumption that we
don't need two sets of bounce buffers - one decrypted for talking to the
host, and one encrypted for "old fashioned" talking to hardware which
has hardware limitations.

Steve