[4/5] include/exec: annotate all the MemoryRegion fields

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/memory.h | 47 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

On 3/7/24 08:11, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/memory.h | 47 +++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 17b741bc4f5..312ed564dbe 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -778,9 +778,48 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>   typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>   typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>   
> -/** MemoryRegion:
> - *
> - * A struct representing a memory region.
> +/**
> + * struct MemoryRegion - represents a memory region
> + * @parent_obj: parent QOM object for the region
> + * @romd_mode: if true ROM devices accessed directly rather than with @ops
> + * @ram: true if a RAM-type device with a @ram_block
> + * @subpage: true if region covers a subpage
> + * @readonly: true for RAM-type devices that are readonly
> + * @nonvolatile: true for nonvolatile RAM-type devices (e.g. NVDIMM)
> + * @rom_device: true for a ROM device, see also @romd_mode
> + * @flush_coalesced_mmio: true to flush any queued coalesced MMIO
> + * operations before access
> + * @unmergeable: this section should not get merged with adjacent
> + * sections
> + * @dirty_log_mask: dirty events region cares about (see DIRTY_ flags)
> + * @is_iommu: true if part of an IOMMU device
> + * @ram_block: backing @RamBlock if @ram is true
> + * @owner: base QOM object that owns this region
> + * @dev: base Device that owns this region
> + * @ops: access operations for MMIO or @romd_mode devices
> + * @opaque: @dev specific data, passed with @ops
> + * @container: parent `MemoryRegion`
> + * @mapped_via_alias: number of times mapped via @alias, container
> + * might be NULL
> + * @size: size of @MemoryRegion
> + * @addr: physical hwaddr of @MemoryRegion
> + * @destructor: cleanup function when @MemoryRegion finalized
> + * @align: alignment requirements for any physical backing store
> + * @terminates: true if this @MemoryRegion is a leaf node
> + * @ram_device: true if @ram device should use @ops to access
> + * @enabled: true once initialised, false once finalized
> + * @vga_logging_count: count of memory logging clients
> + * @alias: link to aliased @MemoryRegion
> + * @alias_offset: offset into aliased region
> + * @priority: priority when resolving overlapping regions
> + * @subregions: list of subregions in this region
> + * @subregions_link: next subregion in the chain
> + * @coalesced: list of coalesced memory ranges
> + * @name: name of memory region
> + * @ioeventfd_nb: count of @ioeventfds for region
> + * @ioeventfds: ioevent notifiers for region
> + * @rdm: if exists see #RamDiscardManager
> + * @disable_reentrancy_guard: if true don't error if device accesses itself
>    */

Why as one big block rather than line by line?
The block is less likely to be properly kept up-to-date and is much harder to read.


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/7/24 08:11, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/exec/memory.h | 47 +++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 43 insertions(+), 4 deletions(-)
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 17b741bc4f5..312ed564dbe 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -778,9 +778,48 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>>   typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>>   typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>>   -/** MemoryRegion:
>> - *
>> - * A struct representing a memory region.
>> +/**
>> + * struct MemoryRegion - represents a memory region
>> + * @parent_obj: parent QOM object for the region
>> + * @romd_mode: if true ROM devices accessed directly rather than with @ops
>> + * @ram: true if a RAM-type device with a @ram_block
>> + * @subpage: true if region covers a subpage
>> + * @readonly: true for RAM-type devices that are readonly
>> + * @nonvolatile: true for nonvolatile RAM-type devices (e.g. NVDIMM)
>> + * @rom_device: true for a ROM device, see also @romd_mode
>> + * @flush_coalesced_mmio: true to flush any queued coalesced MMIO
>> + * operations before access
>> + * @unmergeable: this section should not get merged with adjacent
>> + * sections
>> + * @dirty_log_mask: dirty events region cares about (see DIRTY_ flags)
>> + * @is_iommu: true if part of an IOMMU device
>> + * @ram_block: backing @RamBlock if @ram is true
>> + * @owner: base QOM object that owns this region
>> + * @dev: base Device that owns this region
>> + * @ops: access operations for MMIO or @romd_mode devices
>> + * @opaque: @dev specific data, passed with @ops
>> + * @container: parent `MemoryRegion`
>> + * @mapped_via_alias: number of times mapped via @alias, container
>> + * might be NULL
>> + * @size: size of @MemoryRegion
>> + * @addr: physical hwaddr of @MemoryRegion
>> + * @destructor: cleanup function when @MemoryRegion finalized
>> + * @align: alignment requirements for any physical backing store
>> + * @terminates: true if this @MemoryRegion is a leaf node
>> + * @ram_device: true if @ram device should use @ops to access
>> + * @enabled: true once initialised, false once finalized
>> + * @vga_logging_count: count of memory logging clients
>> + * @alias: link to aliased @MemoryRegion
>> + * @alias_offset: offset into aliased region
>> + * @priority: priority when resolving overlapping regions
>> + * @subregions: list of subregions in this region
>> + * @subregions_link: next subregion in the chain
>> + * @coalesced: list of coalesced memory ranges
>> + * @name: name of memory region
>> + * @ioeventfd_nb: count of @ioeventfds for region
>> + * @ioeventfds: ioevent notifiers for region
>> + * @rdm: if exists see #RamDiscardManager
>> + * @disable_reentrancy_guard: if true don't error if device accesses itself
>>    */
>
> Why as one big block rather than line by line?
> The block is less likely to be properly kept up-to-date and is much
> harder to read.

The inline syntax is a little more prone to parse failures and
annoyingly can't group multiple fields in one inline comment block. But
I can certainly move it inline if that preferable.

On Thu, 7 Mar 2024 at 18:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/exec/memory.h | 47 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 17b741bc4f5..312ed564dbe 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -778,9 +778,48 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>
> -/** MemoryRegion:
> - *
> - * A struct representing a memory region.
> +/**
> + * struct MemoryRegion - represents a memory region
> + * @parent_obj: parent QOM object for the region
> + * @romd_mode: if true ROM devices accessed directly rather than with @ops

If true, read accesses go directly to host memory;
write accesses always go via the MMIO accessor.

You might alternatively say "true if a ROM device is in ROMD mode; see
memory_region_device_set_romd()", since the doc comment for
that function has an explanation of what ROMD mode is.

> + * @ram: true if a RAM-type device with a @ram_block

We call this a "RAM memory region" in memory.rst and in
other doc comments in this file.

> + * @subpage: true if region covers a subpage

This is the kind of doc comment that doesn't tell us anything
we didn't already know from the field name. More useful would
be to explain what the subpage handling is actually doing.

> + * @readonly: true for RAM-type devices that are readonly
> + * @nonvolatile: true for nonvolatile RAM-type devices (e.g. NVDIMM)
> + * @rom_device: true for a ROM device, see also @romd_mode
> + * @flush_coalesced_mmio: true to flush any queued coalesced MMIO
> + * operations before access
> + * @unmergeable: this section should not get merged with adjacent
> + * sections
> + * @dirty_log_mask: dirty events region cares about (see DIRTY_ flags)
> + * @is_iommu: true if part of an IOMMU device
> + * @ram_block: backing @RamBlock if @ram is true
> + * @owner: base QOM object that owns this region
> + * @dev: base Device that owns this region
> + * @ops: access operations for MMIO or @romd_mode devices
> + * @opaque: @dev specific data, passed with @ops
> + * @container: parent `MemoryRegion`
> + * @mapped_via_alias: number of times mapped via @alias, container
> + * might be NULL
> + * @size: size of @MemoryRegion
> + * @addr: physical hwaddr of @MemoryRegion

I think this is not a physical address; it's the address
(i.e. offset) of this MR within its parent MR @container.

> + * @destructor: cleanup function when @MemoryRegion finalized
> + * @align: alignment requirements for any physical backing store
> + * @terminates: true if this @MemoryRegion is a leaf node
> + * @ram_device: true if @ram device should use @ops to access

This doesn't sound right. True if this MR is a RAM device memory
region (i.e. it is backed using a pointer corresponding to a
host physical device); see @memory_region_init_ram_device_ptr().

We should also add "RAM device" to the "Types of regions"
list in docs/devel/memory.rst, which is currently missing it.

> + * @enabled: true once initialised, false once finalized

Not very useful, as it misses entirely the use of the flag.
Enabled MRs appear in the guest's memory space; non-enabled
MRs are ignored. By default MRs are enabled, but they can
be enabled and disabled at runtime using memory_region_set_enabled().

> + * @vga_logging_count: count of memory logging clients
> + * @alias: link to aliased @MemoryRegion

If this is an alias MemoryRegion, pointer to the MR we are aliasing.

> + * @alias_offset: offset into aliased region
> + * @priority: priority when resolving overlapping regions
> + * @subregions: list of subregions in this region
> + * @subregions_link: next subregion in the chain
> + * @coalesced: list of coalesced memory ranges
> + * @name: name of memory region
> + * @ioeventfd_nb: count of @ioeventfds for region
> + * @ioeventfds: ioevent notifiers for region
> + * @rdm: if exists see #RamDiscardManager
> + * @disable_reentrancy_guard: if true don't error if device accesses itself
>   */
>  struct MemoryRegion {
>      Object parent_obj;
> @@ -806,7 +845,7 @@ struct MemoryRegion {
>      const MemoryRegionOps *ops;
>      void *opaque;
>      MemoryRegion *container;
> -    int mapped_via_alias; /* Mapped via an alias, container might be NULL */
> +    int mapped_via_alias;
>      Int128 size;
>      hwaddr addr;
>      void (*destructor)(MemoryRegion *mr);
> --
> 2.39.2

thanks
-- PMM