Re: [PATCH v3 11/52] xen/arm: mmu: fold FIXMAP into MMU system

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/06/2023 04:34, Penny Zheng wrote:
> FIXMAP in MMU system is used to do special-purpose 4K mapping, like
> mapping early UART, temporarily mapping source codes for copy and paste
> (copy_from_paddr), etc.
>
> As FIXMAP feature is highly dependent on virtual address translation, we
> introduce a new Kconfig CONFIG_HAS_FIXMAP to wrap all releated codes, then
> we fold it into MMU system.
> Since PMAP relies on FIXMAP, so we fold it too into MMU system.
>
> Under !CONFIG_HAS_FIXMAP, we provide empty stubbers for not breaking
> compilation.
Looking at the end result, I can't find any use of set_fixmap() in the 
common code. So I am not sure this is warrant to provide any stubs (see 
above).
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
> v1 -> v2
> - new patch
> ---
> v3:
> - fold CONFIG_HAS_FIXMAP into CONFIG_HAS_MMU
> - change CONFIG_HAS_FIXMAP to an Arm-specific Kconfig
> ---
>   xen/arch/arm/Kconfig              |  7 ++++++-
>   xen/arch/arm/include/asm/fixmap.h | 31 ++++++++++++++++++++++++++++---
>   2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index fb77392b82..22b28b8ba2 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 IOMMU_FORCE_PT_SHARE
>   
>   config ARCH_DEFCONFIG
> @@ -63,11 +62,17 @@ source "arch/Kconfig"
>   config HAS_MMU
>         bool "Memory Management Unit support in a VMSA system"
>         default y
> +       select HAS_PMAP
>         help
>           In a VMSA system, a Memory Management Unit (MMU) provides 
> fine-grained control of
>           a memory system through a set of virtual to physical address mappings 
> and associated memory
>           properties held in memory-mapped tables known as translation tables.
>   
> +config HAS_FIXMAP
> +       bool "Provide special-purpose 4K mapping slots in a VMSA"

Regardless what I wrote above, I don't think a developer should be able 
to disable HAS_FIXMAP when the HAS_MMU is used. So the 3 lines should be 
replaced with:
def_bool HAS_MMU

> +       depends on HAS_MMU
> +       default y
> +
>   config ACPI
>         bool "ACPI (Advanced Configuration and Power Interface) Support 
> (UNSUPPORTED)" if UNSUPPORTED
>         depends on ARM_64
> diff --git a/xen/arch/arm/include/asm/fixmap.h 
> b/xen/arch/arm/include/asm/fixmap.h
> index d0c9a52c8c..1b5b62866b 100644
> --- a/xen/arch/arm/include/asm/fixmap.h
> +++ b/xen/arch/arm/include/asm/fixmap.h
> @@ -4,9 +4,6 @@
>   #ifndef __ASM_FIXMAP_H
>   #define __ASM_FIXMAP_H
>   
> -#include <xen/acpi.h>
> -#include <xen/pmap.h>
> -
>   /* Fixmap slots */
>   #define FIXMAP_CONSOLE  0  /* The primary UART */
>   #define FIXMAP_MISC     1  /* Ephemeral mappings of hardware */
> @@ -22,6 +19,11 @@
>   
>   #ifndef __ASSEMBLY__
>   
> +#ifdef CONFIG_HAS_FIXMAP
> +
> +#include <xen/acpi.h>
> +#include <xen/pmap.h>
> +
>   /*
>    * Direct access to xen_fixmap[] should only happen when {set,
>    * clear}_fixmap() is unusable (e.g. where we would end up to
> @@ -43,6 +45,29 @@ static inline unsigned int virt_to_fix(vaddr_t vaddr)
>       return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
>   }
>   
> +#else /* !CONFIG_HAS_FIXMAP */
> +
> +#include <xen/mm-frame.h>
> +#include <xen/errno.h>
I think they should be included outside of #ifdef.

> +
> +static inline void set_fixmap(unsigned int map, mfn_t mfn,
> +                              unsigned int attributes)
> +{
> +    ASSERT_UNREACHABLE();
> +}
If there is an interest to have a stub, then I think we should be using 
BUG() because if this gets call, then it would likely crash right after 
when the user tries to access it.
> +
> +static inline void clear_fixmap(unsigned int map)
> +{
> +    ASSERT_UNREACHABLE();
This one might be OK with ASSERT_UNREACHABLE(). Yet, it might be best to 
use BUG() as nobody should use it.
> +}
> +
> +static inline unsigned int virt_to_fix(vaddr_t vaddr)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -EINVAL;
This is a bit of a random value. This may or may not be mapped. And 
therefore any user of the return may or may not crash.
Overall, it feels like we are trying to just please the compiler by 
writing bogus stubs. It is going to be hard to get them correct. So it 
would be better if we have no use of the 3 helpers in the common code.

> +}
> +#endif /* !CONFIG_HAS_FIXMAP */
> +
>   #endif /* __ASSEMBLY__ */
>   
>   #endif /* __ASM_FIXMAP_H */
Cheers,

--
Julien Grall