| Message ID | 20230822114504.239505-4-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-backend-file related improvements and VM templating support | expand |
David Hildenbrand <david@redhat.com> writes: > 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> [...] > 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 > ## The commit message discusses how @readonly, @rom and @share interact. The doc comments don't, and users have to guess. I can see two ways to help users: 1. Describe their interaction in full, so users can understand how to get from them what they need. 2. Provide suitable guidance on how to use them. > { '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``). > + Here, you provide some guidance. > ``-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
On 22.08.23 15:27, Markus Armbruster wrote: > David Hildenbrand <david@redhat.com> writes: > >> 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> > > [...] > >> 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 >> ## > > The commit message discusses how @readonly, @rom and @share interact. > The doc comments don't, and users have to guess. > > I can see two ways to help users: > > 1. Describe their interaction in full, so users can understand how to > get from them what they need. > > 2. Provide suitable guidance on how to use them. > >> { '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``). >> + > > Here, you provide some guidance. Is that sufficient in your opinion? Then I could similarly replicate (unfortunately) it in the qapi/qom.json doc? Thanks Markus!
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. 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. 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 ``` to advertise the "rom" option. -- Regards, logoerthiner
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) ?
At 2023-08-23 20:43:48, "David Hildenbrand" <david@redhat.com> wrote: >>> + 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. > Well I don't think it's completely sane use cases though we can keep it for backward compatibility. A nonshared memory map backed by an fd opened readwrite is probably problematic and not what one usually expect. Either it should be shared or it should opened readonly. Current behavior sticks to the file being opened readwrite by default but warning can be raised when readonly holds (file path does not prefix with /dev, and is a nonempty regular file, and mapping is private), indicate that the user may probably want a "readonly=on,rom=off" setup. >> >> 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. > Yeah I am ok with that now, and I found a way for my setup to work with your patches, so I am happy with it. >> >> 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) > >? This is good. Wording might need improvement. (Are qemu error messages always this cold?) -- Regards, logoerthiner
On 23.08.23 16:47, ThinerLogoer wrote: > At 2023-08-23 20:43:48, "David Hildenbrand" <david@redhat.com> wrote: >>>> + 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. >> > > Well I don't think it's completely sane use cases though we can keep it for backward > compatibility. Well, yes, but these are sane use cases that have been using that way of dealing with files forever. We cannot simply change their usage, unfortunately. Having that said, your important use case is currently VM templating. Note that that's not what the many other QEMU users actually do. So I can understand why you want a different behavior and more hints; but we have to balance a bit here (after all, I'm writing documentation how to do VM templating for a reason ;) ). [...] >>> >>> 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) >> >> ? > > This is good. Wording might need improvement. (Are qemu error messages always this cold?) Maybe. Maybe just my writing style :P Looking at most error_append_hint(), they are fairly neutral/cold.
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
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(-)