Re: [PATCH 1/2] memory: introduce memory_region_init_ram_protected()

[David Hildenbrand <david@redhat.com>]

On 20.06.23 21:50, Laurent Vivier wrote:
> Commit 56918a126a ("memory: Add RAM_PROTECTED flag to skip IOMMU mappings")
> has introduced the RAM_PROTECTED flag to denote "protected" memory.
> 
> This flags is only used with qemu_ram_alloc_from_fd() for now.
> 
> To be able to register memory region with this flag, define
> memory_region_init_ram_protected() and declare the flag as valid in
> qemu_ram_alloc_internal() and qemu_ram_alloc().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   include/exec/memory.h | 33 +++++++++++++++++++++++++++++++++
>   softmmu/memory.c      | 33 +++++++++++++++++++++++++++------
>   softmmu/physmem.c     |  4 ++--
>   3 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 47c2e0221c35..d8760015c381 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1520,6 +1520,39 @@ void memory_region_init_iommu(void *_iommu_mr,
>                                 const char *name,
>                                 uint64_t size);
>   
> +/**
> + * memory_region_init_ram_protected - Initialize RAM memory region.  Accesses
> + *                                    into the region will modify memory
> + *                                    directly.
> + *
> + * The memory is created with the RAM_PROTECTED flag, for memory that
> + * looks and acts like RAM but inaccessible via normal mechanisms,
> + * including DMA.
> + *
> + * @mr: the #MemoryRegion to be initialized
> + * @owner: the object that tracks the region's reference count (must be
> + *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
> + * @name: name of the memory region
> + * @size: size of the region in bytes
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * This function allocates RAM for a board model or device, and
> + * arranges for it to be migrated (by calling vmstate_register_ram()
> + * if @owner is a DeviceState, or vmstate_register_ram_global() if
> + * @owner is NULL).
> + *
> + * TODO: Currently we restrict @owner to being either NULL (for
> + * global RAM regions with no owner) or devices, so that we can
> + * give the RAM block a unique name for migration purposes.
> + * We should lift this restriction and allow arbitrary Objects.
> + * If you pass a non-NULL non-device @owner then we will assert.
> + */
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp);
> +
>   /**
>    * memory_region_init_ram - Initialize RAM memory region.  Accesses into the
>    *                          region will modify memory directly.
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7d9494ce7028..952c87277353 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3551,16 +3551,18 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>       }
>   }
>   
> -void memory_region_init_ram(MemoryRegion *mr,
> -                            Object *owner,
> -                            const char *name,
> -                            uint64_t size,
> -                            Error **errp)
> +static void memory_region_init_ram_flags(MemoryRegion *mr,
> +                                         Object *owner,
> +                                         const char *name,
> +                                         uint64_t size,
> +                                         uint32_t ram_flags,
> +                                         Error **errp)
>   {
>       DeviceState *owner_dev;
>       Error *err = NULL;
>   
> -    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
> +    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, ram_flags,
> +                                           &err);
>       if (err) {
>           error_propagate(errp, err);
>           return;
> @@ -3575,6 +3577,25 @@ void memory_region_init_ram(MemoryRegion *mr,
>       vmstate_register_ram(mr, owner_dev);
>   }
>   
> +void memory_region_init_ram_protected(MemoryRegion *mr,
> +                                      Object *owner,
> +                                      const char *name,
> +                                      uint64_t size,
> +                                      Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, RAM_PROTECTED,
> +                                     errp);
> +}
> +
> +void memory_region_init_ram(MemoryRegion *mr,
> +                            Object *owner,
> +                            const char *name,
> +                            uint64_t size,
> +                            Error **errp)
> +{
> +        memory_region_init_ram_flags(mr, owner, name, size, 0, errp);
> +}
> +
>   void memory_region_init_rom(MemoryRegion *mr,
>                               Object *owner,
>                               const char *name,
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6bdd944fe880..bf66c81e7255 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1978,7 +1978,7 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
>       Error *local_err = NULL;
>   
>       assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE | RAM_PREALLOC |
> -                          RAM_NORESERVE)) == 0);
> +                          RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       assert(!host ^ (ram_flags & RAM_PREALLOC));
>   
>       size = HOST_PAGE_ALIGN(size);
> @@ -2012,7 +2012,7 @@ RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>   RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags,
>                            MemoryRegion *mr, Error **errp)
>   {
> -    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE)) == 0);
> +    assert((ram_flags & ~(RAM_SHARED | RAM_NORESERVE | RAM_PROTECTED)) == 0);
>       return qemu_ram_alloc_internal(size, size, NULL, NULL, ram_flags, mr, errp);
>   }
>   

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb