[Qemu-devel] [PATCH 0/3] cpu: suffix cpu address spaces with indexes

[Qemu-devel] [PATCH 0/3] cpu: suffix cpu address spaces with indexes

[Peter Xu]

I picked up Alexey's patch to fix pci address space names (which is
possibly lost on the list), and refactored cpu address spaces a bit
more.  Now most of the address spaces (I believe some corner cases are
there) should have a valid and good name.  I think this is pretty safe
even for 2.11, but I'm fine too to delay this to 2.12.

Please review, thanks.

Alexey Kardashevskiy (1):
  pci: Initialize pci_dev->name before use

Peter Xu (2):
  cpu: refactor cpu_address_space_init()
  cpu: suffix cpu address spaces with cpu index

 cpus.c                  |  5 +----
 exec.c                  | 10 +++++++++-
 hw/pci/pci.c            |  2 +-
 include/exec/exec-all.h |  6 ++++--
 target/arm/cpu.c        | 13 +++----------
 target/i386/cpu.c       | 10 ++--------
 6 files changed, 20 insertions(+), 26 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] pci: Initialize pci_dev->name before use

[Peter Xu]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

This moves pci_dev->name initialization earlier so
pci_dev->bus_master_as could get a name instead of an empty string.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 5ed3c8dca4..b2d139bd9a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1030,6 +1030,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
     pci_dev->devfn = devfn;
     pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev);
+    pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
 
     memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev),
                        "bus master container", UINT64_MAX);
@@ -1039,7 +1040,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     if (qdev_hotplug) {
         pci_init_bus_master(pci_dev);
     }
-    pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
     pci_dev->irq_state = 0;
     pci_config_alloc(pci_dev);
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/3] pci: Initialize pci_dev->name before use

[Alexey Kardashevskiy]

On 23/11/17 20:23, Peter Xu wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> This moves pci_dev->name initialization earlier so
> pci_dev->bus_master_as could get a name instead of an empty string.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Peter Xu <peterx@redhat.com>


It is in upstream:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=d06bce95ff8c6e2f290db150695f826c3d57324f


> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 5ed3c8dca4..b2d139bd9a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1030,6 +1030,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  
>      pci_dev->devfn = devfn;
>      pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev);
> +    pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>  
>      memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev),
>                         "bus master container", UINT64_MAX);
> @@ -1039,7 +1040,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      if (qdev_hotplug) {
>          pci_init_bus_master(pci_dev);
>      }
> -    pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>      pci_dev->irq_state = 0;
>      pci_config_alloc(pci_dev);
>  
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/3] pci: Initialize pci_dev->name before use

[Peter Xu]

On Fri, Nov 24, 2017 at 11:16:53AM +1100, Alexey Kardashevskiy wrote:
> On 23/11/17 20:23, Peter Xu wrote:
> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > 
> > This moves pci_dev->name initialization earlier so
> > pci_dev->bus_master_as could get a name instead of an empty string.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> 
> It is in upstream:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=d06bce95ff8c6e2f290db150695f826c3d57324f

Again, I thought I was using a relatively new tree but I was not...  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH 2/3] cpu: refactor cpu_address_space_init()

[Peter Xu]

Normally we create an address space for that CPU and pass that address
space into the function.  Let's just do it inside to unify address space
creations.  It'll simplify my next patch to rename those address spaces.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus.c                  |  5 +----
 exec.c                  |  7 ++++++-
 include/exec/exec-all.h |  6 ++++--
 target/arm/cpu.c        | 13 +++----------
 target/i386/cpu.c       | 10 ++--------
 5 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/cpus.c b/cpus.c
index 114c29b6a0..ae9dbf9510 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1778,11 +1778,8 @@ void qemu_init_vcpu(CPUState *cpu)
         /* If the target cpu hasn't set up any address spaces itself,
          * give it the default one.
          */
-        AddressSpace *as = g_new0(AddressSpace, 1);
-
-        address_space_init(as, cpu->memory, "cpu-memory");
         cpu->num_ases = 1;
-        cpu_address_space_init(cpu, as, 0);
+        cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory);
     }
 
     if (kvm_enabled()) {
diff --git a/exec.c b/exec.c
index 8b579c0cd9..b4abe75931 100644
--- a/exec.c
+++ b/exec.c
@@ -714,9 +714,14 @@ CPUState *qemu_get_cpu(int index)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx)
+void cpu_address_space_init(CPUState *cpu, int asidx,
+                            const char *prefix, MemoryRegion *mr)
 {
     CPUAddressSpace *newas;
+    AddressSpace *as = g_new0(AddressSpace, 1);
+
+    assert(mr);
+    address_space_init(as, mr, prefix);
 
     /* Target code should have set num_ases before calling us */
     assert(asidx < cpu->num_ases);
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 0f51c92adb..b37f7d8d92 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -74,8 +74,9 @@ void cpu_reloading_memory_map(void);
 /**
  * cpu_address_space_init:
  * @cpu: CPU to add this address space to
- * @as: address space to add
  * @asidx: integer index of this address space
+ * @prefix: prefix to be used as name of address space
+ * @mr: the root memory region of address space
  *
  * Add the specified address space to the CPU's cpu_ases list.
  * The address space added with @asidx 0 is the one used for the
@@ -89,7 +90,8 @@ void cpu_reloading_memory_map(void);
  *
  * Note that with KVM only one address space is supported.
  */
-void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx);
+void cpu_address_space_init(CPUState *cpu, int asidx,
+                            const char *prefix, MemoryRegion *mr);
 #endif
 
 #if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7f7a3d1e32..cc1856c32b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -705,9 +705,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUARMState *env = &cpu->env;
     int pagebits;
     Error *local_err = NULL;
-#ifndef CONFIG_USER_ONLY
-    AddressSpace *as;
-#endif
 
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
@@ -912,21 +909,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     if (cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY)) {
-        as = g_new0(AddressSpace, 1);
-
         cs->num_ases = 2;
 
         if (!cpu->secure_memory) {
             cpu->secure_memory = cs->memory;
         }
-        address_space_init(as, cpu->secure_memory, "cpu-secure-memory");
-        cpu_address_space_init(cs, as, ARMASIdx_S);
+        cpu_address_space_init(cs, ARMASIdx_S, "cpu-secure-memory",
+                               cpu->secure_memory);
     } else {
         cs->num_ases = 1;
     }
-    as = g_new0(AddressSpace, 1);
-    address_space_init(as, cs->memory, "cpu-memory");
-    cpu_address_space_init(cs, as, ARMASIdx_NS);
+    cpu_address_space_init(cs, ARMASIdx_NS, "cpu-memory", cs->memory);
 #endif
 
     qemu_init_vcpu(cs);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1edcf29e27..7dd408cba8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3736,11 +3736,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     if (tcg_enabled()) {
-        AddressSpace *as_normal = g_new0(AddressSpace, 1);
-        AddressSpace *as_smm = g_new(AddressSpace, 1);
-
-        address_space_init(as_normal, cs->memory, "cpu-memory");
-
         cpu->cpu_as_mem = g_new(MemoryRegion, 1);
         cpu->cpu_as_root = g_new(MemoryRegion, 1);
 
@@ -3755,11 +3750,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                                  get_system_memory(), 0, ~0ull);
         memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
         memory_region_set_enabled(cpu->cpu_as_mem, true);
-        address_space_init(as_smm, cpu->cpu_as_root, "CPU");
 
         cs->num_ases = 2;
-        cpu_address_space_init(cs, as_normal, 0);
-        cpu_address_space_init(cs, as_smm, 1);
+        cpu_address_space_init(cs, 0, "cpu-memory", cs->memory);
+        cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_as_root);
 
         /* ... SMRAM with higher priority, linked from /machine/smram.  */
         cpu->machine_done.notify = x86_cpu_machine_done;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/3] cpu: refactor cpu_address_space_init()

[Paolo Bonzini]

On 23/11/2017 10:23, Peter Xu wrote:
> +                            const char *prefix, MemoryRegion *mr)
>  {
>      CPUAddressSpace *newas;
> +    AddressSpace *as = g_new0(AddressSpace, 1);
> +
> +    assert(mr);
> +    address_space_init(as, mr, prefix);
>  

Now that address_space_init_shareable doesn't exist anymore, the
AddressSpace could be placed directly in CPUAddressSpace instead of
having a pointer.  But the patches are already okay, that's a separate
change.

Paolo

Re: [Qemu-devel] [PATCH 2/3] cpu: refactor cpu_address_space_init()

[Peter Xu]

On Thu, Nov 23, 2017 at 05:06:31PM +0100, Paolo Bonzini wrote:
> On 23/11/2017 10:23, Peter Xu wrote:
> > +                            const char *prefix, MemoryRegion *mr)
> >  {
> >      CPUAddressSpace *newas;
> > +    AddressSpace *as = g_new0(AddressSpace, 1);
> > +
> > +    assert(mr);
> > +    address_space_init(as, mr, prefix);
> >  
> 
> Now that address_space_init_shareable doesn't exist anymore, the
> AddressSpace could be placed directly in CPUAddressSpace instead of
> having a pointer.  But the patches are already okay, that's a separate
> change.

Yes.  Let me append another patch and repost.  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH 3/3] cpu: suffix cpu address spaces with cpu index

[Peter Xu]

Renaming cpu address space names so that they won't be the same when
there are more than one.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index b4abe75931..c6e4c20c8f 100644
--- a/exec.c
+++ b/exec.c
@@ -719,9 +719,12 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 {
     CPUAddressSpace *newas;
     AddressSpace *as = g_new0(AddressSpace, 1);
+    char *as_name;
 
     assert(mr);
-    address_space_init(as, mr, prefix);
+    as_name = g_strdup_printf("%s-%d", prefix, cpu->cpu_index);
+    address_space_init(as, mr, as_name);
+    g_free(as_name);
 
     /* Target code should have set num_ases before calling us */
     assert(asidx < cpu->num_ases);
-- 
2.14.3