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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi,

On 2023/7/5 06:12, Julien Grall wrote:
> 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).
Yes, you're rignt.

I think we do not need to provide stubs for set_fixmap/clear_fixmap, or
even for virt_to_fix(), if we change the static inline virt_to_fix()
to the definition in mmu/mm.c

> > 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
Understood, will fix

> > +    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.
Agree.

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