[V5,01/25] qemu_ram_volatile

Commit Message

Steven Sistare July 7, 2021, 5:20 p.m. UTC

Add a function that returns true if any ram_list block represents
volatile memory.

Signed-off-by: Steve Sistare <[e-mail]>
---
 include/exec/memory.h |  8 ++++++++
 softmmu/memory.c      | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Marc-André Lureau July 8, 2021, 12:01 p.m. UTC | #1

Hi

On Wed, Jul 7, 2021 at 9:35 PM Steve Sistare <[e-mail]>
wrote:

> Add a function that returns true if any ram_list block represents
> volatile memory.
>
> Signed-off-by: Steve Sistare <[e-mail]>
> ---
>  include/exec/memory.h |  8 ++++++++
>  softmmu/memory.c      | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index b116f7c..7ad63f8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2649,6 +2649,14 @@ bool ram_block_discard_is_disabled(void);
>   */
>  bool ram_block_discard_is_required(void);
>
> +/**
> + * qemu_ram_volatile: return true if any memory regions are writable and
> not
> + * backed by shared memory.
> + *
> + * @errp: returned error message identifying the bad region.
> + */
> +bool qemu_ram_volatile(Error **errp);
>

Usually, bool-value functions with an error return true on success. If it
deviates from the recommendation, it should at least be documented.

Also, we have a preference for using _is_ in the function name for such
tests.

+
>  #endif
>
>  #endif
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index f016151..e9536bc 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2714,6 +2714,36 @@ void memory_global_dirty_log_stop(void)
>      memory_global_dirty_log_do_stop();
>  }
>
> +/*
> + * Return true if any memory regions are writable and not backed by shared
> + * memory.
> + */
>

Let's not duplicate API comments.

+bool qemu_ram_volatile(Error **errp)
> +{
> +    RAMBlock *block;
> +    MemoryRegion *mr;
> +    bool ret = false;
> +
> +    rcu_read_lock();
>

RCU_READ_LOCK_GUARD()


> +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>

RAMBLOCK_FOREACH() should do.

Or rather use the qemu_ram_foreach_block() helper.


+        mr = block->mr;
> +        if (mr &&
> +            memory_region_is_ram(mr) &&
> +            !memory_region_is_ram_device(mr) &&
> +            !memory_region_is_rom(mr) &&
> +            (block->fd == -1 || !qemu_ram_is_shared(block))) {
> +
> +            error_setg(errp, "Memory region %s is volatile",
> +                       memory_region_name(mr));
> +            ret = true;
> +            break;
> +        }
> +    }
> +
> +    rcu_read_unlock();
> +    return ret;
> +}
> +
>  static void listener_add_address_space(MemoryListener *listener,
>                                         AddressSpace *as)
>  {
> --
> 1.8.3.1
>
>
>

Steven Sistare July 12, 2021, 5:06 p.m. UTC | #2

Will do for all comments - steve

On 7/8/2021 8:01 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 7, 2021 at 9:35 PM Steve Sistare <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     Add a function that returns true if any ram_list block represents
>     volatile memory.
> 
>     Signed-off-by: Steve Sistare <[e-mail] <mailto:[e-mail]>>
>     ---
>      include/exec/memory.h |  8 ++++++++
>      softmmu/memory.c      | 30 ++++++++++++++++++++++++++++++
>      2 files changed, 38 insertions(+)
> 
>     diff --git a/include/exec/memory.h b/include/exec/memory.h
>     index b116f7c..7ad63f8 100644
>     --- a/include/exec/memory.h
>     +++ b/include/exec/memory.h
>     @@ -2649,6 +2649,14 @@ bool ram_block_discard_is_disabled(void);
>       */
>      bool ram_block_discard_is_required(void);
> 
>     +/**
>     + * qemu_ram_volatile: return true if any memory regions are writable and not
>     + * backed by shared memory.
>     + *
>     + * @errp: returned error message identifying the bad region.
>     + */
>     +bool qemu_ram_volatile(Error **errp);
> 
> 
> Usually, bool-value functions with an error return true on success. If it deviates from the recommendation, it should at least be documented.
> 
> Also, we have a preference for using _is_ in the function name for such tests.
> 
>     +
>      #endif
> 
>      #endif
>     diff --git a/softmmu/memory.c b/softmmu/memory.c
>     index f016151..e9536bc 100644
>     --- a/softmmu/memory.c
>     +++ b/softmmu/memory.c
>     @@ -2714,6 +2714,36 @@ void memory_global_dirty_log_stop(void)
>          memory_global_dirty_log_do_stop();
>      }
> 
>     +/*
>     + * Return true if any memory regions are writable and not backed by shared
>     + * memory.
>     + */
> 
> 
> Let's not duplicate API comments.
> 
>     +bool qemu_ram_volatile(Error **errp)
>     +{
>     +    RAMBlock *block;
>     +    MemoryRegion *mr;
>     +    bool ret = false;
>     +
>     +    rcu_read_lock();
> 
> 
> RCU_READ_LOCK_GUARD()
>  
> 
>     +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> 
> 
> RAMBLOCK_FOREACH() should do.
> 
> Or rather use the qemu_ram_foreach_block() helper.
> 
> 
>     +        mr = block->mr;
>     +        if (mr &&
>     +            memory_region_is_ram(mr) &&
>     +            !memory_region_is_ram_device(mr) &&
>     +            !memory_region_is_rom(mr) &&
>     +            (block->fd == -1 || !qemu_ram_is_shared(block))) {
>     +
>     +            error_setg(errp, "Memory region %s is volatile",
>     +                       memory_region_name(mr));
>     +            ret = true;
>     +            break;
>     +        }
>     +    }
>     +
>     +    rcu_read_unlock();
>     +    return ret;
>     +}
>     +
>      static void listener_add_address_space(MemoryListener *listener,
>                                             AddressSpace *as)
>      {
>     -- 
>     1.8.3.1
> 
> 
> 
> 
> -- 
> Marc-André Lureau

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index b116f7c..7ad63f8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2649,6 +2649,14 @@  bool ram_block_discard_is_disabled(void);
  */
 bool ram_block_discard_is_required(void);
 
+/**
+ * qemu_ram_volatile: return true if any memory regions are writable and not
+ * backed by shared memory.
+ *
+ * @errp: returned error message identifying the bad region.
+ */
+bool qemu_ram_volatile(Error **errp);
+
 #endif
 
 #endif
diff --git a/softmmu/memory.c b/softmmu/memory.c
index f016151..e9536bc 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2714,6 +2714,36 @@  void memory_global_dirty_log_stop(void)
     memory_global_dirty_log_do_stop();
 }
 
+/*
+ * Return true if any memory regions are writable and not backed by shared
+ * memory.
+ */
+bool qemu_ram_volatile(Error **errp)
+{
+    RAMBlock *block;
+    MemoryRegion *mr;
+    bool ret = false;
+
+    rcu_read_lock();
+    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+        mr = block->mr;
+        if (mr &&
+            memory_region_is_ram(mr) &&
+            !memory_region_is_ram_device(mr) &&
+            !memory_region_is_rom(mr) &&
+            (block->fd == -1 || !qemu_ram_is_shared(block))) {
+
+            error_setg(errp, "Memory region %s is volatile",
+                       memory_region_name(mr));
+            ret = true;
+            break;
+        }
+    }
+
+    rcu_read_unlock();
+    return ret;
+}
+
 static void listener_add_address_space(MemoryListener *listener,
                                        AddressSpace *as)
 {