Re: [PATCH V7 01/29] memory: qemu_check_ram_volatile

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

On 22/12/21 20:05, Steve Sistare wrote:
> Add a function that returns an error if any ram_list block represents
> volatile memory.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   include/exec/memory.h |  8 ++++++++
>   softmmu/memory.c      | 26 ++++++++++++++++++++++++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 20f1b27..137f5f3 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2981,6 +2981,14 @@ bool ram_block_discard_is_disabled(void);
>    */
>   bool ram_block_discard_is_required(void);
>   
> +/**
> + * qemu_ram_check_volatile: return 1 if any memory regions are writable and not
> + * backed by shared memory, else return 0.
> + *
> + * @errp: returned error message identifying the first volatile region found.

This doesn't seem a good usage of the Error API. This is not an error
actually, but the expected result. If you want to return the first
or all, better use an explicit argument for them. Returning the first
is odd however. Is it useful for the user? If so, we want to return
them all, eventually in a GArray/GPtrArray, and return the MemoryRegion
handle, not its name. Otherwise if it is only useful for developers I'd
simply log the volatile MR name in a trace event.

Then we get:

   bool ram_block_is_volatile(void);

Or

   bool qemu_ram_is_volatile(void);

> + */
> +int qemu_check_ram_volatile(Error **errp);


> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7340e19..30b2f68 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2837,6 +2837,32 @@ void memory_global_dirty_log_stop(unsigned int flags)
>       memory_global_dirty_log_do_stop(flags);
>   }
>   
> +static int check_volatile(RAMBlock *rb, void *opaque)

If using the 'qemu_ram_is_volatile' name for the public API,
this one could be 'static bool ram_block_is_volatile(...)'.

> +{
> +    MemoryRegion *mr = rb->mr;
> +
> +    if (mr &&
> +        memory_region_is_ram(mr) &&
> +        !memory_region_is_ram_device(mr) &&
> +        !memory_region_is_rom(mr) &&
> +        (rb->fd == -1 || !qemu_ram_is_shared(rb))) {
> +        *(const char **)opaque = memory_region_name(mr);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +int qemu_check_ram_volatile(Error **errp)
> +{
> +    char *name;
> +
> +    if (qemu_ram_foreach_block(check_volatile, &name)) {
> +        error_setg(errp, "Memory region %s is volatile", name);
> +        return -1;
> +    }
> +    return 0;
> +}

Regards,

Phil.