Re: [PATCH v2 32/40] xen/mpu: implement MPU version of ioremap_xxx

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/01/2023 05:29, Penny Zheng wrote:
> Function ioremap_xxx is normally being used to remap device address ranges
> in MMU system during device driver initialization.
>
> However, in MPU system, virtual translation is not supported and
> device memory layout is statically configured in Device Tree, and being mapped
> at very early stage.
> So here we only add a check to verify this assumption.
>
> But for tolerating a few cases where the function is called to map for
> temporary copy and paste, like ioremap_wc in kernel image loading, the
> region attribute mismatch will be treated as warning than error.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/arm64/mpu.h |  1 +
>   xen/arch/arm/include/asm/mm.h        | 16 ++++-
>   xen/arch/arm/include/asm/mm_mpu.h    |  2 +
>   xen/arch/arm/mm_mpu.c                | 88 ++++++++++++++++++++++++----
>   xen/include/xen/vmap.h               | 12 ++++
>   5 files changed, 106 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> b/xen/arch/arm/include/asm/arm64/mpu.h
> index 8e8679bc82..b4e50a9a0e 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -82,6 +82,7 @@
>   #define REGION_HYPERVISOR_BOOT    (REGION_HYPERVISOR_RW|_REGION_BOOTONLY)
>   #define REGION_HYPERVISOR_SWITCH  (REGION_HYPERVISOR_RW|_REGION_SWITCH)
>   #define REGION_HYPERVISOR_NOCACHE 
> (_REGION_DEVICE|MT_DEVICE_nGnRE|_REGION_SWITCH)
> +#define REGION_HYPERVISOR_WC      (_REGION_DEVICE|MT_NORMAL_NC)
>   
>   #define INVALID_REGION            (~0UL)
>   
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 7969ec9f98..fa44cfc50d 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -14,6 +14,10 @@
>   # error "unknown ARM variant"
>   #endif
>   
> +#if defined(CONFIG_HAS_MPU)
> +# include <asm/arm64/mpu.h>
> +#endif
> +
>   /* Align Xen to a 2 MiB boundary. */
>   #define XEN_PADDR_ALIGN (1 << 21)
>   
> @@ -198,19 +202,25 @@ extern void setup_frametable_mappings(paddr_t ps, paddr_t pe);
>   /* map a physical range in virtual memory */
>   void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int 
> attributes);
>   
> +#ifndef CONFIG_HAS_MPU
> +#define DEFINE_ATTRIBUTE(var)   (PAGE_##var)
> +#else
> +#define DEFINE_ATTRIBUTE(var)   (REGION_##var)
> +#endif
The macro implies that part of the naming would be common between the 
MPU and MMU code. So I think it would be better if the full name is the 
shared.
My preference would be to go with PAGE_* as this is used for both x86 
and MMU arm. I think the naming would still be OK on the MPU because, 
AFAICT, you still map at a page granularity (or aligned to).
Cheers,

--
Julien Grall