diff mbox series

[v5,08/13] xen/arm: Fold pmap and fixmap into MMU system

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

Commit Message

Henry Wang Aug. 14, 2023, 4:25 a.m. UTC
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(-)

Comments

Julien Grall Aug. 21, 2023, 6:14 p.m. UTC | #1
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,
Henry Wang Aug. 22, 2023, 2:42 a.m. UTC | #2
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
>
Julien Grall Aug. 22, 2023, 8:06 a.m. UTC | #3
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,
Henry Wang Aug. 22, 2023, 8:08 a.m. UTC | #4
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 mbox series

Patch

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