Re: [PATCH v4 05/15] arm64: Mark all I/O as non-secure shared

[Will Deacon]

On Mon, Jul 01, 2024 at 10:54:55AM +0100, Steven Price wrote:
> All I/O is by default considered non-secure for realms. As such
> mark them as shared with the host.
> 
> Co-developed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Signed-off-by: Steven Price <steven.price@xxxxxxx>
> ---
> Changes since v3:
>  * Add PROT_NS_SHARED to FIXMAP_PAGE_IO rather than overriding
>    set_fixmap_io() with a custom function.
>  * Modify ioreamp_cache() to specify PROT_NS_SHARED too.
> ---
>  arch/arm64/include/asm/fixmap.h | 2 +-
>  arch/arm64/include/asm/io.h     | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> index 87e307804b99..f2c5e653562e 100644
> --- a/arch/arm64/include/asm/fixmap.h
> +++ b/arch/arm64/include/asm/fixmap.h
> @@ -98,7 +98,7 @@ enum fixed_addresses {
>  #define FIXADDR_TOT_SIZE	(__end_of_fixed_addresses << PAGE_SHIFT)
>  #define FIXADDR_TOT_START	(FIXADDR_TOP - FIXADDR_TOT_SIZE)
>  
> -#define FIXMAP_PAGE_IO     __pgprot(PROT_DEVICE_nGnRE)
> +#define FIXMAP_PAGE_IO     __pgprot(PROT_DEVICE_nGnRE | PROT_NS_SHARED)
>  
>  void __init early_fixmap_init(void);
>  
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 4ff0ae3f6d66..07fc1801c6ad 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -277,12 +277,12 @@ static inline void __const_iowrite64_copy(void __iomem *to, const void *from,
>  
>  #define ioremap_prot ioremap_prot
>  
> -#define _PAGE_IOREMAP PROT_DEVICE_nGnRE
> +#define _PAGE_IOREMAP (PROT_DEVICE_nGnRE | PROT_NS_SHARED)
>  
>  #define ioremap_wc(addr, size)	\
> -	ioremap_prot((addr), (size), PROT_NORMAL_NC)
> +	ioremap_prot((addr), (size), (PROT_NORMAL_NC | PROT_NS_SHARED))
>  #define ioremap_np(addr, size)	\
> -	ioremap_prot((addr), (size), PROT_DEVICE_nGnRnE)
> +	ioremap_prot((addr), (size), (PROT_DEVICE_nGnRnE | PROT_NS_SHARED))

Hmm. I do wonder whether you've pushed the PROT_NS_SHARED too far here.

There's nothing _architecturally_ special about the top address bit.
Even if the RSI divides the IPA space in half, the CPU doesn't give two
hoots about it in the hardware. In which case, it feels wrong to bake
PROT_NS_SHARED into ioremap_prot -- it feels much better to me if the
ioremap() code OR'd that into the physical address when passing it down

There's a selfish side of that argument, in that we need to hook
ioremap() for pKVM protected guests, but I do genuinely feel that
treating address bits as protection bits is arbitrary and doesn't belong
in these low-level definitions. In a similar vein, AMD has its
sme_{set,clr}() macros that operate on the PA (e.g. via dma_to_phys()),
which feels like a more accurate abstraction to me.

Will