Re: [PATCH 09/10] xen/arm: introduce Kconfig HAS_PAGING_MEMPOOL

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

> On 14 Mar 2025, at 07:58, Orzel, Michal <michal.orzel@xxxxxxx> wrote:
> 
> 
> 
> On 12/03/2025 14:52, Luca Fancellu wrote:
>> 
>> 
>> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>> 
>> ARM MPU system doesn't need to use paging memory pool, as MPU memory
>> mapping table at most takes only one 4KB page, which is enough to
> NIT: I always wonder what "mapping" means in MPU world. Is this common to use
> this word even though there's no mapping being done? I'd expect to see e.g.
> protection table or similar.

well if we have a look on the Arm specifications for Armv8-r aarch64, chapter 
C1 PMSA,
it refers to “address map” and “flat maps”, so I think mapping is still ok for 
MPU.

> 
>> manage the maximum 255 MPU memory regions, for all EL2 stage 1
>> translation and EL1 stage 2 translation.
>> 
>> Introduce HAS_PAGING_MEMPOOL Kconfig symbol for Arm, selected for MMU
>> systems.
> I don't think this should be Arm symbol. After all, we have common helpers 
> like
> arch_get_paging_mempool_size(), so I'd naturally expect this symbol to be in 
> the
> common Kconfig.

yes you are right, I’ll move this

> 
>> 
>> Wrap code accessing the 'struct paging_domain' paging member with
>> the new Kconfig, introduce arch_{get,set}_paging_mempool_size
>> implementation for MPU system, provide stubs for p2m_teardown and
>> p2m_teardown_allocation because they will not be used for MPU systems,
>> remove 'struct paging_domain' from Arm 'struct arch_domain' when the
>> field is not required.
>> 
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> ---
>> xen/arch/arm/Kconfig              |  4 ++++
>> xen/arch/arm/dom0less-build.c     |  2 ++
>> xen/arch/arm/include/asm/domain.h |  2 ++
>> xen/arch/arm/mpu/Makefile         |  1 +
>> xen/arch/arm/mpu/p2m.c            | 36 +++++++++++++++++++++++++++++++
>> 5 files changed, 45 insertions(+)
>> create mode 100644 xen/arch/arm/mpu/p2m.c
>> 
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 5ac6ec0212d2..89c099ff464b 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -76,6 +76,7 @@ choice
>> config MMU
>>        bool "MMU"
>>        select HAS_LLC_COLORING if !NUMA && ARM_64
>> +       select HAS_PAGING_MEMPOOL
>>        select HAS_PMAP
>>        select HAS_VMAP
>>        select HAS_PASSTHROUGH
>> @@ -158,6 +159,9 @@ config VGICV2
>> config HVM
>>         def_bool y
>> 
>> +config HAS_PAGING_MEMPOOL
>> +       bool
> See above
> 
>> +
>> config NEW_VGIC
>>        bool "Use new VGIC implementation"
>>        select GICV2
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 31f31c38da3f..feaba576198b 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -865,11 +865,13 @@ static int __init construct_domU(struct domain *d,
>>                 p2m_mem_mb << (20 - PAGE_SHIFT) :
>>                 domain_p2m_pages(mem, d->max_vcpus);
>> 
>> +#ifdef CONFIG_HAS_PAGING_MEMPOOL
>>     spin_lock(&d->arch.paging.lock);
>>     rc = p2m_set_allocation(d, p2m_pages, NULL);
>>     spin_unlock(&d->arch.paging.lock);
>>     if ( rc != 0 )
>>         return rc;
>> +#endif
> That's does not look right. If we don't have P2M pool for MPU domains, then
> xen,domain-p2m-mem-mb dom0less parameter does not make sense as well as its
> handling above that you did not protect.

yes the protection should be extended above
    rc = dt_property_read_u32(node, "xen,domain-p2m-mem-mb", &p2m_mem_mb);

Shall we throw any warning if !HAS_PAGING_MEMPOOL but the user sets 
"xen,domain-p2m-mem-mb”?


> 
>> 
>>     printk("*** LOADING DOMU cpus=%u memory=%#"PRIx64"KB ***\n",
>>            d->max_vcpus, mem);
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 50b6a4b00982..fadec7d8fa9e 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -75,7 +75,9 @@ struct arch_domain
>> 
>>     struct hvm_domain hvm;
>> 
>> +#ifdef CONFIG_HAS_PAGING_MEMPOOL
>>     struct paging_domain paging;
>> +#endif
>> 
>>     struct vmmio vmmio;
>> 
>> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
>> index 04df0b2ee760..f1417cd1b9db 100644
>> --- a/xen/arch/arm/mpu/Makefile
>> +++ b/xen/arch/arm/mpu/Makefile
>> @@ -1,2 +1,3 @@
>> obj-y += mm.o
>> +obj-y += p2m.o
>> obj-y += setup.init.o
>> diff --git a/xen/arch/arm/mpu/p2m.c b/xen/arch/arm/mpu/p2m.c
>> new file mode 100644
>> index 000000000000..7525d9b00bcb
>> --- /dev/null
>> +++ b/xen/arch/arm/mpu/p2m.c
>> @@ -0,0 +1,36 @@
>> +
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#include <xen/domain.h>
>> +#include <asm/p2m.h>
> Why do we need to include p2m.h at this stage?

it was to have visibility of the declarations, but ...

> 
>> +
>> +/* Not used on MPU system */
>> +int p2m_teardown(struct domain *d)
>> +{
>> +    return 0;
>> +}
>> +
>> +/* Not used on MPU system */
>> +int p2m_teardown_allocation(struct domain *d)
>> +{
>> +    return 0;
>> +}
> What's the reason for adding e.g. p2m_teardown() but not p2m_final_teardown() 
> in
> this patch? I initially thought that in the next patch other functions will be
> added that require implementation in the future (i.e. stubs) but then I saw
> relinquish_p2m_mapping() which returns 0 same as these 2 functions.

as you said these can be part of the last patch? Or would you like an additional
patch that defines all the functions that are implemented as stubs (e.g. like 
these
two whose implementation is already defined and don’t require implementation
in the future)?

Cheers,
Luca