[Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The 'name' parameter to memory_region_init_* had been marked as debug
only, however vmstate_region_ram uses it as a parameter to
qemu_ram_set_idstr to set RAMBlock names and these form part of the
migration stream.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
v2
  Reword to state uniqueness
  Only change RAM related calls
  Diff with -U7 so you can see the function names with each comment

 include/exec/memory.h | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6911023..6a3d7d5 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -303,15 +303,16 @@ struct MemoryRegionSection {
  * memory_region_init: Initialize a memory region
  *
  * The region typically acts as a container for other memory regions.  Use
  * memory_region_add_subregion() to add subregions.
  *
  * @mr: the #MemoryRegion to be initialized
  * @owner: the object that tracks the region's reference count
- * @name: used for debugging; not visible to the user or ABI
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region; any subregions beyond this size will be clipped
  */
 void memory_region_init(MemoryRegion *mr,
                         struct Object *owner,
                         const char *name,
                         uint64_t size);
 
@@ -367,15 +368,16 @@ void memory_region_init_io(MemoryRegion *mr,
 
 /**
  * memory_region_init_ram:  Initialize RAM memory region.  Accesses into the
  *                          region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_ram(MemoryRegion *mr,
                             struct Object *owner,
                             const char *name,
                             uint64_t size,
@@ -386,15 +388,16 @@ void memory_region_init_ram(MemoryRegion *mr,
  *                                     RAM.  Accesses into the region will
  *                                     modify memory directly.  Only an initial
  *                                     portion of this RAM is actually used.
  *                                     The used size can change across reboots.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: used size of the region.
  * @max_size: max size of the region.
  * @resized: callback to notify owner about used size change.
  * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_resizeable_ram(MemoryRegion *mr,
                                        struct Object *owner,
@@ -408,15 +411,16 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
 #ifdef __linux__
 /**
  * memory_region_init_ram_from_file:  Initialize RAM memory region with a
  *                                    mmap-ed backend.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region.
  * @share: %true if memory must be mmaped with the MAP_SHARED flag
  * @path: the path in which to allocate the RAM.
  * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       struct Object *owner,
@@ -430,15 +434,16 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
 /**
  * memory_region_init_ram_ptr:  Initialize RAM memory region from a
  *                              user-provided pointer.  Accesses into the
  *                              region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region.
  * @ptr: memory to be mapped; must contain at least @size bytes.
  */
 void memory_region_init_ram_ptr(MemoryRegion *mr,
                                 struct Object *owner,
                                 const char *name,
                                 uint64_t size,
@@ -492,15 +497,16 @@ void memory_region_init_alias(MemoryRegion *mr,
  *
  * This has the same effect as calling memory_region_init_ram()
  * and then marking the resulting region read-only with
  * memory_region_set_readonly().
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_rom(MemoryRegion *mr,
                             struct Object *owner,
                             const char *name,
                             uint64_t size,
@@ -509,15 +515,16 @@ void memory_region_init_rom(MemoryRegion *mr,
 /**
  * memory_region_init_rom_device:  Initialize a ROM memory region.  Writes are
  *                                 handled via callbacks.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
  * @ops: callbacks for write access handling (must not be NULL).
- * @name: the name of the region.
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_rom_device(MemoryRegion *mr,
                                    struct Object *owner,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Paolo Bonzini]

On 09/03/2017 16:27, Dr. David Alan Gilbert (git) wrote:
> + * @name: Region name, becomes part of RAMBlock name used in migration stream
> + *        must be unique within any device
>   * @size: size of the region; any subregions beyond this size will be clipped
>   */
>  void memory_region_init(MemoryRegion *mr,
>                          struct Object *owner,
>                          const char *name,
>                          uint64_t size);

This one should not be necessary I guess?

Paolo

Re: [Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 09/03/2017 16:27, Dr. David Alan Gilbert (git) wrote:
> > + * @name: Region name, becomes part of RAMBlock name used in migration stream
> > + *        must be unique within any device
> >   * @size: size of the region; any subregions beyond this size will be clipped
> >   */
> >  void memory_region_init(MemoryRegion *mr,
> >                          struct Object *owner,
> >                          const char *name,
> >                          uint64_t size);
> 
> This one should not be necessary I guess?

Why? It's not static so something else could be calling it for
any use.

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Paolo Bonzini]

On 09/03/2017 17:13, Dr. David Alan Gilbert wrote:
>>> + * @name: Region name, becomes part of RAMBlock name used in migration stream
>>> + *        must be unique within any device
>>>   * @size: size of the region; any subregions beyond this size will be clipped
>>>   */
>>>  void memory_region_init(MemoryRegion *mr,
>>>                          struct Object *owner,
>>>                          const char *name,
>>>                          uint64_t size);
>> This one should not be necessary I guess?
> Why? It's not static so something else could be calling it for
> any use.

But why would the name end up in a RAMBlock?

Paolo

Re: [Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 09/03/2017 17:13, Dr. David Alan Gilbert wrote:
> >>> + * @name: Region name, becomes part of RAMBlock name used in migration stream
> >>> + *        must be unique within any device
> >>>   * @size: size of the region; any subregions beyond this size will be clipped
> >>>   */
> >>>  void memory_region_init(MemoryRegion *mr,
> >>>                          struct Object *owner,
> >>>                          const char *name,
> >>>                          uint64_t size);
> >> This one should not be necessary I guess?
> > Why? It's not static so something else could be calling it for
> > any use.
> 
> But why would the name end up in a RAMBlock?

It is used by all of the _ram specific functions, so it seemed worth
including the comment.

(I also see it's used in some of the boards but I don't understand
why - e.g. hw/ppc/ppc405_boards.c calls it on something that looks
like RAM.  Although perhaps that's related to the 'fix this' above it)

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] memory_region: Fix name comments

[Paolo Bonzini]

On 09/03/2017 17:38, Dr. David Alan Gilbert wrote:
> It is used by all of the _ram specific functions, so it seemed worth
> including the comment.

It's internal though.  memory.c users never use it in a way that creates
a RAMBlock.

> (I also see it's used in some of the boards but I don't understand
> why - e.g. hw/ppc/ppc405_boards.c calls it on something that looks
> like RAM.  Although perhaps that's related to the 'fix this' above it)

It's a weird 0-length memory region, and it doesn't have a RAMBlock.
Probably needed only to satisfy ppc405ep_init.

    ram_bases[0] = 0;
    ram_sizes[0] = 0x08000000;
    memory_region_init(&ram_memories[1], NULL, "ef405ep.ram1", 0);
    ram_bases[1] = 0x00000000;
    ram_sizes[1] = 0x00000000;

Paolo