diff mbox series

[v2,6/7] xen: introduce Kconfig HAS_PAGING_MEMPOOL

Message ID 20250316192445.2376484-7-luca.fancellu@arm.com (mailing list archive)
State New
Headers show
Series MPU mm subsystem skeleton | expand

Commit Message

Luca Fancellu March 16, 2025, 7:24 p.m. UTC
From: Penny Zheng <Penny.Zheng@arm.com>

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
manage the maximum 255 MPU memory regions, for all EL2 stage 1
translation and EL1 stage 2 translation.

Introduce HAS_PAGING_MEMPOOL Kconfig common symbol, selected for Arm
MMU systems.

Wrap the code inside 'construct_domU' that deal with p2m paging
allocation in a new function 'domu_p2m_set_allocation', protected
by HAS_PAGING_MEMPOOL, this is done in this way to prevent polluting
the former function with #ifdefs and improve readability

Introduce arch_{get,set}_paging_mempool_size implementation for MPU
system.

Remove 'struct paging_domain' from Arm 'struct arch_domain' when the
field is not required.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
 - make Kconfig HAS_PAGING_MEMPOOL common
 - protect also "xen,domain-p2m-mem-mb" reading with HAS_PAGING_MEMPOOL
 - do not define p2m_teardown{_allocation} in this patch
 - change commit message
---
 xen/arch/arm/Kconfig              |  1 +
 xen/arch/arm/dom0less-build.c     | 48 ++++++++++++++++++++++++-------
 xen/arch/arm/include/asm/domain.h |  2 ++
 xen/arch/arm/mpu/Makefile         |  1 +
 xen/arch/arm/mpu/p2m.c            | 25 ++++++++++++++++
 xen/common/Kconfig                |  3 ++
 6 files changed, 69 insertions(+), 11 deletions(-)
 create mode 100644 xen/arch/arm/mpu/p2m.c

Comments

Jan Beulich March 17, 2025, 8:41 a.m. UTC | #1
On 16.03.2025 20:24, Luca Fancellu wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -74,6 +74,9 @@ config HAS_KEXEC
>  config HAS_LLC_COLORING
>  	bool
>  
> +config HAS_PAGING_MEMPOOL
> +	bool

Imo this is too little of a change outside of Arm-specific code here. Just
go grep for "paging_mempool" to see what pretty obviously wants to fall
under that new control. There may be more stuff. The stubs for
arch_{get,set}_paging_mempool_size() likely also want to live in a header,
perhaps even one in asm-generic/.

Jan
Luca Fancellu March 17, 2025, 9:29 a.m. UTC | #2
Hi Jan,

> On 17 Mar 2025, at 08:41, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.03.2025 20:24, Luca Fancellu wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -74,6 +74,9 @@ config HAS_KEXEC
>> config HAS_LLC_COLORING
>> bool
>> 
>> +config HAS_PAGING_MEMPOOL
>> + bool
> 
> Imo this is too little of a change outside of Arm-specific code here. Just
> go grep for "paging_mempool" to see what pretty obviously wants to fall
> under that new control. There may be more stuff. The stubs for
> arch_{get,set}_paging_mempool_size() likely also want to live in a header,
> perhaps even one in asm-generic/.

I’m wondering, since we already have arch_{get,set}_paging_mempool_size
architecture specific, I believe this config needs to be ARCH_PAGING_MEMPOOL
and needs to provide stubs inside include/xen/domain.h in a similar way of how
ARCH_MAP_DOMAIN_PAGE is done.

What do you think?

Cheers,
Luca
Jan Beulich March 17, 2025, 9:34 a.m. UTC | #3
On 17.03.2025 10:29, Luca Fancellu wrote:
>> On 17 Mar 2025, at 08:41, Jan Beulich <jbeulich@suse.com> wrote:
>> On 16.03.2025 20:24, Luca Fancellu wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -74,6 +74,9 @@ config HAS_KEXEC
>>> config HAS_LLC_COLORING
>>> bool
>>>
>>> +config HAS_PAGING_MEMPOOL
>>> + bool
>>
>> Imo this is too little of a change outside of Arm-specific code here. Just
>> go grep for "paging_mempool" to see what pretty obviously wants to fall
>> under that new control. There may be more stuff. The stubs for
>> arch_{get,set}_paging_mempool_size() likely also want to live in a header,
>> perhaps even one in asm-generic/.
> 
> I’m wondering, since we already have arch_{get,set}_paging_mempool_size
> architecture specific, I believe this config needs to be ARCH_PAGING_MEMPOOL
> and needs to provide stubs inside include/xen/domain.h in a similar way of how
> ARCH_MAP_DOMAIN_PAGE is done.
> 
> What do you think?

Imo both routes are possible. I'm not sure we have a clear preference when
looking at the code base as a whole.

Jan
Orzel, Michal March 17, 2025, 9:39 a.m. UTC | #4
On 16/03/2025 20:24, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> 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
> manage the maximum 255 MPU memory regions, for all EL2 stage 1
> translation and EL1 stage 2 translation.
> 
> Introduce HAS_PAGING_MEMPOOL Kconfig common symbol, selected for Arm
> MMU systems.
I think it should be selected by other arches that implement the stubs.
Anyway, Jan added a comment and I do agree with him.

Just a few remarks.

> 
> Wrap the code inside 'construct_domU' that deal with p2m paging
> allocation in a new function 'domu_p2m_set_allocation', protected
> by HAS_PAGING_MEMPOOL, this is done in this way to prevent polluting
> the former function with #ifdefs and improve readability
> 
> Introduce arch_{get,set}_paging_mempool_size implementation for MPU
> system.
> 
> Remove 'struct paging_domain' from Arm 'struct arch_domain' when the
> field is not required.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
>  - make Kconfig HAS_PAGING_MEMPOOL common
>  - protect also "xen,domain-p2m-mem-mb" reading with HAS_PAGING_MEMPOOL
>  - do not define p2m_teardown{_allocation} in this patch
>  - change commit message
> ---
>  xen/arch/arm/Kconfig              |  1 +
>  xen/arch/arm/dom0less-build.c     | 48 ++++++++++++++++++++++++-------
>  xen/arch/arm/include/asm/domain.h |  2 ++
>  xen/arch/arm/mpu/Makefile         |  1 +
>  xen/arch/arm/mpu/p2m.c            | 25 ++++++++++++++++
>  xen/common/Kconfig                |  3 ++
>  6 files changed, 69 insertions(+), 11 deletions(-)
>  create mode 100644 xen/arch/arm/mpu/p2m.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 5ac6ec0212d2..6b4bcf12683e 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
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 573b0d25ae41..a65bbbb05301 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -673,6 +673,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>      return -EINVAL;
>  }
> 
> +#ifdef CONFIG_HAS_PAGING_MEMPOOL
> +
>  static unsigned long __init domain_p2m_pages(unsigned long maxmem_kb,
>                                               unsigned int smp_cpus)
>  {
> @@ -688,6 +690,8 @@ static unsigned long __init domain_p2m_pages(unsigned long maxmem_kb,
>      return DIV_ROUND_UP(memkb, 1024) << (20 - PAGE_SHIFT);
>  }
> 
> +#endif
> +
>  static int __init alloc_xenstore_evtchn(struct domain *d)
>  {
>      evtchn_alloc_unbound_t alloc;
> @@ -841,6 +845,38 @@ static void __init domain_vcpu_affinity(struct domain *d,
>      }
>  }
> 
> +#ifdef CONFIG_HAS_PAGING_MEMPOOL
Why can't this be moved to upper CONFIG_HAS_PAGING_MEMPOOL block to avoid
excessive ifdefery?

> +
> +static int __init domu_p2m_set_allocation(struct domain *d, u64 mem,
s/domu/domain/ to match naming of other functions.
uint64_t. Linux types shall no longer be used.

> +                                          const struct dt_device_node *node)
> +{
> +    unsigned long p2m_pages;
> +    u32 p2m_mem_mb;
uint32_t

~Michal
diff mbox series

Patch

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 5ac6ec0212d2..6b4bcf12683e 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
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 573b0d25ae41..a65bbbb05301 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -673,6 +673,8 @@  static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     return -EINVAL;
 }
 
+#ifdef CONFIG_HAS_PAGING_MEMPOOL
+
 static unsigned long __init domain_p2m_pages(unsigned long maxmem_kb,
                                              unsigned int smp_cpus)
 {
@@ -688,6 +690,8 @@  static unsigned long __init domain_p2m_pages(unsigned long maxmem_kb,
     return DIV_ROUND_UP(memkb, 1024) << (20 - PAGE_SHIFT);
 }
 
+#endif
+
 static int __init alloc_xenstore_evtchn(struct domain *d)
 {
     evtchn_alloc_unbound_t alloc;
@@ -841,6 +845,38 @@  static void __init domain_vcpu_affinity(struct domain *d,
     }
 }
 
+#ifdef CONFIG_HAS_PAGING_MEMPOOL
+
+static int __init domu_p2m_set_allocation(struct domain *d, u64 mem,
+                                          const struct dt_device_node *node)
+{
+    unsigned long p2m_pages;
+    u32 p2m_mem_mb;
+    int rc;
+
+    rc = dt_property_read_u32(node, "xen,domain-p2m-mem-mb", &p2m_mem_mb);
+    /* If xen,domain-p2m-mem-mb is not specified, use the default value. */
+    p2m_pages = rc ?
+                p2m_mem_mb << (20 - PAGE_SHIFT) :
+                domain_p2m_pages(mem, d->max_vcpus);
+
+    spin_lock(&d->arch.paging.lock);
+    rc = p2m_set_allocation(d, p2m_pages, NULL);
+    spin_unlock(&d->arch.paging.lock);
+
+    return rc;
+}
+
+#else /* !CONFIG_HAS_PAGING_MEMPOOL */
+
+static inline int domu_p2m_set_allocation(struct domain *d, u64 mem,
+                                          const struct dt_device_node *node)
+{
+    return 0;
+}
+
+#endif /* CONFIG_HAS_PAGING_MEMPOOL */
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -848,8 +884,6 @@  static int __init construct_domU(struct domain *d,
     const char *dom0less_enhanced;
     int rc;
     u64 mem;
-    u32 p2m_mem_mb;
-    unsigned long p2m_pages;
 
     rc = dt_property_read_u64(node, "memory", &mem);
     if ( !rc )
@@ -859,15 +893,7 @@  static int __init construct_domU(struct domain *d,
     }
     kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
 
-    rc = dt_property_read_u32(node, "xen,domain-p2m-mem-mb", &p2m_mem_mb);
-    /* If xen,domain-p2m-mem-mb is not specified, use the default value. */
-    p2m_pages = rc ?
-                p2m_mem_mb << (20 - PAGE_SHIFT) :
-                domain_p2m_pages(mem, d->max_vcpus);
-
-    spin_lock(&d->arch.paging.lock);
-    rc = p2m_set_allocation(d, p2m_pages, NULL);
-    spin_unlock(&d->arch.paging.lock);
+    rc = domu_p2m_set_allocation(d, mem, node);
     if ( rc != 0 )
         return rc;
 
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 b18cec483671..5424ab6239c8 100644
--- a/xen/arch/arm/mpu/Makefile
+++ b/xen/arch/arm/mpu/Makefile
@@ -1 +1,2 @@ 
 obj-y += mm.o
+obj-y += p2m.o
diff --git a/xen/arch/arm/mpu/p2m.c b/xen/arch/arm/mpu/p2m.c
new file mode 100644
index 000000000000..71f9cdbf2fdb
--- /dev/null
+++ b/xen/arch/arm/mpu/p2m.c
@@ -0,0 +1,25 @@ 
+
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/domain.h>
+#include <xen/errno.h>
+#include <xen/types.h>
+
+int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
+{
+    return -EOPNOTSUPP;
+}
+
+int arch_set_paging_mempool_size(struct domain *d, uint64_t size)
+{
+    return -EOPNOTSUPP;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d14..a3bdf546d02f 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -74,6 +74,9 @@  config HAS_KEXEC
 config HAS_LLC_COLORING
 	bool
 
+config HAS_PAGING_MEMPOOL
+	bool
+
 config HAS_PIRQ
 	bool