Re: [PATCH V8 04/39] memory: RAM_ANON flag

[Steven Sistare]

On 6/15/2022 4:25 PM, David Hildenbrand wrote:
> On 15.06.22 16:51, Steve Sistare wrote:
>> A memory-backend-ram or a memory-backend-memfd block with the RAM_SHARED
>> flag set is not migrated when migrate_ignore_shared() is true, but this
>> is wrong, because it has no named backing store, and its contents will be
>> lost.  Define a new flag RAM_ANON to distinguish this case.  Cpr will also
>> test this flag, for similar reasons.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  backends/hostmem-epc.c   |  2 +-
>>  backends/hostmem-memfd.c |  1 +
>>  backends/hostmem-ram.c   |  1 +
>>  include/exec/memory.h    |  3 +++
>>  include/exec/ram_addr.h  |  1 +
>>  migration/ram.c          |  3 ++-
>>  softmmu/physmem.c        | 12 +++++++++---
>>  7 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/backends/hostmem-epc.c b/backends/hostmem-epc.c
>> index 037292d..cb06255 100644
>> --- a/backends/hostmem-epc.c
>> +++ b/backends/hostmem-epc.c
>> @@ -37,7 +37,7 @@ sgx_epc_backend_memory_alloc(HostMemoryBackend *backend, 
>> Error **errp)
>>      }
>>  
>>      name = object_get_canonical_path(OBJECT(backend));
>> -    ram_flags = (backend->share ? RAM_SHARED : 0) | RAM_PROTECTED;
>> +    ram_flags = (backend->share ? RAM_SHARED : 0) | RAM_PROTECTED | 
>> MAP_ANON;
> 
> I'm pretty sure that doesn't compile. -> RAM_ANON

Oh it does, but not what we want!  Thanks for the catch.

>>      memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
>>                                     name, backend->size, ram_flags,
>>                                     fd, 0, errp);
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index 3fc85c3..c9d8001 100644
>> --- a/backends/hostmem-memfd.c
>> +++ b/backends/hostmem-memfd.c
>> @@ -55,6 +55,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, 
>> Error **errp)
>>      name = host_memory_backend_get_name(backend);
>>      ram_flags = backend->share ? RAM_SHARED : 0;
>>      ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>> +    ram_flags |= RAM_ANON;
>>      memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
>>                                     backend->size, ram_flags, fd, 0, errp);
>>      g_free(name);
>> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
>> index b8e55cd..5e80149 100644
>> --- a/backends/hostmem-ram.c
>> +++ b/backends/hostmem-ram.c
>> @@ -30,6 +30,7 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error 
>> **errp)
>>      name = host_memory_backend_get_name(backend);
>>      ram_flags = backend->share ? RAM_SHARED : 0;
>>      ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>> +    ram_flags |= RAM_ANON;
>>      memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend), 
>> name,
>>                                             backend->size, ram_flags, errp);
>>      g_free(name);
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index f1c1945..0daddd7 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -203,6 +203,9 @@ typedef struct IOMMUTLBEvent {
>>  /* RAM that isn't accessible through normal means. */
>>  #define RAM_PROTECTED (1 << 8)
>>  
>> +/* RAM has no name outside the qemu process. */
>> +#define RAM_ANON (1 << 9)
> 
> That name is a bit misleading because it mangles anonymous memory with
> an anonymous file, which doesn't provide anonymous memory in "kernel
> speak". Please find a better name, some idea below ...
> 
> I think what you actual want to know is: is this from a real file,
> instead of from an anonymous file or anonymous memory. A real file can
> be re-opened and remapped after closing QEMU. Further, you need
> MAP_SHARED semantics.
> 
> 
> /* RAM maps a real file instead of an anonymous file or no file/fd. */
> #define RAM_REAL_FILE (1 << 9)
> 
> bool ramblock_maps_real_file(RAMBlock *rb)
> {
>     return rb->flags & RAM_REAL_FILE;
> }
> 
> 
> Maybe we can come up with a better name for "real file".

Sure.  Ideas:
  RAM_FILE
  RAM_NAMED
  RAM_NAMED_FILE

> Set the flag from applicable callsites. When setting the flag
> internally, assert that we don't have a fd -- that cannot possibly make
> sense.

It will only be set in hostmem-file.c

> At applicable callsites check for ramblock_maps_real_file() and that
> it's actually a shared mapping. If not, it cannot be preserved by
> restarting QEMU (easily, there might be ways for memfd involving other
> processes).

Memfd is allowed for cpr restart by virtue of being shared and having an
fd which can be mapped, which I test for.  See ram_is_volatile in patch 22.
ramblock_is_anon() becomes !ramblock_is_file().

- Steve