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@arm.com>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> 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,
>