[Qemu-devel] [PATCH 1/1] Added address_space_init2().

[Qemu-devel] [PATCH 1/1] Added address_space_init2().

[Alexander Barabash]

address_space_init2: initializes a named address space.

Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
---
 include/exec/memory.h |    9 +++++++++
 memory.c              |    6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..8f8a31d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -820,6 +820,15 @@ void mtree_info(fprintf_function mon_printf, void *f);
  */
 void address_space_init(AddressSpace *as, MemoryRegion *root);
 
+/**
+ * address_space_init2: initializes a named address space
+ *
+ * @as: an uninitialized #AddressSpace
+ * @root: a #MemoryRegion that routes addesses for the address space
+ * @name: used for debugging
+ */
+void address_space_init2(AddressSpace *as, MemoryRegion *root,
+                         const char *name);
 
 /**
  * address_space_destroy: destroy an address space
diff --git a/memory.c b/memory.c
index 410c5f8..1652c10 100644
--- a/memory.c
+++ b/memory.c
@@ -1574,6 +1574,12 @@ void address_space_init(AddressSpace *as, MemoryRegion *root)
     address_space_init_dispatch(as);
 }
 
+void address_space_init2(AddressSpace *as, MemoryRegion *root, const char *name)
+{
+    address_space_init(as, root);
+    as->name = g_strdup(name);
+}
+
 void address_space_destroy(AddressSpace *as)
 {
     /* Flush out anything from MemoryListeners listening in on this */
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/1] Added address_space_init2().

[Andreas Färber]

A "memory: " prefix in the subject would've been nice for filtering.

Am 07.01.2013 13:07, schrieb Alexander Barabash:
> address_space_init2: initializes a named address space.

What for? There are no users in this patch that justify its utility over
setting the field manually.

The name is really awful, maybe ..._init_with_name or add a name
argument to the existing function and let the existing callers pass NULL?

Regards,
Andreas

> 
> Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
> ---
>  include/exec/memory.h |    9 +++++++++
>  memory.c              |    6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2322732..8f8a31d 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -820,6 +820,15 @@ void mtree_info(fprintf_function mon_printf, void *f);
>   */
>  void address_space_init(AddressSpace *as, MemoryRegion *root);
>  
> +/**
> + * address_space_init2: initializes a named address space
> + *
> + * @as: an uninitialized #AddressSpace
> + * @root: a #MemoryRegion that routes addesses for the address space
> + * @name: used for debugging
> + */
> +void address_space_init2(AddressSpace *as, MemoryRegion *root,
> +                         const char *name);
>  
>  /**
>   * address_space_destroy: destroy an address space
> diff --git a/memory.c b/memory.c
> index 410c5f8..1652c10 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1574,6 +1574,12 @@ void address_space_init(AddressSpace *as, MemoryRegion *root)
>      address_space_init_dispatch(as);
>  }
>  
> +void address_space_init2(AddressSpace *as, MemoryRegion *root, const char *name)
> +{
> +    address_space_init(as, root);
> +    as->name = g_strdup(name);
> +}
> +
>  void address_space_destroy(AddressSpace *as)
>  {
>      /* Flush out anything from MemoryListeners listening in on this */

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/1] Added address_space_init2().

[Alexander Barabash]

Hi,
On 01/07/2013 03:05 PM, Andreas Färber wrote:
> A "memory: " prefix in the subject would've been nice for filtering.
OK
>
> Am 07.01.2013 13:07, schrieb Alexander Barabash:
>> address_space_init2: initializes a named address space.
> What for? There are no users in this patch that justify its utility over
> setting the field manually.
Names of memory regions and address spaces are used for debugging purposes.
Currently, there is no way for address spaces other than "memory" and 
"io" to
appear in debugging output.

This is used in some code to be posted later.

>
> The name is really awful, maybe ..._init_with_name or add a name
> argument to the existing function and let the existing callers pass NULL?
Changing the existing callers of address_space_init() is OK with me.
I did not do that primarily because that would be an obvious way to do
to start with. And since this was not implemented this way,
there should have been some some reason.

However, I'll send a PATCH along these lines.

>
> Regards,
> Andreas

Regards,
Alex

>
>> Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
>> ---
>>   include/exec/memory.h |    9 +++++++++
>>   memory.c              |    6 ++++++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 2322732..8f8a31d 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -820,6 +820,15 @@ void mtree_info(fprintf_function mon_printf, void *f);
>>    */
>>   void address_space_init(AddressSpace *as, MemoryRegion *root);
>>   
>> +/**
>> + * address_space_init2: initializes a named address space
>> + *
>> + * @as: an uninitialized #AddressSpace
>> + * @root: a #MemoryRegion that routes addesses for the address space
>> + * @name: used for debugging
>> + */
>> +void address_space_init2(AddressSpace *as, MemoryRegion *root,
>> +                         const char *name);
>>   
>>   /**
>>    * address_space_destroy: destroy an address space
>> diff --git a/memory.c b/memory.c
>> index 410c5f8..1652c10 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1574,6 +1574,12 @@ void address_space_init(AddressSpace *as, MemoryRegion *root)
>>       address_space_init_dispatch(as);
>>   }
>>   
>> +void address_space_init2(AddressSpace *as, MemoryRegion *root, const char *name)
>> +{
>> +    address_space_init(as, root);
>> +    as->name = g_strdup(name);
>> +}
>> +
>>   void address_space_destroy(AddressSpace *as)
>>   {
>>       /* Flush out anything from MemoryListeners listening in on this */

[Qemu-devel] [PATCH 1/1] memory: add name in AddressSpace initialization.

[Alexander Barabash]

Pass the AddressSpace's name (to be used for debugging)
to address_space_init(). If NULL is passed, the name
of root memory region is used instead.

Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
---
 exec.c                |    6 ++----
 hw/pci/pci.c          |    3 ++-
 include/exec/memory.h |    5 +++--
 memory.c              |    5 +++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/exec.c b/exec.c
index a6923ad..4637c28 100644
--- a/exec.c
+++ b/exec.c
@@ -1784,13 +1784,11 @@ static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
     memory_region_init(system_memory, "system", INT64_MAX);
-    address_space_init(&address_space_memory, system_memory);
-    address_space_memory.name = "memory";
+    address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
     memory_region_init(system_io, "io", 65536);
-    address_space_init(&address_space_io, system_io);
-    address_space_io.name = "I/O";
+    address_space_init(&address_space_io, system_io, "I/O");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
     memory_listener_register(&io_memory_listener, &address_space_io);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 94840c4..6cc3abe 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -790,7 +790,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                  get_system_memory(), 0,
                                  memory_region_size(get_system_memory()));
         memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
-        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
+        address_space_init(&pci_dev->bus_master_as,
+                           &pci_dev->bus_master_enable_region, NULL);
         pci_dev->dma = g_new(DMAContext, 1);
         dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
     }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..cb79a65 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -817,9 +817,10 @@ void mtree_info(fprintf_function mon_printf, void *f);
  *
  * @as: an uninitialized #AddressSpace
  * @root: a #MemoryRegion that routes addesses for the address space
+ * @name: used for debugging. If NULL, name of the root memory region is used.
  */
-void address_space_init(AddressSpace *as, MemoryRegion *root);
-
+void address_space_init(AddressSpace *as, MemoryRegion *root,
+                         const char *name);
 
 /**
  * address_space_destroy: destroy an address space
diff --git a/memory.c b/memory.c
index 410c5f8..ab3b136 100644
--- a/memory.c
+++ b/memory.c
@@ -1562,14 +1562,14 @@ void memory_listener_unregister(MemoryListener *listener)
     QTAILQ_REMOVE(&memory_listeners, listener, link);
 }
 
-void address_space_init(AddressSpace *as, MemoryRegion *root)
+void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_transaction_begin();
     as->root = root;
     as->current_map = g_new(FlatView, 1);
     flatview_init(as->current_map);
     QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
-    as->name = NULL;
+    as->name = g_strdup(name ? name : root->name);
     memory_region_transaction_commit();
     address_space_init_dispatch(as);
 }
@@ -1584,6 +1584,7 @@ void address_space_destroy(AddressSpace *as)
     address_space_destroy_dispatch(as);
     flatview_destroy(as->current_map);
     g_free(as->current_map);
+    g_free((char *)as->name);
 }
 
 uint64_t io_mem_read(MemoryRegion *mr, hwaddr addr, unsigned size)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/1] memory: add name in AddressSpace initialization.

[Anthony Liguori]

Alexander Barabash <alexander_barabash@mentor.com> writes:

> Pass the AddressSpace's name (to be used for debugging)
> to address_space_init(). If NULL is passed, the name
> of root memory region is used instead.
>
> Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
> ---
>  exec.c                |    6 ++----
>  hw/pci/pci.c          |    3 ++-
>  include/exec/memory.h |    5 +++--
>  memory.c              |    5 +++--
>  4 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a6923ad..4637c28 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1784,13 +1784,11 @@ static void memory_map_init(void)
>  {
>      system_memory = g_malloc(sizeof(*system_memory));
>      memory_region_init(system_memory, "system", INT64_MAX);
> -    address_space_init(&address_space_memory, system_memory);
> -    address_space_memory.name = "memory";
> +    address_space_init(&address_space_memory, system_memory, "memory");
>  
>      system_io = g_malloc(sizeof(*system_io));
>      memory_region_init(system_io, "io", 65536);
> -    address_space_init(&address_space_io, system_io);
> -    address_space_io.name = "I/O";
> +    address_space_init(&address_space_io, system_io, "I/O");

It's best to avoid special characters in names.  It will make a
conversion to QOM later a lot easier.  So s:I/O:IO:g'

>  
>      memory_listener_register(&core_memory_listener, &address_space_memory);
>      memory_listener_register(&io_memory_listener, &address_space_io);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 94840c4..6cc3abe 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -790,7 +790,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                   get_system_memory(), 0,
>                                   memory_region_size(get_system_memory()));
>          memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
> -        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
> +        address_space_init(&pci_dev->bus_master_as,
> +                           &pci_dev->bus_master_enable_region, NULL);

Please top-post new patches.

Any reason to not name this address space?

Regards,

Anthony Liguori


>          pci_dev->dma = g_new(DMAContext, 1);
>          dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
>      }
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2322732..cb79a65 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -817,9 +817,10 @@ void mtree_info(fprintf_function mon_printf, void *f);
>   *
>   * @as: an uninitialized #AddressSpace
>   * @root: a #MemoryRegion that routes addesses for the address space
> + * @name: used for debugging. If NULL, name of the root memory region is used.
>   */
> -void address_space_init(AddressSpace *as, MemoryRegion *root);
> -
> +void address_space_init(AddressSpace *as, MemoryRegion *root,
> +                         const char *name);
>  
>  /**
>   * address_space_destroy: destroy an address space
> diff --git a/memory.c b/memory.c
> index 410c5f8..ab3b136 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1562,14 +1562,14 @@ void memory_listener_unregister(MemoryListener *listener)
>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>  }
>  
> -void address_space_init(AddressSpace *as, MemoryRegion *root)
> +void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
>      memory_region_transaction_begin();
>      as->root = root;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
>      QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
> -    as->name = NULL;
> +    as->name = g_strdup(name ? name : root->name);
>      memory_region_transaction_commit();
>      address_space_init_dispatch(as);
>  }
> @@ -1584,6 +1584,7 @@ void address_space_destroy(AddressSpace *as)
>      address_space_destroy_dispatch(as);
>      flatview_destroy(as->current_map);
>      g_free(as->current_map);
> +    g_free((char *)as->name);
>  }
>  
>  uint64_t io_mem_read(MemoryRegion *mr, hwaddr addr, unsigned size)
> -- 
> 1.7.9.5

Re: [Qemu-devel] [PATCH 1/1] memory: add name in AddressSpace initialization.

[Alexander Barabash]

On 01/08/2013 04:00 PM, Anthony Liguori wrote:
> Alexander Barabash <alexander_barabash@mentor.com> writes:
>
>> Pass the AddressSpace's name (to be used for debugging)
>> to address_space_init(). If NULL is passed, the name
>> of root memory region is used instead.
>>
>> @@ -1784,13 +1784,11 @@ static void memory_map_init(void)
...
>>       memory_region_init(system_io, "io", 65536);
>> -    address_space_init(&address_space_io, system_io);
>> -    address_space_io.name = "I/O";
>> +    address_space_init(&address_space_io, system_io, "I/O");
> It's best to avoid special characters in names.  It will make a
> conversion to QOM later a lot easier.  So s:I/O:IO:g'

I did not change the name of this address space.
We can do it in a separate patch.
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -790,7 +790,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>                                    get_system_memory(), 0,
>>                                    memory_region_size(get_system_memory()));
>>           memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
>> -        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
>> +        address_space_init(&pci_dev->bus_master_as,
>> +                           &pci_dev->bus_master_enable_region, NULL);
> Please top-post new patches.

I assume you this to mean, "post new patches as reply to the previous 
one". ) OK

> Any reason to not name this address space?
It was not named previously. Now, it will pick up the name from the root 
memory region,
passed to the same call.
> Regards,
>
> Anthony Liguori

Regards,
Alex Barabash

>
>
>>           pci_dev->dma = g_new(DMAContext, 1);
>>           dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
>>       }
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 2322732..cb79a65 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -817,9 +817,10 @@ void mtree_info(fprintf_function mon_printf, void *f);
>>    *
>>    * @as: an uninitialized #AddressSpace
>>    * @root: a #MemoryRegion that routes addesses for the address space
>> + * @name: used for debugging. If NULL, name of the root memory region is used.
>>    */
>> -void address_space_init(AddressSpace *as, MemoryRegion *root);
>> -
>> +void address_space_init(AddressSpace *as, MemoryRegion *root,
>> +                         const char *name);
>>   
>>   /**
>>    * address_space_destroy: destroy an address space
>> diff --git a/memory.c b/memory.c
>> index 410c5f8..ab3b136 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1562,14 +1562,14 @@ void memory_listener_unregister(MemoryListener *listener)
>>       QTAILQ_REMOVE(&memory_listeners, listener, link);
>>   }
>>   
>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>> +void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>>   {
>>       memory_region_transaction_begin();
>>       as->root = root;
>>       as->current_map = g_new(FlatView, 1);
>>       flatview_init(as->current_map);
>>       QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
>> -    as->name = NULL;
>> +    as->name = g_strdup(name ? name : root->name);
>>       memory_region_transaction_commit();
>>       address_space_init_dispatch(as);
>>   }
>> @@ -1584,6 +1584,7 @@ void address_space_destroy(AddressSpace *as)
>>       address_space_destroy_dispatch(as);
>>       flatview_destroy(as->current_map);
>>       g_free(as->current_map);
>> +    g_free((char *)as->name);
>>   }
>>   
>>   uint64_t io_mem_read(MemoryRegion *mr, hwaddr addr, unsigned size)
>> -- 
>> 1.7.9.5

[Qemu-devel] [PATCH v3 1/1] memory: add name in AddressSpace initialization.

[Alexander Barabash]

Pass the AddressSpace's name (to be used for debugging)
to address_space_init(). If NULL is passed, the name
of root memory region is used instead.

Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
---
 exec.c                |    6 ++----
 hw/pci/pci.c          |    3 ++-
 include/exec/memory.h |    5 +++--
 memory.c              |    5 +++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/exec.c b/exec.c
index a6923ad..b22abcc 100644
--- a/exec.c
+++ b/exec.c
@@ -1784,13 +1784,11 @@ static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
     memory_region_init(system_memory, "system", INT64_MAX);
-    address_space_init(&address_space_memory, system_memory);
-    address_space_memory.name = "memory";
+    address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
     memory_region_init(system_io, "io", 65536);
-    address_space_init(&address_space_io, system_io);
-    address_space_io.name = "I/O";
+    address_space_init(&address_space_io, system_io, "IO");
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
     memory_listener_register(&io_memory_listener, &address_space_io);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 94840c4..6cc3abe 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -790,7 +790,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                  get_system_memory(), 0,
                                  memory_region_size(get_system_memory()));
         memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
-        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
+        address_space_init(&pci_dev->bus_master_as,
+                           &pci_dev->bus_master_enable_region, NULL);
         pci_dev->dma = g_new(DMAContext, 1);
         dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
     }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..cb79a65 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -817,9 +817,10 @@ void mtree_info(fprintf_function mon_printf, void *f);
  *
  * @as: an uninitialized #AddressSpace
  * @root: a #MemoryRegion that routes addesses for the address space
+ * @name: used for debugging. If NULL, name of the root memory region is used.
  */
-void address_space_init(AddressSpace *as, MemoryRegion *root);
-
+void address_space_init(AddressSpace *as, MemoryRegion *root,
+                         const char *name);
 
 /**
  * address_space_destroy: destroy an address space
diff --git a/memory.c b/memory.c
index 410c5f8..ab3b136 100644
--- a/memory.c
+++ b/memory.c
@@ -1562,14 +1562,14 @@ void memory_listener_unregister(MemoryListener *listener)
     QTAILQ_REMOVE(&memory_listeners, listener, link);
 }
 
-void address_space_init(AddressSpace *as, MemoryRegion *root)
+void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_transaction_begin();
     as->root = root;
     as->current_map = g_new(FlatView, 1);
     flatview_init(as->current_map);
     QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
-    as->name = NULL;
+    as->name = g_strdup(name ? name : root->name);
     memory_region_transaction_commit();
     address_space_init_dispatch(as);
 }
@@ -1584,6 +1584,7 @@ void address_space_destroy(AddressSpace *as)
     address_space_destroy_dispatch(as);
     flatview_destroy(as->current_map);
     g_free(as->current_map);
+    g_free((char *)as->name);
 }
 
 uint64_t io_mem_read(MemoryRegion *mr, hwaddr addr, unsigned size)
-- 
1.7.9.5