Re: [PATCH v5 08/13] xen/arm: Fold pmap and fixmap into MMU system

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 14/08/2023 05:25, Henry Wang wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> fixmap and pmap are MMU-specific features, so fold them to MMU system.
> Do the folding for pmap by moving the HAS_PMAP Kconfig selection under
> HAS_MMU. Do the folding for fixmap by moving the implementation of
> virt_to_fix() to mmu/mm.c, so that unnecessary stubs can be avoided.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> v5:
> - Rebase on top of xen/arm: Introduce CONFIG_MMU Kconfig option
> v4:
> - Rework "[v3,11/52] xen/arm: mmu: fold FIXMAP into MMU system",
>    change the order of this patch and avoid introducing stubs.
> ---
>   xen/arch/arm/Kconfig              | 2 +-
>   xen/arch/arm/include/asm/fixmap.h | 7 +------
>   xen/arch/arm/mmu/mm.c             | 7 +++++++
>   3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index eb0413336b..8a7b79b4b5 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -15,7 +15,6 @@ config ARM
>         select HAS_DEVICE_TREE
>         select HAS_PASSTHROUGH
>         select HAS_PDX
> -       select HAS_PMAP
>         select HAS_UBSAN
>         select IOMMU_FORCE_PT_SHARE
>   
> @@ -61,6 +60,7 @@ config PADDR_BITS
>   
>   config MMU
>         def_bool y
> +       select HAS_PMAP
>   
>   source "arch/Kconfig"
>   
> diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h
> index 734eb9b1d4..5d5de6995a 100644
> --- a/xen/arch/arm/include/asm/fixmap.h
> +++ b/xen/arch/arm/include/asm/fixmap.h
> @@ -36,12 +36,7 @@ extern void clear_fixmap(unsigned int map);
>   
>   #define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
>   
> -static inline unsigned int virt_to_fix(vaddr_t vaddr)
> -{
> -    BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
> -
> -    return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
> -}
> +extern unsigned int virt_to_fix(vaddr_t vaddr);
AFAICT, virt_to_fix() is not going to be implemented for the MPU code. 
This implies that no-one should call it.
Also, none of the definitions in fixmap.h actually makes sense for the 
MPU. I would prefer if we instead try to lmit the include of fixmap to 
when this is strictly necessary. Looking for the inclusion in staging I 
could find:
42sh> ack "\#include" | ack "fixmap" | ack -v x86
arch/arm/acpi/lib.c:28:#include <asm/fixmap.h>
arch/arm/kernel.c:19:#include <asm/fixmap.h>
arch/arm/mm.c:27:#include <asm/fixmap.h>
arch/arm/include/asm/fixmap.h:7:#include <xen/acpi.h>
arch/arm/include/asm/fixmap.h:8:#include <xen/pmap.h>
arch/arm/include/asm/pmap.h:6:#include <asm/fixmap.h>
arch/arm/include/asm/early_printk.h:14:#include <asm/fixmap.h>
common/efi/boot.c:30:#include <asm/fixmap.h>
common/pmap.c:7:#include <asm/fixmap.h>
drivers/acpi/apei/erst.c:36:#include <asm/fixmap.h>
drivers/acpi/apei/apei-io.c:32:#include <asm/fixmap.h>
drivers/char/xhci-dbc.c:30:#include <asm/fixmap.h>
drivers/char/ehci-dbgp.c:16:#include <asm/fixmap.h>
drivers/char/ns16550.c:40:#include <asm/fixmap.h>
drivers/char/xen_pv_console.c:28:#include <asm/fixmap.h>

Some of them are gone after your rework. The only remaining that we care 
are in kernel.h (but I think it can be removed after your series).
So I think it would be feasible to not touch fixmap.h at all.

Cheers,

--
Julien Grall