| Message ID | 20230626033443.2943270-12-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
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@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 > + 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,
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, >
Hi Penny, On 05/07/2023 09:19, Penny Zheng wrote: > On 2023/7/5 06:12, Julien Grall wrote: >> On 26/06/2023 04:34, Penny Zheng wrote: >>> 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 Do you still need HAS_FIXMAP if this patch is dropped? Cheers,
Hi Julien On 2023/7/5 18:31, Julien Grall wrote: > Hi Penny, > > On 05/07/2023 09:19, Penny Zheng wrote: >> On 2023/7/5 06:12, Julien Grall wrote: >>> On 26/06/2023 04:34, Penny Zheng wrote: >>>> 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 > > Do you still need HAS_FIXMAP if this patch is dropped? Right now, this patch only contains: 1) wrap PMAP into MMU 2) change static inline virt_to_fix() to declaration in fixmap.h and definition in mmu/mm.c. HAS_FIXMAP is not needed anymore. > > Cheers, >
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" + 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> + +static inline void set_fixmap(unsigned int map, mfn_t mfn, + unsigned int attributes) +{ + ASSERT_UNREACHABLE(); +} + +static inline void clear_fixmap(unsigned int map) +{ + ASSERT_UNREACHABLE(); +} + +static inline unsigned int virt_to_fix(vaddr_t vaddr) +{ + ASSERT_UNREACHABLE(); + return -EINVAL; +} +#endif /* !CONFIG_HAS_FIXMAP */ + #endif /* __ASSEMBLY__ */ #endif /* __ASM_FIXMAP_H */