Re: [Qemu-devel] [PATCH v4 01/16] memory: Allow subregions to not be printed by info mtree

["Alex Bennée" <alex.bennee@linaro.org>]

Alistair Francis <alistair.francis@xilinx.com> writes:

> Add a function called memory_region_add_subregion_no_print() that
> creates memory subregions that won't be printed when running
> the 'info mtree' command.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>
>  include/exec/memory.h | 17 +++++++++++++++++
>  memory.c              | 10 +++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c92734a..25353df 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -186,6 +186,7 @@ struct MemoryRegion {
>      bool skip_dump;
>      bool enabled;
>      bool warning_printed; /* For reservations */
> +    bool do_not_print;
>      uint8_t vga_logging_count;
>      MemoryRegion *alias;
>      hwaddr alias_offset;
> @@ -954,6 +955,22 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>  void memory_region_add_subregion(MemoryRegion *mr,
>                                   hwaddr offset,
>                                   MemoryRegion *subregion);
> +
> +/**
> + * memory_region_add_subregion_no_print: Add a subregion to a container.
> + *
> + * The same functionality as memory_region_add_subregion except that any
> + * memory regions added by this function are not printed by 'info mtree'.
> + *
> + * @mr: the region to contain the new subregion; must be a container
> + *      initialized with memory_region_init().
> + * @offset: the offset relative to @mr where @subregion is added.
> + * @subregion: the subregion to be added.
> + */
> +void memory_region_add_subregion_no_print(MemoryRegion *mr,
> +                                          hwaddr offset,
> +                                          MemoryRegion *subregion);
> +

I think this needlessly complicates the memory region code and I'm not
sure what is too be gained for the register code. The only usage of the
code is inside a loop in register_init_block32. In each case the region
has the same set of ops. Why isn't a single region being created with an
indirect handler which can dispatch to the individual register handling
code?

While its true some drivers create individual IO regions by an large
most are creating a block with a common handler.

>  /**
>   * memory_region_add_subregion_overlap: Add a subregion to a container
>   *                                      with overlap.
> diff --git a/memory.c b/memory.c
> index 09041ed..228a8a7 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1829,6 +1829,14 @@ void memory_region_add_subregion(MemoryRegion *mr,
>      memory_region_add_subregion_common(mr, offset, subregion);
>  }
>
> +void memory_region_add_subregion_no_print(MemoryRegion *mr,
> +                                          hwaddr offset,
> +                                          MemoryRegion *subregion)
> +{
> +    memory_region_add_subregion(mr, offset, subregion);
> +    subregion->do_not_print = true;
> +}
> +
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           hwaddr offset,
>                                           MemoryRegion *subregion,
> @@ -2219,7 +2227,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>      const MemoryRegion *submr;
>      unsigned int i;
>
> -    if (!mr) {
> +    if (!mr || mr->do_not_print) {
>          return;
>      }


--
Alex Bennée