qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 3/9] backends/hostmem-file: Add "rom" property to support


From: David Hildenbrand
Subject: Re: [PATCH v2 3/9] backends/hostmem-file: Add "rom" property to support VM templating with R/O files
Date: Wed, 23 Aug 2023 14:43:48 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0

On 22.08.23 16:26, ThinerLogoer wrote:
Hello,

At 2023-08-22 19:44:51, "David Hildenbrand" <david@redhat.com> wrote:
For now, "share=off,readonly=on" would always result in us opening the
file R/O and mmap'ing the opened file MAP_PRIVATE R/O -- effectively
turning it into ROM.

Especially for VM templating, "share=off" is a common use case. However,
that use case is impossible with files that lack write permissions,
because "share=off,readonly=on" will not give us writable RAM.

The sole user of ROM via memory-backend-file are R/O NVDIMMs, but as we
have users (Kata Containers) that rely on the existing behavior --
malicious VMs should not be able to consume COW memory for R/O NVDIMMs --
we cannot change the semantics of "share=off,readonly=on"

So let's add a new "rom" property with on/off/auto values. "auto" is
the default and what most people will use: for historical reasons, to not
change the old semantics, it defaults to the value of the "readonly"
property.

For VM templating, one can now use:
    -object memory-backend-file,share=off,readonly=on,rom=off,...

But we'll disallow:
    -object memory-backend-file,share=on,readonly=on,rom=off,...
because we would otherwise get an error when trying to mmap the R/O file
shared and writable. An explicit error message is cleaner.

We will also disallow for now:
    -object memory-backend-file,share=off,readonly=off,rom=on,...
    -object memory-backend-file,share=on,readonly=off,rom=on,...
It's not harmful, but also not really required for now.

Alternatives that were abandoned:
* Make "unarmed=on" for the NVDIMM set the memory region container
  readonly. We would still see a change of ROM->RAM and possibly run
  into memslot limits with vhost-user. Further, there might be use cases
  for "unarmed=on" that should still allow writing to that memory
  (temporary files, system RAM, ...).
* Add a new "readonly=on/off/auto" parameter for NVDIMMs. Similar issues
  as with "unarmed=on".
* Make "readonly" consume "on/off/file" instead of being a 'bool' type.
  This would slightly changes the behavior of the "readonly" parameter:
  values like true/false (as accepted by a 'bool'type) would no longer be
  accepted.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
backends/hostmem-file.c | 59 ++++++++++++++++++++++++++++++++++++++++-
qapi/qom.json           |  6 ++++-
qemu-options.hx         | 10 ++++++-
3 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index ef2d5533af..361d4a8103 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -18,6 +18,8 @@
#include "sysemu/hostmem.h"
#include "qom/object_interfaces.h"
#include "qom/object.h"
+#include "qapi/visitor.h"
+#include "qapi/qapi-visit-common.h"

OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)

@@ -31,6 +33,7 @@ struct HostMemoryBackendFile {
     bool discard_data;
     bool is_pmem;
     bool readonly;
+    OnOffAuto rom;
};

static void
@@ -53,9 +56,33 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error 
**errp)
         return;
     }

+    switch (fb->rom) {
+    case ON_OFF_AUTO_AUTO:
+        /* Traditionally, opening the file readonly always resulted in ROM. */
+        fb->rom = fb->readonly ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+        break;
+    case ON_OFF_AUTO_ON:
+        if (!fb->readonly) {
+            error_setg(errp, "property 'rom' = 'on' is not supported with"
+                       " 'readonly' = 'off'");
+            return;
+        }
+        break;
+    case ON_OFF_AUTO_OFF:
+        if (fb->readonly && backend->share) {
+            error_setg(errp, "property 'rom' = 'off' is incompatible with"
+                       " 'readonly' = 'on' and 'share' = 'on'");
+            return;
+        }
+        break;
+    default:
+        assert(false);
+    }
+
     name = host_memory_backend_get_name(backend);
     ram_flags = backend->share ? RAM_SHARED : 0;
-    ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0;
+    ram_flags |= fb->readonly ? RAM_READONLY_FD : 0;
+    ram_flags |= fb->rom == ON_OFF_AUTO_ON ? RAM_READONLY : 0;
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
     ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
     ram_flags |= RAM_NAMED_FILE;
@@ -201,6 +228,32 @@ static void file_memory_backend_set_readonly(Object *obj, 
bool value,
     fb->readonly = value;
}

+static void file_memory_backend_get_rom(Object *obj, Visitor *v,
+                                        const char *name, void *opaque,
+                                        Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
+    OnOffAuto rom = fb->rom;
+
+    visit_type_OnOffAuto(v, name, &rom, errp);
+}
+
+static void file_memory_backend_set_rom(Object *obj, Visitor *v,
+                                        const char *name, void *opaque,
+                                        Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
+
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "cannot change property '%s' of %s.", name,
+                   object_get_typename(obj));
+        return;
+    }
+
+    visit_type_OnOffAuto(v, name, &fb->rom, errp);
+}
+
static void file_backend_unparent(Object *obj)
{
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -243,6 +296,10 @@ file_backend_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, "readonly",
         file_memory_backend_get_readonly,
         file_memory_backend_set_readonly);
+    object_class_property_add(oc, "rom", "OnOffAuto",
+        file_memory_backend_get_rom, file_memory_backend_set_rom, NULL, NULL);
+    object_class_property_set_description(oc, "rom",
+        "Whether to create Read Only Memory (ROM)");
}

static void file_backend_instance_finalize(Object *o)
diff --git a/qapi/qom.json b/qapi/qom.json
index fa3e88c8e6..0cf83c6f39 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -668,6 +668,9 @@
# @readonly: if true, the backing file is opened read-only; if false,
#     it is opened read-write.  (default: false)
#
+# @rom: whether to create Read Only Memory (ROM).  If set to 'auto', it
+#       defaults to the value of @readonly.  (default: auto, since 8.2)
+#
# Since: 2.1
##
{ 'struct': 'MemoryBackendFileProperties',
@@ -677,7 +680,8 @@
             '*discard-data': 'bool',
             'mem-path': 'str',
             '*pmem': { 'type': 'bool', 'if': 'CONFIG_LIBPMEM' },
-            '*readonly': 'bool' } }
+            '*readonly': 'bool',
+            '*rom': 'OnOffAuto' } }

##
# @MemoryBackendMemfdProperties:
diff --git a/qemu-options.hx b/qemu-options.hx
index 29b98c3d4c..03ce0b0a30 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4976,7 +4976,7 @@ SRST
     they are specified. Note that the 'id' property must be set. These
     objects are placed in the '/objects' path.

-    ``-object 
memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off``
+    ``-object 
memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off,rom=on|off|auto``
         Creates a memory file backend object, which can be used to back
         the guest RAM with huge pages.

@@ -5066,6 +5066,14 @@ SRST
         The ``readonly`` option specifies whether the backing file is opened
         read-only or read-write (default).

+        The ``rom`` option specifies whether to create Read Only Memory (ROM)
+        that cannot be modified by the VM. If set to ``on``, the VM cannot
+        modify the memory. If set to ``off``, the VM can modify the memory.
+        If set to ``auto`` (default), the value of the ``readonly`` property
+        is used. This option is primarily helpful for VM templating, where we
+        want to open a file readonly (``readonly=on``) and allow private
+        modifications of the memory by the VM (``share=off``, ``rom=off``).
+
     ``-object 
memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
         Creates a memory backend object, which can be used to back the
         guest RAM. Memory backend objects offer more control than the

In one word, I'd suggest advertising the existence of "rom" option more 
invasively, whenever
private file mapping is used.

IMHO you should probably be more invasive here to warn unconditionally when
the memory backend file is going to be opened readwrite but is mapped non 
shared.

As Daniel said, we should not add new warnings for sane use cases. But we can 
indeed give a hint when opening the file failed, see below.


I as a user find the patch series indeed work functionally when I am aware of the 
"rom"
option - but what if I am not aware, the outcome is still that qemu tried
to open the file readwrite even when it is going to be mapped private.

Yes, the implicit "readonly=off" is active in any case, and we cannot change 
that due to existing users unfortunately.


When the file is readonly, the error message is:
```
qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission 
denied
```

This should be probably helpful if qemu found that the file exists as a regular 
file and
is mapped private, to say something like

```
qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission 
denied
tip: mapping is private and ram file is probably readonly, maybe you should append 
"readonly=on,rom=off"
to "-object memory-backend-file,..." option list. see documentation xxx for 
details
```

What about the following, if we can indeed open the file R/O and we're dealing 
witha  private mapping:

qemu-system-x86_64: can't open backing store tmp-file for guest RAM: Permission 
denied
Consider opening the backing store read-only using '-object 
memory-backend-file,readonly=on,rom=off,...' (see "VM templating" documentation)

?

--
Cheers,

David / dhildenb




reply via email to

[Prev in Thread] Current Thread [Next in Thread]