Re: [Qemu-devel] [PATCH] RFC: mark non-volatile memory region

[Michael S. Tsirkin]

On Thu, Sep 20, 2018 at 07:17:30PM +0400, Marc-André Lureau wrote:
> This is RFC patch to show how NVDIMM could set a flag on the
> MemoryRegion that would be propagated down to the flat view.
> 
> This way, guest_phys_blocks_region_add() can skip the NV memory
> regions for dumps and TCG memory clear.
> 
> qemu-system-x86_64 -machine pc,nvdimm -m 2G,slots=4,maxmem=16G -enable-kvm 
> -monitor stdio -object 
> memory-backend-file,id=mem1,share=on,mem-path=/tmp/foo,size=1G -device 
> nvdimm,id=nvdimm1,memdev=mem1
> 
> HMP info mtree command reflects the flag with "nv-" prefix on memory type:
> 
> (qemu) info mtree
> 0000000100000000-000000013fffffff (prio 0, nv-i/o): alias nvdimm-memory 
> @/objects/mem1 0000000000000000-000000003fffffff
> 
> (qemu) info mtree -f
> 0000000100000000-000000013fffffff (prio 0, nv-ram): /objects/mem1
> 
> Signed-off-by: Marc-André Lureau <address@hidden>

I would do s/nv/nonvolatile/ besides that looks good to me.

> ---
>  include/exec/memory.h    | 25 +++++++++++++++++++++++
>  hw/mem/nvdimm.c          |  1 +
>  memory.c                 | 43 ++++++++++++++++++++++++++++++----------
>  memory_mapping.c         |  3 ++-
>  docs/devel/migration.rst |  1 +
>  5 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index eb4f2fb249..b334331d3e 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -360,6 +360,7 @@ struct MemoryRegion {
>      bool ram;
>      bool subpage;
>      bool readonly; /* For RAM regions */
> +    bool nv; /* For non-volatile regions */
>      bool rom_device;
>      bool flush_coalesced_mmio;
>      bool global_locking;
> @@ -485,6 +486,7 @@ static inline FlatView 
> *address_space_to_flatview(AddressSpace *as)
>   * @offset_within_address_space: the address of the first byte of the section
>   *     relative to the region's address space
>   * @readonly: writes to this section are ignored
> + * @nv: this section is non-volatile
>   */
>  struct MemoryRegionSection {
>      MemoryRegion *mr;
> @@ -493,6 +495,7 @@ struct MemoryRegionSection {
>      Int128 size;
>      hwaddr offset_within_address_space;
>      bool readonly;
> +    bool nv;
>  };
>  
>  /**
> @@ -1175,6 +1178,17 @@ static inline bool memory_region_is_rom(MemoryRegion 
> *mr)
>      return mr->ram && mr->readonly;
>  }
>  
> +/**
> + * memory_region_is_nv: check whether a memory region is non-volatile
> + *
> + * Returns %true is a memory region is non-volatile memory.
> + *
> + * @mr: the memory region being queried
> + */
> +static inline bool memory_region_is_nv(MemoryRegion *mr)
> +{
> +    return mr->nv;
> +}
>  
>  /**
>   * memory_region_get_fd: Get a file descriptor backing a RAM memory region.
> @@ -1346,6 +1360,17 @@ void memory_region_reset_dirty(MemoryRegion *mr, 
> hwaddr addr,
>   */
>  void memory_region_set_readonly(MemoryRegion *mr, bool readonly);
>  
> +/**
> + * memory_region_set_nv: Turn a memory region non-volatile
> + *
> + * Allows a memory region to be marked as non-volatile.
> + * only useful on RAM regions.
> + *
> + * @mr: the region being updated.
> + * @nv: whether rhe region is to be NV.
> + */
> +void memory_region_set_nv(MemoryRegion *mr, bool nv);
> +
>  /**
>   * memory_region_rom_device_set_romd: enable/disable ROMD mode
>   *
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 1c6674c4ed..016a9fbbb4 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -115,6 +115,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice 
> *nvdimm, Error **errp)
>      nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
>      memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
>                               "nvdimm-memory", mr, 0, pmem_size);
> +    memory_region_set_nv(nvdimm->nvdimm_mr, true);
>      nvdimm->nvdimm_mr->align = align;
>  }
>  
> diff --git a/memory.c b/memory.c
> index 9b73892768..8b34a1a077 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -216,6 +216,7 @@ struct FlatRange {
>      uint8_t dirty_log_mask;
>      bool romd_mode;
>      bool readonly;
> +    bool nv;
>  };
>  
>  #define FOR_EACH_FLAT_RANGE(var, view)          \
> @@ -231,6 +232,7 @@ section_from_flat_range(FlatRange *fr, FlatView *fv)
>          .size = fr->addr.size,
>          .offset_within_address_space = int128_get64(fr->addr.start),
>          .readonly = fr->readonly,
> +        .nv = fr->nv,
>      };
>  }
>  
> @@ -240,7 +242,8 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b)
>          && addrrange_equal(a->addr, b->addr)
>          && a->offset_in_region == b->offset_in_region
>          && a->romd_mode == b->romd_mode
> -        && a->readonly == b->readonly;
> +        && a->readonly == b->readonly
> +        && a->nv == b->nv;
>  }
>  
>  static FlatView *flatview_new(MemoryRegion *mr_root)
> @@ -312,7 +315,8 @@ static bool can_merge(FlatRange *r1, FlatRange *r2)
>                       int128_make64(r2->offset_in_region))
>          && r1->dirty_log_mask == r2->dirty_log_mask
>          && r1->romd_mode == r2->romd_mode
> -        && r1->readonly == r2->readonly;
> +        && r1->readonly == r2->readonly
> +        && r1->nv == r2->nv;
>  }
>  
>  /* Attempt to simplify a view by merging adjacent ranges */
> @@ -619,7 +623,8 @@ static void render_memory_region(FlatView *view,
>                                   MemoryRegion *mr,
>                                   Int128 base,
>                                   AddrRange clip,
> -                                 bool readonly)
> +                                 bool readonly,
> +                                 bool nv)
>  {
>      MemoryRegion *subregion;
>      unsigned i;
> @@ -635,6 +640,7 @@ static void render_memory_region(FlatView *view,
>  
>      int128_addto(&base, int128_make64(mr->addr));
>      readonly |= mr->readonly;
> +    nv |= mr->nv;
>  
>      tmp = addrrange_make(base, mr->size);
>  
> @@ -647,13 +653,13 @@ static void render_memory_region(FlatView *view,
>      if (mr->alias) {
>          int128_subfrom(&base, int128_make64(mr->alias->addr));
>          int128_subfrom(&base, int128_make64(mr->alias_offset));
> -        render_memory_region(view, mr->alias, base, clip, readonly);
> +        render_memory_region(view, mr->alias, base, clip, readonly, nv);
>          return;
>      }
>  
>      /* Render subregions in priority order. */
>      QTAILQ_FOREACH(subregion, &mr->subregions, subregions_link) {
> -        render_memory_region(view, subregion, base, clip, readonly);
> +        render_memory_region(view, subregion, base, clip, readonly, nv);
>      }
>  
>      if (!mr->terminates) {
> @@ -668,6 +674,7 @@ static void render_memory_region(FlatView *view,
>      fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>      fr.romd_mode = mr->romd_mode;
>      fr.readonly = readonly;
> +    fr.nv = nv;
>  
>      /* Render the region itself into any gaps left by the current view. */
>      for (i = 0; i < view->nr && int128_nz(remain); ++i) {
> @@ -753,7 +760,8 @@ static FlatView *generate_memory_topology(MemoryRegion 
> *mr)
>  
>      if (mr) {
>          render_memory_region(view, mr, int128_zero(),
> -                             addrrange_make(int128_zero(), int128_2_64()), 
> false);
> +                             addrrange_make(int128_zero(), int128_2_64()),
> +                             false, false);
>      }
>      flatview_simplify(view);
>  
> @@ -2038,6 +2046,16 @@ void memory_region_set_readonly(MemoryRegion *mr, bool 
> readonly)
>      }
>  }
>  
> +void memory_region_set_nv(MemoryRegion *mr, bool nv)
> +{
> +    if (mr->nv != nv) {
> +        memory_region_transaction_begin();
> +        mr->nv = nv;
> +        memory_region_update_pending |= mr->enabled;
> +        memory_region_transaction_commit();
> +    }
> +}
> +
>  void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode)
>  {
>      if (mr->romd_mode != romd_mode) {
> @@ -2488,6 +2506,7 @@ static MemoryRegionSection 
> memory_region_find_rcu(MemoryRegion *mr,
>      ret.size = range.size;
>      ret.offset_within_address_space = int128_get64(range.start);
>      ret.readonly = fr->readonly;
> +    ret.nv = fr->nv;
>      return ret;
>  }
>  
> @@ -2838,10 +2857,11 @@ static void mtree_print_mr(fprintf_function 
> mon_printf, void *f,
>              QTAILQ_INSERT_TAIL(alias_print_queue, ml, mrqueue);
>          }
>          mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx
> -                   " (prio %d, %s): alias %s @%s " TARGET_FMT_plx
> +                   " (prio %d, %s%s): alias %s @%s " TARGET_FMT_plx
>                     "-" TARGET_FMT_plx "%s",
>                     cur_start, cur_end,
>                     mr->priority,
> +                   mr->nv ? "nv-" : "",
>                     memory_region_type((MemoryRegion *)mr),
>                     memory_region_name(mr),
>                     memory_region_name(mr->alias),
> @@ -2853,9 +2873,10 @@ static void mtree_print_mr(fprintf_function 
> mon_printf, void *f,
>          }
>      } else {
>          mon_printf(f,
> -                   TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s%s",
> +                   TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s%s): 
> %s%s",
>                     cur_start, cur_end,
>                     mr->priority,
> +                   mr->nv ? "nv-" : "",
>                     memory_region_type((MemoryRegion *)mr),
>                     memory_region_name(mr),
>                     mr->enabled ? "" : " [disabled]");
> @@ -2940,19 +2961,21 @@ static void mtree_print_flatview(gpointer key, 
> gpointer value,
>          mr = range->mr;
>          if (range->offset_in_region) {
>              p(f, MTREE_INDENT TARGET_FMT_plx "-"
> -              TARGET_FMT_plx " (prio %d, %s): %s @" TARGET_FMT_plx,
> +              TARGET_FMT_plx " (prio %d, %s%s): %s @" TARGET_FMT_plx,
>                int128_get64(range->addr.start),
>                int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
>                mr->priority,
> +              range->nv ? "nv-" : "",
>                range->readonly ? "rom" : memory_region_type(mr),
>                memory_region_name(mr),
>                range->offset_in_region);
>          } else {
>              p(f, MTREE_INDENT TARGET_FMT_plx "-"
> -              TARGET_FMT_plx " (prio %d, %s): %s",
> +              TARGET_FMT_plx " (prio %d, %s%s): %s",
>                int128_get64(range->addr.start),
>                int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
>                mr->priority,
> +              range->nv ? "nv-" : "",
>                range->readonly ? "rom" : memory_region_type(mr),
>                memory_region_name(mr));
>          }
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 775466f3a8..d59806f53b 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener 
> *listener,
>  
>      /* we only care about RAM */
>      if (!memory_region_is_ram(section->mr) ||
> -        memory_region_is_ram_device(section->mr)) {
> +        memory_region_is_ram_device(section->mr) ||
> +        memory_region_is_nv(section->mr)) {
>          return;
>      }
>  
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index 687570754d..1a0a39d500 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -435,6 +435,7 @@ Examples of such memory API functions are:
>    - memory_region_add_subregion()
>    - memory_region_del_subregion()
>    - memory_region_set_readonly()
> +  - memory_region_set_nv()
>    - memory_region_set_enabled()
>    - memory_region_set_address()
>    - memory_region_set_alias_offset()
> -- 
> 2.19.0