[PATCH 0/6] memory: Introduce address_space_create(), re-use &address_space_memory

[PATCH 0/6] memory: Introduce address_space_create(), re-use &address_space_memory

[Philippe Mathieu-Daudé]

Introduce address_space_create() (return .heap allocated AddressSpace)
and return directly &address_space_memory if the root MemoryRegion is
get_system_memory().

This simplifies the 'info mtree' output of some boards. Flatview is
unchanged.

Inspired by this thread:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg829819.html

Based-on: <20210819141527.2821842-1-philmd@redhat.com>

Philippe Mathieu-Daudé (6):
  memory: Do not increase refcount on global system_memory region
  memory: Introduce address_space_create()
  memory: Have cpu_address_space_init() use address_space_create()
  hw/dma: Replace alloc() + address_space_init() by
    address_space_create()
  hw/usb: Replace alloc() + address_space_init() by
    address_space_create()
  memory: Have address_space_create() re-use global
    &address_space_memory

 include/exec/memory.h    | 14 ++++++++++++++
 hw/dma/xlnx-zdma.c       | 15 +++++++++------
 hw/dma/xlnx_csu_dma.c    |  9 ++-------
 hw/usb/hcd-xhci-sysbus.c | 16 ++++++++++------
 softmmu/memory.c         | 24 ++++++++++++++++++++++--
 softmmu/physmem.c        |  4 ++--
 6 files changed, 59 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH 1/6] memory: Do not increase refcount on global system_memory region

[Philippe Mathieu-Daudé]

system_memory is statically allocated in memory_map_init()
(softmmu/physmem.c) and is never destroyed. No need to
increment its refcount.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 softmmu/memory.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..185f978c925 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -24,7 +24,7 @@
 #include "qemu/qemu-print.h"
 #include "qom/object.h"
 #include "trace.h"
-
+#include "exec/address-spaces.h"
 #include "exec/memory-internal.h"
 #include "exec/ram_addr.h"
 #include "sysemu/kvm.h"
@@ -2923,7 +2923,9 @@ void address_space_remove_listeners(AddressSpace *as)
 
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
-    memory_region_ref(root);
+    if (root != get_system_memory()) {
+        memory_region_ref(root);
+    }
     as->root = root;
     as->current_map = NULL;
     as->ioeventfd_nb = 0;
-- 
2.31.1

Re: [PATCH 1/6] memory: Do not increase refcount on global system_memory region

[Peter Maydell]

On Thu, 19 Aug 2021 at 15:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> system_memory is statically allocated in memory_map_init()
> (softmmu/physmem.c) and is never destroyed. No need to
> increment its refcount.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  softmmu/memory.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4df..185f978c925 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -24,7 +24,7 @@
>  #include "qemu/qemu-print.h"
>  #include "qom/object.h"
>  #include "trace.h"
> -
> +#include "exec/address-spaces.h"
>  #include "exec/memory-internal.h"
>  #include "exec/ram_addr.h"
>  #include "sysemu/kvm.h"
> @@ -2923,7 +2923,9 @@ void address_space_remove_listeners(AddressSpace *as)
>
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
> -    memory_region_ref(root);
> +    if (root != get_system_memory()) {
> +        memory_region_ref(root);
> +    }

...but there's no need to have an odd special in this code either,
is there? What harm does it do if the system memory MR has a lot of
references ?

-- PMM

[PATCH 2/6] memory: Introduce address_space_create()

[Philippe Mathieu-Daudé]

Introduce address_space_create(). In is similar to
address_space_init() but returns a pointer to a heap
allocated  AddressSpace.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 14 ++++++++++++++
 softmmu/memory.c      | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c3d417d317f..b353a48c25f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2418,6 +2418,20 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
  */
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
 
+/**
+ * address_space_create: Create and initializes an address space
+ *
+ * @root: a #MemoryRegion that routes addresses for the address space
+ * @name: an address space name.  The name is only used for debugging
+ *        output.
+ *
+ * Returns pointer to initialized #AddressSpace.
+ *
+ * The caller is responsible for releasing the pointer returned
+ * with address_space_destroy() after use.
+ */
+AddressSpace *address_space_create(MemoryRegion *root, const char *name);
+
 /**
  * address_space_destroy: destroy an address space
  *
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 185f978c925..16a2b518d8d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2937,6 +2937,16 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
     address_space_update_ioeventfds(as);
 }
 
+AddressSpace *address_space_create(MemoryRegion *root, const char *name)
+{
+    AddressSpace *as;
+
+    as = g_new(AddressSpace, 1);
+    address_space_init(as, root, name);
+
+    return as;
+}
+
 static void do_address_space_destroy(AddressSpace *as)
 {
     assert(QTAILQ_EMPTY(&as->listeners));
-- 
2.31.1

Re: [PATCH 2/6] memory: Introduce address_space_create()

[Peter Maydell]

On Thu, 19 Aug 2021 at 15:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Introduce address_space_create(). In is similar to
> address_space_init() but returns a pointer to a heap
> allocated  AddressSpace.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/exec/memory.h | 14 ++++++++++++++
>  softmmu/memory.c      | 10 ++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317f..b353a48c25f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2418,6 +2418,20 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>   */
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>
> +/**
> + * address_space_create: Create and initializes an address space
> + *
> + * @root: a #MemoryRegion that routes addresses for the address space
> + * @name: an address space name.  The name is only used for debugging
> + *        output.
> + *
> + * Returns pointer to initialized #AddressSpace.
> + *
> + * The caller is responsible for releasing the pointer returned
> + * with address_space_destroy() after use.
> + */
> +AddressSpace *address_space_create(MemoryRegion *root, const char *name);
> +

I'm not really a fan of this as an API -- almost always I think
devices would do better to have an AddressSpace foo field in
their device struct and call address_space_init() on that.
Hiding the heap allocation inside this function makes it harder
to notice it during code review, I think.

thanks
-- PMM

Re: [PATCH 2/6] memory: Introduce address_space_create()

[Philippe Mathieu-Daudé]

On 8/19/21 4:24 PM, Peter Maydell wrote:
> On Thu, 19 Aug 2021 at 15:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Introduce address_space_create(). In is similar to
>> address_space_init() but returns a pointer to a heap
>> allocated  AddressSpace.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/exec/memory.h | 14 ++++++++++++++
>>  softmmu/memory.c      | 10 ++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index c3d417d317f..b353a48c25f 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -2418,6 +2418,20 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>>   */
>>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>>
>> +/**
>> + * address_space_create: Create and initializes an address space
>> + *
>> + * @root: a #MemoryRegion that routes addresses for the address space
>> + * @name: an address space name.  The name is only used for debugging
>> + *        output.
>> + *
>> + * Returns pointer to initialized #AddressSpace.
>> + *
>> + * The caller is responsible for releasing the pointer returned
>> + * with address_space_destroy() after use.
>> + */
>> +AddressSpace *address_space_create(MemoryRegion *root, const char *name);
>> +
> 
> I'm not really a fan of this as an API -- almost always I think
> devices would do better to have an AddressSpace foo field in
> their device struct and call address_space_init() on that.
> Hiding the heap allocation inside this function makes it harder
> to notice it during code review, I think.

So I understand you rather I discard this (simple) approach and
rather modify 'info mtree' "was designed on the assumption that
there's really only one or two interesting address spaces." [*]

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg829821.html

[PATCH 3/6] memory: Have cpu_address_space_init() use address_space_create()

[Philippe Mathieu-Daudé]

Replace g_new0() + address_space_init() by address_space_create().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 softmmu/physmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3c1912a1a07..cd8b670a731 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -727,12 +727,12 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr)
 {
     CPUAddressSpace *newas;
-    AddressSpace *as = g_new0(AddressSpace, 1);
+    AddressSpace *as;
     char *as_name;
 
     assert(mr);
     as_name = g_strdup_printf("%s-%d", prefix, cpu->cpu_index);
-    address_space_init(as, mr, as_name);
+    as = address_space_create(mr, as_name);
     g_free(as_name);
 
     /* Target code should have set num_ases before calling us */
-- 
2.31.1

[PATCH 4/6] hw/dma: Replace alloc() + address_space_init() by address_space_create()

[Philippe Mathieu-Daudé]

Replace g_malloc0() + address_space_init() by address_space_create().
Release the resource in DeviceUnrealize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/dma/xlnx-zdma.c    | 15 +++++++++------
 hw/dma/xlnx_csu_dma.c |  9 ++-------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index fa38a556341..9f6b3fa47c6 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp)
         };
     }
 
-    if (s->dma_mr) {
-        s->dma_as = g_malloc0(sizeof(AddressSpace));
-        address_space_init(s->dma_as, s->dma_mr, NULL);
-    } else {
-        s->dma_as = &address_space_memory;
-    }
+    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
     s->attr = MEMTXATTRS_UNSPECIFIED;
 }
 
+static void zdma_unrealize(DeviceState *dev)
+{
+    XlnxZDMA *s = XLNX_ZDMA(dev);
+
+    address_space_destroy(s->dma_as);
+}
+
 static void zdma_init(Object *obj)
 {
     XlnxZDMA *s = XLNX_ZDMA(obj);
@@ -827,6 +829,7 @@ static void zdma_class_init(ObjectClass *klass, void *data)
 
     dc->reset = zdma_reset;
     dc->realize = zdma_realize;
+    dc->unrealize = zdma_unrealize;
     device_class_set_props(dc, zdma_props);
     dc->vmsd = &vmstate_zdma;
 }
diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 0c1c19cab5a..730c0f999ce 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -648,13 +648,7 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp)
     s->src_timer = ptimer_init(xlnx_csu_dma_src_timeout_hit,
                                s, PTIMER_POLICY_DEFAULT);
 
-    if (s->dma_mr) {
-        s->dma_as = g_malloc0(sizeof(AddressSpace));
-        address_space_init(s->dma_as, s->dma_mr, NULL);
-    } else {
-        s->dma_as = &address_space_memory;
-    }
-
+    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
     s->attr = MEMTXATTRS_UNSPECIFIED;
 
     s->r_size_last_word = 0;
@@ -665,6 +659,7 @@ static void xlnx_csu_dma_unrealize(DeviceState *dev)
     XlnxCSUDMA *s = XLNX_CSU_DMA(dev);
 
     ptimer_free(s->src_timer);
+    address_space_destroy(s->dma_as);
 }
 
 static const VMStateDescription vmstate_xlnx_csu_dma = {
-- 
2.31.1

Re: [PATCH 4/6] hw/dma: Replace alloc() + address_space_init() by address_space_create()

[Peter Maydell]

On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Replace g_malloc0() + address_space_init() by address_space_create().
> Release the resource in DeviceUnrealize().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/dma/xlnx-zdma.c    | 15 +++++++++------
>  hw/dma/xlnx_csu_dma.c |  9 ++-------
>  2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index fa38a556341..9f6b3fa47c6 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp)
>          };
>      }
>
> -    if (s->dma_mr) {
> -        s->dma_as = g_malloc0(sizeof(AddressSpace));
> -        address_space_init(s->dma_as, s->dma_mr, NULL);
> -    } else {
> -        s->dma_as = &address_space_memory;
> -    }
> +    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
>      s->attr = MEMTXATTRS_UNSPECIFIED;
>  }

Why are these devices doing a heap allocation rather than
having an AddressSpace whatever field in their device struct ?

thanks
-- PMM

Re: [PATCH 4/6] hw/dma: Replace alloc() + address_space_init() by address_space_create()

[Philippe Mathieu-Daudé]

On 8/19/21 4:22 PM, Peter Maydell wrote:
> On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Replace g_malloc0() + address_space_init() by address_space_create().
>> Release the resource in DeviceUnrealize().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/dma/xlnx-zdma.c    | 15 +++++++++------
>>  hw/dma/xlnx_csu_dma.c |  9 ++-------
>>  2 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
>> index fa38a556341..9f6b3fa47c6 100644
>> --- a/hw/dma/xlnx-zdma.c
>> +++ b/hw/dma/xlnx-zdma.c
>> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp)
>>          };
>>      }
>>
>> -    if (s->dma_mr) {
>> -        s->dma_as = g_malloc0(sizeof(AddressSpace));
>> -        address_space_init(s->dma_as, s->dma_mr, NULL);
>> -    } else {
>> -        s->dma_as = &address_space_memory;
>> -    }
>> +    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
>>      s->attr = MEMTXATTRS_UNSPECIFIED;
>>  }
> 
> Why are these devices doing a heap allocation rather than
> having an AddressSpace whatever field in their device struct ?

To easily use &address_space_memory if 'memory' link is NULL?

Re: [PATCH 4/6] hw/dma: Replace alloc() + address_space_init() by address_space_create()

[Peter Maydell]

On Thu, 19 Aug 2021 at 15:32, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/19/21 4:22 PM, Peter Maydell wrote:
> > On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>
> >> Replace g_malloc0() + address_space_init() by address_space_create().
> >> Release the resource in DeviceUnrealize().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  hw/dma/xlnx-zdma.c    | 15 +++++++++------
> >>  hw/dma/xlnx_csu_dma.c |  9 ++-------
> >>  2 files changed, 11 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> >> index fa38a556341..9f6b3fa47c6 100644
> >> --- a/hw/dma/xlnx-zdma.c
> >> +++ b/hw/dma/xlnx-zdma.c
> >> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp)
> >>          };
> >>      }
> >>
> >> -    if (s->dma_mr) {
> >> -        s->dma_as = g_malloc0(sizeof(AddressSpace));
> >> -        address_space_init(s->dma_as, s->dma_mr, NULL);
> >> -    } else {
> >> -        s->dma_as = &address_space_memory;
> >> -    }
> >> +    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
> >>      s->attr = MEMTXATTRS_UNSPECIFIED;
> >>  }
> >
> > Why are these devices doing a heap allocation rather than
> > having an AddressSpace whatever field in their device struct ?
>
> To easily use &address_space_memory if 'memory' link is NULL?

They could do that with
    AddressSpace our_as;
    AddressSpace *dma_as;

and set dma_as to &s->our_as or &address_space_memory.

Or we could fix the two boards which create these devices to always
pass in an MR to use for DMA and drop the conditionality.

-- PMM

Re: [PATCH 4/6] hw/dma: Replace alloc() + address_space_init() by address_space_create()

[Philippe Mathieu-Daudé]

On 8/19/21 4:38 PM, Peter Maydell wrote:
> On Thu, 19 Aug 2021 at 15:32, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 8/19/21 4:22 PM, Peter Maydell wrote:
>>> On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>> Replace g_malloc0() + address_space_init() by address_space_create().
>>>> Release the resource in DeviceUnrealize().
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/dma/xlnx-zdma.c    | 15 +++++++++------
>>>>  hw/dma/xlnx_csu_dma.c |  9 ++-------
>>>>  2 files changed, 11 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
>>>> index fa38a556341..9f6b3fa47c6 100644
>>>> --- a/hw/dma/xlnx-zdma.c
>>>> +++ b/hw/dma/xlnx-zdma.c
>>>> @@ -777,15 +777,17 @@ static void zdma_realize(DeviceState *dev, Error **errp)
>>>>          };
>>>>      }
>>>>
>>>> -    if (s->dma_mr) {
>>>> -        s->dma_as = g_malloc0(sizeof(AddressSpace));
>>>> -        address_space_init(s->dma_as, s->dma_mr, NULL);
>>>> -    } else {
>>>> -        s->dma_as = &address_space_memory;
>>>> -    }
>>>> +    s->dma_as = address_space_create(s->dma_mr ?: get_system_memory(), NULL);
>>>>      s->attr = MEMTXATTRS_UNSPECIFIED;
>>>>  }
>>>
>>> Why are these devices doing a heap allocation rather than
>>> having an AddressSpace whatever field in their device struct ?
>>
>> To easily use &address_space_memory if 'memory' link is NULL?
> 
> They could do that with
>     AddressSpace our_as;
>     AddressSpace *dma_as;
> 
> and set dma_as to &s->our_as or &address_space_memory.
> 
> Or we could fix the two boards which create these devices to always
> pass in an MR to use for DMA and drop the conditionality.

Clever, thanks.

[PATCH 5/6] hw/usb: Replace alloc() + address_space_init() by address_space_create()

[Philippe Mathieu-Daudé]

Replace g_malloc0() + address_space_init() by address_space_create().
Release the resource in DeviceUnrealize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/hcd-xhci-sysbus.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c
index a14e4381960..04ac485e8b3 100644
--- a/hw/usb/hcd-xhci-sysbus.c
+++ b/hw/usb/hcd-xhci-sysbus.c
@@ -43,16 +43,19 @@ static void xhci_sysbus_realize(DeviceState *dev, Error **errp)
     s->irq = g_new0(qemu_irq, s->xhci.numintrs);
     qdev_init_gpio_out_named(dev, s->irq, SYSBUS_DEVICE_GPIO_IRQ,
                              s->xhci.numintrs);
-    if (s->xhci.dma_mr) {
-        s->xhci.as =  g_malloc0(sizeof(AddressSpace));
-        address_space_init(s->xhci.as, s->xhci.dma_mr, NULL);
-    } else {
-        s->xhci.as = &address_space_memory;
-    }
+    s->xhci.as = address_space_create(s->xhci.dma_mr ?: get_system_memory(),
+                                      NULL);
 
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->xhci.mem);
 }
 
+static void xhci_sysbus_unrealize(DeviceState *dev)
+{
+    XHCISysbusState *s = XHCI_SYSBUS(dev);
+
+    address_space_destroy(s->xhci.as);
+}
+
 static void xhci_sysbus_instance_init(Object *obj)
 {
     XHCISysbusState *s = XHCI_SYSBUS(obj);
@@ -103,6 +106,7 @@ static void xhci_sysbus_class_init(ObjectClass *klass, void *data)
 
     dc->reset = xhci_sysbus_reset;
     dc->realize = xhci_sysbus_realize;
+    dc->unrealize = xhci_sysbus_unrealize;
     dc->vmsd = &vmstate_xhci_sysbus;
     device_class_set_props(dc, xhci_sysbus_props);
 }
-- 
2.31.1

[RFC PATCH 6/6] memory: Have address_space_create() re-use global &address_space_memory

[Philippe Mathieu-Daudé]

We already have a global AddressSpace created along with the
global get_system_memory(): address_space_memory. Return it
directly instead of creating the same AS with a different name.

This drastically reduce 'info mtree' on some boards (diff -c100):

  (echo info mtree; echo q) | ./qemu-system-aarch64 -S -monitor stdio -M raspi3b
  QEMU 6.0.93 monitor - type 'help' for more information
  (qemu) info mtree
  address-space: memory
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-000000003fffffff (prio 0, ram): ram
      000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
        000000003f003000-000000003f00301f (prio 0, i/o): bcm2835-sys-timer
        000000003f004000-000000003f004fff (prio -1000, i/o): bcm2835-txp
        000000003f006000-000000003f006fff (prio 0, i/o): mphi
        000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
        000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
        000000003f00b400-000000003f00b43f (prio -1000, i/o): bcm2835-sp804
        000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
        000000003f100000-000000003f1001ff (prio 0, i/o): bcm2835-powermgt
        000000003f101000-000000003f102fff (prio 0, i/o): bcm2835-cprman
        000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
        000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
        000000003f201000-000000003f201fff (prio 0, i/o): pl011
        000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
        000000003f203000-000000003f2030ff (prio -1000, i/o): bcm2835-i2s
        000000003f204000-000000003f20401f (prio -1000, i/o): bcm2835-spi0
        000000003f205000-000000003f20501f (prio -1000, i/o): bcm2835-i2c0
        000000003f20f000-000000003f20f07f (prio -1000, i/o): bcm2835-otp
        000000003f212000-000000003f212007 (prio 0, i/o): bcm2835-thermal
        000000003f214000-000000003f2140ff (prio -1000, i/o): bcm2835-spis
        000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
        000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
        000000003f600000-000000003f6000ff (prio -1000, i/o): bcm2835-smi
        000000003f804000-000000003f80401f (prio -1000, i/o): bcm2835-i2c1
        000000003f805000-000000003f80501f (prio -1000, i/o): bcm2835-i2c2
        000000003f900000-000000003f907fff (prio -1000, i/o): bcm2835-dbus
        000000003f910000-000000003f917fff (prio -1000, i/o): bcm2835-ave0
        000000003f980000-000000003f990fff (prio 0, i/o): dwc2
          000000003f980000-000000003f980fff (prio 0, i/o): dwc2-io
          000000003f981000-000000003f990fff (prio 0, i/o): dwc2-fifo
        000000003fc00000-000000003fc00fff (prio -1000, i/o): bcm2835-v3d
        000000003fe00000-000000003fe000ff (prio -1000, i/o): bcm2835-sdramc
        000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15
      0000000040000000-00000000400000ff (prio 0, i/o): bcm2836-control

  address-space: I/O
    0000000000000000-000000000000ffff (prio 0, i/o): io

  address-space: bcm2835-mbox-memory
    0000000000000000-000000000000008f (prio 0, i/o): bcm2835-mbox
      0000000000000010-000000000000001f (prio 0, i/o): bcm2835-fb
      0000000000000080-000000000000008f (prio 0, i/o): bcm2835-property

  address-space: bcm2835-fb-memory
    0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
      0000000000000000-000000003fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      0000000040000000-000000007fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
      0000000080000000-00000000bfffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      00000000c0000000-00000000ffffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff

  address-space: bcm2835-property-memory
    0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
      0000000000000000-000000003fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      0000000040000000-000000007fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
      0000000080000000-00000000bfffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      00000000c0000000-00000000ffffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff

  address-space: bcm2835-dma-memory
    0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
      0000000000000000-000000003fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      0000000040000000-000000007fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
      0000000080000000-00000000bfffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      00000000c0000000-00000000ffffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff

  address-space: dwc2
    0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
      0000000000000000-000000003fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      0000000040000000-000000007fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
      0000000080000000-00000000bfffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
      00000000c0000000-00000000ffffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff

- address-space: cpu-secure-memory-0
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
-     0000000000000000-000000003fffffff (prio 0, ram): ram
-     000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
-       000000003f003000-000000003f00301f (prio 0, i/o): bcm2835-sys-timer
-       000000003f004000-000000003f004fff (prio -1000, i/o): bcm2835-txp
-       000000003f006000-000000003f006fff (prio 0, i/o): mphi
-       000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
-       000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
-       000000003f00b400-000000003f00b43f (prio -1000, i/o): bcm2835-sp804
-       000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
-       000000003f100000-000000003f1001ff (prio 0, i/o): bcm2835-powermgt
-       000000003f101000-000000003f102fff (prio 0, i/o): bcm2835-cprman
-       000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
-       000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
-       000000003f201000-000000003f201fff (prio 0, i/o): pl011
-       000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
-       000000003f203000-000000003f2030ff (prio -1000, i/o): bcm2835-i2s
-       000000003f204000-000000003f20401f (prio -1000, i/o): bcm2835-spi0
-       000000003f205000-000000003f20501f (prio -1000, i/o): bcm2835-i2c0
-       000000003f20f000-000000003f20f07f (prio -1000, i/o): bcm2835-otp
-       000000003f212000-000000003f212007 (prio 0, i/o): bcm2835-thermal
-       000000003f214000-000000003f2140ff (prio -1000, i/o): bcm2835-spis
-       000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
-       000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
-       000000003f600000-000000003f6000ff (prio -1000, i/o): bcm2835-smi
-       000000003f804000-000000003f80401f (prio -1000, i/o): bcm2835-i2c1
-       000000003f805000-000000003f80501f (prio -1000, i/o): bcm2835-i2c2
-       000000003f900000-000000003f907fff (prio -1000, i/o): bcm2835-dbus
-       000000003f910000-000000003f917fff (prio -1000, i/o): bcm2835-ave0
-       000000003f980000-000000003f990fff (prio 0, i/o): dwc2
-         000000003f980000-000000003f980fff (prio 0, i/o): dwc2-io
-         000000003f981000-000000003f990fff (prio 0, i/o): dwc2-fifo
-       000000003fc00000-000000003fc00fff (prio -1000, i/o): bcm2835-v3d
-       000000003fe00000-000000003fe000ff (prio -1000, i/o): bcm2835-sdramc
-       000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15
-     0000000040000000-00000000400000ff (prio 0, i/o): bcm2836-control
-
- address-space: cpu-memory-0
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-secure-memory-1
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-memory-1
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-secure-memory-2
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-memory-2
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-secure-memory-3
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
- address-space: cpu-memory-3
-   0000000000000000-ffffffffffffffff (prio 0, i/o): system
    [...]
-
  memory-region: ram
    0000000000000000-000000003fffffff (prio 0, ram): ram

  memory-region: bcm2835-peripherals
    000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
      000000003f003000-000000003f00301f (prio 0, i/o): bcm2835-sys-timer
      000000003f004000-000000003f004fff (prio -1000, i/o): bcm2835-txp
      000000003f006000-000000003f006fff (prio 0, i/o): mphi
      000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
      000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
      000000003f00b400-000000003f00b43f (prio -1000, i/o): bcm2835-sp804
      000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
      000000003f100000-000000003f1001ff (prio 0, i/o): bcm2835-powermgt
      000000003f101000-000000003f102fff (prio 0, i/o): bcm2835-cprman
      000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
      000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
      000000003f201000-000000003f201fff (prio 0, i/o): pl011
      000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
      000000003f203000-000000003f2030ff (prio -1000, i/o): bcm2835-i2s
      000000003f204000-000000003f20401f (prio -1000, i/o): bcm2835-spi0
      000000003f205000-000000003f20501f (prio -1000, i/o): bcm2835-i2c0
      000000003f20f000-000000003f20f07f (prio -1000, i/o): bcm2835-otp
      000000003f212000-000000003f212007 (prio 0, i/o): bcm2835-thermal
      000000003f214000-000000003f2140ff (prio -1000, i/o): bcm2835-spis
      000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
      000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
      000000003f600000-000000003f6000ff (prio -1000, i/o): bcm2835-smi
      000000003f804000-000000003f80401f (prio -1000, i/o): bcm2835-i2c1
      000000003f805000-000000003f80501f (prio -1000, i/o): bcm2835-i2c2
      000000003f900000-000000003f907fff (prio -1000, i/o): bcm2835-dbus
      000000003f910000-000000003f917fff (prio -1000, i/o): bcm2835-ave0
      000000003f980000-000000003f990fff (prio 0, i/o): dwc2
        000000003f980000-000000003f980fff (prio 0, i/o): dwc2-io
        000000003f981000-000000003f990fff (prio 0, i/o): dwc2-fifo
      000000003fc00000-000000003fc00fff (prio -1000, i/o): bcm2835-v3d
      000000003fe00000-000000003fe000ff (prio -1000, i/o): bcm2835-sdramc
      000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15

  (qemu) q

Inspired-by: Jianxian Wen <jianxian.wen@verisilicon.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Note, we loose the specific description of the duplicated
address spaces, but this doesn't seem very useful in this
output, it is rather more confusing IMO.
---
 softmmu/memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 16a2b518d8d..e4506b5a0d5 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2941,6 +2941,10 @@ AddressSpace *address_space_create(MemoryRegion *root, const char *name)
 {
     AddressSpace *as;
 
+    if (root == get_system_memory()) {
+        return &address_space_memory;
+    }
+
     as = g_new(AddressSpace, 1);
     address_space_init(as, root, name);
 
@@ -2961,6 +2965,10 @@ void address_space_destroy(AddressSpace *as)
 {
     MemoryRegion *root = as->root;
 
+    if (as == &address_space_memory) {
+        return;
+    }
+
     /* Flush out anything from MemoryListeners listening in on this */
     memory_region_transaction_begin();
     as->root = NULL;
-- 
2.31.1

Re: [RFC PATCH 6/6] memory: Have address_space_create() re-use global &address_space_memory

[Peter Maydell]

On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> We already have a global AddressSpace created along with the
> global get_system_memory(): address_space_memory. Return it
> directly instead of creating the same AS with a different name.
>

> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 16a2b518d8d..e4506b5a0d5 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2941,6 +2941,10 @@ AddressSpace *address_space_create(MemoryRegion *root, const char *name)
>  {
>      AddressSpace *as;
>
> +    if (root == get_system_memory()) {
> +        return &address_space_memory;
> +    }

But most ASes aren't set up with address_space_create().
This doesn't do anything for the common case where the
AS is initialized with address_space_init().

This also seems to me to be the tail wagging the dog. If we think
'info mtree' has too much duplicate information (which it certainly
does) then we should make mtree_info() smarter about reducing that
duplication. Off the top of my head, we could change the code that
prints ASes to do something like:

   hashtable = an empty hashtable;
   QEMU_FOREACH(as, ...) {
       qemu_printf("address-space: %s\n", as->name);
       name = lookup as->root in hashtable;
       if (name) {
           qemu_printf("...same as address-space %s\n", name);
           continue;
       }
       add (as->root, as->name) to hashtable;
       mtree_print_mr(as->root...);
       qemu_printf("\n");
   }

thanks
-- PMM

Re: [RFC PATCH 6/6] memory: Have address_space_create() re-use global &address_space_memory

[Philippe Mathieu-Daudé]

On 8/19/21 4:34 PM, Peter Maydell wrote:
> On Thu, 19 Aug 2021 at 15:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> We already have a global AddressSpace created along with the
>> global get_system_memory(): address_space_memory. Return it
>> directly instead of creating the same AS with a different name.
>>
> 
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 16a2b518d8d..e4506b5a0d5 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -2941,6 +2941,10 @@ AddressSpace *address_space_create(MemoryRegion *root, const char *name)
>>  {
>>      AddressSpace *as;
>>
>> +    if (root == get_system_memory()) {
>> +        return &address_space_memory;
>> +    }
> 
> But most ASes aren't set up with address_space_create().
> This doesn't do anything for the common case where the
> AS is initialized with address_space_init().
> 
> This also seems to me to be the tail wagging the dog. If we think
> 'info mtree' has too much duplicate information (which it certainly
> does) then we should make mtree_info() smarter about reducing that
> duplication. Off the top of my head, we could change the code that
> prints ASes to do something like:
> 
>    hashtable = an empty hashtable;
>    QEMU_FOREACH(as, ...) {
>        qemu_printf("address-space: %s\n", as->name);
>        name = lookup as->root in hashtable;
>        if (name) {
>            qemu_printf("...same as address-space %s\n", name);
>            continue;
>        }
>        add (as->root, as->name) to hashtable;
>        mtree_print_mr(as->root...);
>        qemu_printf("\n");
>    }

Got it, thanks for the review, explanation & suggestion :)

Re: [RFC PATCH 6/6] memory: Have address_space_create() re-use global &address_space_memory

[Gerd Hoffmann]

  Hi,

> This also seems to me to be the tail wagging the dog. If we think
> 'info mtree' has too much duplicate information (which it certainly
> does) then we should make mtree_info() smarter about reducing that
> duplication. Off the top of my head, we could change the code that
> prints ASes to do something like:

>            qemu_printf("...same as address-space %s\n", name);

Neat idea.

Having 'info mtree' accept an (optional) 'name' parameter to pick an
address space to be printed would be useful too.

take care,
  Gerd

Re: [RFC PATCH 6/6] memory: Have address_space_create() re-use global &address_space_memory

[Philippe Mathieu-Daudé]

On 8/20/21 8:07 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>> This also seems to me to be the tail wagging the dog. If we think
>> 'info mtree' has too much duplicate information (which it certainly
>> does) then we should make mtree_info() smarter about reducing that
>> duplication. Off the top of my head, we could change the code that
>> prints ASes to do something like:
> 
>>            qemu_printf("...same as address-space %s\n", name);
> 
> Neat idea.
> 
> Having 'info mtree' accept an (optional) 'name' parameter to pick an
> address space to be printed would be useful too.

Yeah, for now I am thinking of a match string (for my use cases):

  (qemu) info mtree -a dma # all address spaces matching *dma*