[PATCH v1 0/6] system/physmem: Enhance the Address Space API

[PATCH v1 0/6] system/physmem: Enhance the Address Space API

[Gustavo Romero]

CPUAddressSpace structures are currently allocated statically in
cpu_address_space_init(). This approach is clunky to use because
cpu->num_ases needs to be set immediately before the function is called.

Moreover, the static allocation is tightly coupled to the Address Space
(AS) indexes, which, depending on the CPU features, can leave unused
CPUAddressSpace structures.

For instance, on aarch64's 'virt' machine, if secure=off but mte=on,
three ASes are allocated, but only two ASes (ARMASIdx_NS and
ARMASIdx_Tag) are actually used.

This situation gets worse when new CPU features that require additional
ASes are added, such as FEAT_MEC (which I'm working on right now).

This series addresses these limitations by allocating the
CPUAddressSpace dynamically.

The last 4 patches in the series probably need to get squashed with the
changes in the API, but I kept them separate so it's easy to see how
simple the changes needed on the target side are.

CI results:

https://gitlab.com/gusbromero/qemu/-/pipelines/2184288007 


Cheers,
Gustavo

Gustavo Romero (6):
  system/physmem: Enhance the Address Space API
  target/arm: Initialize AS 0 first
  target/arm: Add a _MAX sentinel to ARMASIdx enum
  target/arm: Use new CPU address space API
  target/i386: Add a _MAX sentinel to X86ASIdx enum
  target/i386: Use new CPU address space API

 include/exec/cpu-common.h          | 16 +++++++--
 include/hw/core/cpu.h              |  6 +++-
 stubs/cpu-destroy-address-spaces.c |  2 +-
 system/cpus.c                      |  4 +--
 system/physmem.c                   | 56 ++++++++++++++++++++++--------
 target/arm/cpu.c                   | 20 ++++-------
 target/arm/cpu.h                   |  1 +
 target/i386/cpu.h                  |  1 +
 target/i386/kvm/kvm-cpu.c          |  4 +--
 target/i386/kvm/kvm.c              |  4 +--
 target/i386/tcg/system/tcg-cpu.c   |  6 ++--
 11 files changed, 78 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH v1 1/6] system/physmem: Enhance the Address Space API

[Gustavo Romero]

Currently, the Address Space (AS) API allocates the ASes for a CPU
statically using g_new0 in cpu_address_space_init(). This requires
computing the number of ASes and setting cpu->num_ases, directly, before
calling cpu_address_space_init() to initialize the ASes for a CPU. The
number of ASes usually depends on the number of CPU features that
require an AS.

Since it's necessary to know the total number of ASes to set it in
cpu->num_ases before calling cpu_address_space_init(), and because we
usually only check for the CPU features that require an AS later in the
code, having to set cpu->num_ases earlier complicates the code that
uses the current API.

Moreover, the number of ASes currently allocated statically is tightly
coupled to the AS index defined in an enum. For example, if ASes with
indexes 0 and 2 are required but not the AS at index 1, three ASes, i.e.
three CPUAddressSpace structs, will be allocated in the cpu->cpu_ases
array, but only two will be effectively used.

This commit addresses both issues by allocating the CPUAddressSpace
dynamically, using g_array_new and g_array_append instead of g_new0 in
cpu_address_space_add().

It introduces a new cpu_address_space_init(), used only to initialize
the number of ASes and that is called only once before adding the ASes.
An index map is also introduced, cpu->as_to_cpuas_idx, which is meant to
map the AS indexes, which can form a sparse sequence, to the CPU AS
indexes, which must be a dense sequence, as they are the indexes used to
access the CPU ASes in the cpu->cpu_ases array.

The old cpu_address_space_init() is renamed to cpu_address_space_add().
It can be called any number of times, once per AS type, up to the
maximum number of ASes defined by an architecture. Hence, no prior
computation of the number of ASes is required.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 include/exec/cpu-common.h          | 16 +++++++--
 include/hw/core/cpu.h              |  6 +++-
 stubs/cpu-destroy-address-spaces.c |  2 +-
 system/cpus.c                      |  4 +--
 system/physmem.c                   | 56 ++++++++++++++++++++++--------
 5 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index e0be4ee2b8..f3a1accdad 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -103,6 +103,17 @@ size_t qemu_ram_pagesize_largest(void);
 
 /**
  * cpu_address_space_init:
+ * @cpu: CPU to init the address space(s)
+ * @num_ases: number of address space(s) to init in the @cpu
+ *
+ * This function must be called before calling
+ * cpu_address_space_add and it sets the number of address
+ * spaces in @cpu.
+ */
+void cpu_address_space_init(CPUState *cpu, int num_ases);
+
+/**
+ * cpu_address_space_add:
  * @cpu: CPU to add this address space to
  * @asidx: integer index of this address space
  * @prefix: prefix to be used as name of address space
@@ -120,8 +131,9 @@ size_t qemu_ram_pagesize_largest(void);
  *
  * Note that with KVM only one address space is supported.
  */
-void cpu_address_space_init(CPUState *cpu, int asidx,
-                            const char *prefix, MemoryRegion *mr);
+void cpu_address_space_add(CPUState *cpu, int asidx,
+                           const char *prefix, MemoryRegion *mr);
+
 /**
  * cpu_destroy_address_spaces:
  * @cpu: CPU for which address spaces need to be destroyed
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 9615051774..cb26d6acf0 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -402,6 +402,9 @@ struct qemu_work_item;
 
 #define CPU_UNSET_NUMA_NODE_ID -1
 
+#define CPUAS_IDX(asidx)     (cpu->as_to_cpuas_idx[asidx])
+#define CPU_ASES(cpuas_idx)  (g_array_index(cpu->cpu_ases, CPUAddressSpace *, cpuas_idx))
+
 /**
  * struct CPUState - common state of one CPU core or thread.
  *
@@ -515,7 +518,8 @@ struct CPUState {
     QemuMutex work_mutex;
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
-    struct CPUAddressSpace *cpu_ases;
+    GArray *cpu_ases;
+    int *as_to_cpuas_idx;
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
diff --git a/stubs/cpu-destroy-address-spaces.c b/stubs/cpu-destroy-address-spaces.c
index dc6813f5bd..a86e8d4db1 100644
--- a/stubs/cpu-destroy-address-spaces.c
+++ b/stubs/cpu-destroy-address-spaces.c
@@ -5,7 +5,7 @@
 
 /*
  * user-mode CPUs never create address spaces with
- * cpu_address_space_init(), so the cleanup function doesn't
+ * cpu_address_space_add(), so the cleanup function doesn't
  * need to do anything. We need this stub because cpu-common.c
  * is built-once so it can't #ifndef CONFIG_USER around the
  * call; the real function is in physmem.c which is system-only.
diff --git a/system/cpus.c b/system/cpus.c
index ef2d2f241f..cfe05ef0be 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -718,8 +718,8 @@ void qemu_init_vcpu(CPUState *cpu)
         /* If the target cpu hasn't set up any address spaces itself,
          * give it the default one.
          */
-        cpu->num_ases = 1;
-        cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory);
+        cpu_address_space_init(cpu, 1 /* Number of address spaces */);
+        cpu_address_space_add(cpu, 0 /* AS index */, "cpu-memory", cpu->memory);
     }
 
     /* accelerators all implement the AccelOpsClass */
diff --git a/system/physmem.c b/system/physmem.c
index c9869e4049..03a2ecf495 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -692,7 +692,7 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
     IOMMUTLBEntry iotlb;
     int iommu_idx;
     hwaddr addr = orig_addr;
-    AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
+    AddressSpaceDispatch *d = address_space_to_dispatch(CPU_ASES(CPUAS_IDX(asidx))->as);
 
     for (;;) {
         section = address_space_translate_internal(d, addr, &addr, plen, false);
@@ -752,7 +752,7 @@ MemoryRegionSection *iotlb_to_section(CPUState *cpu,
                                       hwaddr index, MemTxAttrs attrs)
 {
     int asidx = cpu_asidx_from_attrs(cpu, attrs);
-    CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
+    CPUAddressSpace *cpuas = CPU_ASES(CPUAS_IDX(asidx));
     AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
     int section_index = index & ~TARGET_PAGE_MASK;
     MemoryRegionSection *ret;
@@ -775,10 +775,23 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
 
 #endif /* CONFIG_TCG */
 
-void cpu_address_space_init(CPUState *cpu, int asidx,
-                            const char *prefix, MemoryRegion *mr)
+void cpu_address_space_init(CPUState *cpu, int num_ases)
 {
-    CPUAddressSpace *newas;
+    /* Must be called only once per vCPU. */
+    assert(cpu->as_to_cpuas_idx == NULL);
+
+    cpu->num_ases = num_ases;
+    cpu->as_to_cpuas_idx = g_new0(int, num_ases);
+    /* Mark all AS indexes as uninitialized. */
+    for (int i = 0; i < num_ases; i++) {
+        cpu->as_to_cpuas_idx[i] = -1;
+    }
+}
+
+void cpu_address_space_add(CPUState *cpu, int asidx,
+                           const char *prefix, MemoryRegion *mr)
+{
+    CPUAddressSpace *newas = g_new0(CPUAddressSpace, 1);
     AddressSpace *as = g_new0(AddressSpace, 1);
     char *as_name;
 
@@ -787,19 +800,24 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     address_space_init(as, mr, as_name);
     g_free(as_name);
 
-    /* Target code should have set num_ases before calling us */
+    /*
+     * Target code should have called cpu_address_space_init()
+     * before to set the number of address spaces in a vCPU.
+     */
     assert(asidx < cpu->num_ases);
 
+    /* An address space can only be initialized once. */
+    assert(cpu->as_to_cpuas_idx[asidx] == -1);
+
     if (asidx == 0) {
         /* address space 0 gets the convenience alias */
         cpu->as = as;
     }
 
     if (!cpu->cpu_ases) {
-        cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
+        cpu->cpu_ases = g_array_new(true, true, sizeof(CPUAddressSpace *));
     }
 
-    newas = &cpu->cpu_ases[asidx];
     newas->cpu = cpu;
     newas->as = as;
     if (tcg_enabled()) {
@@ -808,20 +826,25 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
         newas->tcg_as_listener.name = "tcg";
         memory_listener_register(&newas->tcg_as_listener, as);
     }
+
+   /* Map the AS index to the next available index in the CPU ASes array. */
+   cpu->as_to_cpuas_idx[asidx] = cpu->cpu_ases->len;
+   /* Add the new CPU AS to the slot at the next available index. */
+   g_array_append_val(cpu->cpu_ases, newas);
 }
 
 void cpu_destroy_address_spaces(CPUState *cpu)
 {
     CPUAddressSpace *cpuas;
-    int asidx;
+    int cpuas_idx;
 
     assert(cpu->cpu_ases);
 
     /* convenience alias just points to some cpu_ases[n] */
     cpu->as = NULL;
 
-    for (asidx = 0; asidx < cpu->num_ases; asidx++) {
-        cpuas = &cpu->cpu_ases[asidx];
+    for (cpuas_idx = cpu->cpu_ases->len; cpuas_idx-- > 0; ) {
+        cpuas = CPU_ASES(cpuas_idx);
         if (!cpuas->as) {
             /* This index was never initialized; no deinit needed */
             continue;
@@ -829,16 +852,19 @@ void cpu_destroy_address_spaces(CPUState *cpu)
         if (tcg_enabled()) {
             memory_listener_unregister(&cpuas->tcg_as_listener);
         }
+        /* Free CPUAddressSpace->AddressSpace. */
         g_clear_pointer(&cpuas->as, address_space_destroy_free);
+        /* Free CPUAddressSpace. */
+        g_free(cpuas);
+        /* Remove pointer to freed CPUAddressSpace. */
+        g_array_remove_index(cpu->cpu_ases, cpuas_idx);
     }
-
-    g_clear_pointer(&cpu->cpu_ases, g_free);
 }
 
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 {
     /* Return the AddressSpace corresponding to the specified index */
-    return cpu->cpu_ases[asidx].as;
+    return CPU_ASES(CPUAS_IDX(asidx))->as;
 }
 
 /* Called from RCU critical section */
@@ -4056,7 +4082,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
         if (l > len)
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
-        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
+        res = address_space_rw(CPU_ASES(CPUAS_IDX(asidx))->as, phys_addr, attrs, buf,
                                l, is_write);
         if (res != MEMTX_OK) {
             return -1;
-- 
2.34.1

[PATCH v1 2/6] target/arm: Initialize AS 0 first

[Gustavo Romero]

Move the initialization of AS of index 0 (ARMASIdx_NS) so it happens
before the initialization of the other address spaces. This is purely a
code organization change with no effect on behavior.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Suggested-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 39292fb9bc..1640b20b4d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2158,6 +2158,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cs->num_ases = 1 + has_secure;
     }
 
+    cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);
+
     if (has_secure) {
         if (!cpu->secure_memory) {
             cpu->secure_memory = cs->memory;
@@ -2175,8 +2177,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
-    cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);
-
     /* No core_count specified, default to smp_cpus. */
     if (cpu->core_count == -1) {
         cpu->core_count = smp_cpus;
-- 
2.34.1

[PATCH v1 3/6] target/arm: Add a _MAX sentinel to ARMASIdx enum

[Gustavo Romero]

Add a sentinel to the ARMASIdx enum so it can be used when the total
number of address spaces is required.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/cpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 39f2b2e54d..00f5af0fcd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2336,6 +2336,7 @@ typedef enum ARMASIdx {
     ARMASIdx_S = 1,
     ARMASIdx_TagNS = 2,
     ARMASIdx_TagS = 3,
+    ARMASIdx_MAX
 } ARMASIdx;
 
 static inline ARMMMUIdx arm_space_to_phys(ARMSecuritySpace space)
-- 
2.34.1

[PATCH v1 4/6] target/arm: Use new CPU address space API

[Gustavo Romero]

Use new cpu_addess_space_init() to initialize the ASes for CPU, instead
of setting cpu->num_ases directly and use cpu_address_space_add() to
add the required ASes dynamically.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/cpu.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1640b20b4d..c7151a2c1e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2148,31 +2148,23 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     unsigned int smp_cpus = ms->smp.cpus;
     bool has_secure = cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY);
 
-    /*
-     * We must set cs->num_ases to the final value before
-     * the first call to cpu_address_space_init.
-     */
-    if (cpu->tag_memory != NULL) {
-        cs->num_ases = 3 + has_secure;
-    } else {
-        cs->num_ases = 1 + has_secure;
-    }
+    cpu_address_space_init(cs, ARMASIdx_MAX);
 
-    cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);
+    cpu_address_space_add(cs, ARMASIdx_NS, "cpu-memory", cs->memory);
 
     if (has_secure) {
         if (!cpu->secure_memory) {
             cpu->secure_memory = cs->memory;
         }
-        cpu_address_space_init(cs, ARMASIdx_S, "cpu-secure-memory",
+        cpu_address_space_add(cs, ARMASIdx_S, "cpu-secure-memory",
                                cpu->secure_memory);
     }
 
     if (cpu->tag_memory != NULL) {
-        cpu_address_space_init(cs, ARMASIdx_TagNS, "cpu-tag-memory",
+        cpu_address_space_add(cs, ARMASIdx_TagNS, "cpu-tag-memory",
                                cpu->tag_memory);
         if (has_secure) {
-            cpu_address_space_init(cs, ARMASIdx_TagS, "cpu-tag-memory",
+            cpu_address_space_add(cs, ARMASIdx_TagS, "cpu-tag-memory",
                                    cpu->secure_tag_memory);
         }
     }
-- 
2.34.1

[PATCH v1 5/6] target/i386: Add a _MAX sentinel to X86ASIdx enum

[Gustavo Romero]

Add a sentinel to the X86ASIdx enum so it can be used when the total
number of address spaces is required.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/i386/cpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index cee1f692a1..783ad66d74 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2600,6 +2600,7 @@ void cpu_sync_avx_hflag(CPUX86State *env);
 typedef enum X86ASIdx {
     X86ASIdx_MEM = 0,
     X86ASIdx_SMM = 1,
+    X86ASIdx_MAX
 } X86ASIdx;
 
 #ifndef CONFIG_USER_ONLY
-- 
2.34.1

[PATCH v1 6/6] target/i386: Use new CPU address space API

[Gustavo Romero]

Use new cpu_addess_space_init() to initialize the ASes for CPU, instead
of setting cpu->num_ases directly and use cpu_address_space_add() to
add the required ASes dynamically.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/i386/kvm/kvm-cpu.c        | 4 ++--
 target/i386/kvm/kvm.c            | 4 ++--
 target/i386/tcg/system/tcg-cpu.c | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 9c25b55839..9b7364f1c6 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -98,8 +98,8 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      * Only initialize address space 0 here, the second one for SMM is
      * initialized at register_smram_listener() after machine init done.
      */
-    cs->num_ases = x86_machine_is_smm_enabled(X86_MACHINE(current_machine)) ? 2 : 1;
-    cpu_address_space_init(cs, X86ASIdx_MEM, "cpu-memory", cs->memory);
+    cpu_address_space_init(cs, X86ASIdx_MAX);
+    cpu_address_space_add(cs, X86ASIdx_MEM, "cpu-memory", cs->memory);
 
     return true;
 }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 60c7981138..dcc7e5eeae 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2744,14 +2744,14 @@ static void register_smram_listener(Notifier *n, void *unused)
                                  &smram_address_space, X86ASIdx_SMM, "kvm-smram");
 
     CPU_FOREACH(cpu) {
-        cpu_address_space_init(cpu, X86ASIdx_SMM, "cpu-smm", &smram_as_root);
+        cpu_address_space_add(cpu, X86ASIdx_SMM, "cpu-smm", &smram_as_root);
     }
 }
 
 /* It should only be called in cpu's hotplug callback */
 void kvm_smm_cpu_address_space_init(X86CPU *cpu)
 {
-    cpu_address_space_init(CPU(cpu), X86ASIdx_SMM, "cpu-smm", &smram_as_root);
+    cpu_address_space_add(CPU(cpu), X86ASIdx_SMM, "cpu-smm", &smram_as_root);
 }
 
 static void *kvm_msr_energy_thread(void *data)
diff --git a/target/i386/tcg/system/tcg-cpu.c b/target/i386/tcg/system/tcg-cpu.c
index 7255862c24..ab72c5ff7b 100644
--- a/target/i386/tcg/system/tcg-cpu.c
+++ b/target/i386/tcg/system/tcg-cpu.c
@@ -73,9 +73,9 @@ bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
     memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
     memory_region_set_enabled(cpu->cpu_as_mem, true);
 
-    cs->num_ases = 2;
-    cpu_address_space_init(cs, X86ASIdx_MEM, "cpu-memory", cs->memory);
-    cpu_address_space_init(cs, X86ASIdx_SMM, "cpu-smm", cpu->cpu_as_root);
+    cpu_address_space_init(cs, X86ASIdx_MAX);
+    cpu_address_space_add(cs, X86ASIdx_MEM, "cpu-memory", cs->memory);
+    cpu_address_space_add(cs, X86ASIdx_SMM, "cpu-smm", cpu->cpu_as_root);
 
     /* ... SMRAM with higher priority, linked from /machine/smram.  */
     cpu->machine_done.notify = tcg_cpu_machine_done;
-- 
2.34.1