Message ID | 20230814042536.878720-9-Henry.Wang@arm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xen/arm: Split MMU code as the prepration of MPU work | expand |
Hi Henry, On 14/08/2023 05:25, Henry Wang wrote: > From: Penny Zheng <penny.zheng@arm.com> > > 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@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > 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,
Hi Julien, > On Aug 22, 2023, at 02:14, Julien Grall <julien@xen.org> wrote: > > Hi Henry, > > On 14/08/2023 05:25, Henry Wang wrote: >> From: Penny Zheng <penny.zheng@arm.com> >> >> 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). I think you are correct, so I reverted the virt_to_fix() change from this patch, deleted the include of asm/fixmap.h in kernel.c and put this patch as the (last - 1)th patch of the series. The building of Xen works fine. Does below updated patch look good to you? ``` 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/kernel.c b/xen/arch/arm/kernel.c index 0d433a32e7..bc3e5bd6f9 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -16,7 +16,6 @@ #include <xen/vmap.h> #include <asm/byteorder.h> -#include <asm/fixmap.h> #include <asm/kernel.h> #include <asm/setup.h> ``` I will update the commit message accordingly. Kind regards, Henry > > So I think it would be feasible to not touch fixmap.h at all. > > Cheers, > > -- > Julien Grall >
Hi, On 22/08/2023 03:42, Henry Wang wrote: > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index 0d433a32e7..bc3e5bd6f9 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -16,7 +16,6 @@ > #include <xen/vmap.h> > > #include <asm/byteorder.h> > -#include <asm/fixmap.h> > #include <asm/kernel.h> > #include <asm/setup.h> > ``` The changes in kernel.c should go in patch #12 where the fixmap user is moved out. Cheers,
Hi Julien, > On Aug 22, 2023, at 16:06, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 22/08/2023 03:42, Henry Wang wrote: >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >> index 0d433a32e7..bc3e5bd6f9 100644 >> --- a/xen/arch/arm/kernel.c >> +++ b/xen/arch/arm/kernel.c >> @@ -16,7 +16,6 @@ >> #include <xen/vmap.h> >> #include <asm/byteorder.h> >> -#include <asm/fixmap.h> >> #include <asm/kernel.h> >> #include <asm/setup.h> >> ``` > > The changes in kernel.c should go in patch #12 where the fixmap user is moved out. Thanks, sounds good. I will fix it in v6. Kind regards, Henry > > Cheers, > > -- > Julien Grall
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); #endif /* __ASSEMBLY__ */ diff --git a/xen/arch/arm/mmu/mm.c b/xen/arch/arm/mmu/mm.c index b70982e9d6..1d6267e6c5 100644 --- a/xen/arch/arm/mmu/mm.c +++ b/xen/arch/arm/mmu/mm.c @@ -1136,6 +1136,13 @@ int __init populate_pt_range(unsigned long virt, unsigned long nr_mfns) return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE); } +unsigned int virt_to_fix(vaddr_t vaddr) +{ + BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START); + + return ((vaddr - FIXADDR_START) >> PAGE_SHIFT); +} + /* * Local variables: * mode: C