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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 22, 2023, at 02:14, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Penny Zheng <penny.zheng@xxxxxxx>
>> 
>>  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
>