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.sist...@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
