[PATCH 0/3] nvdimm: read-only file support

[PATCH 0/3] nvdimm: read-only file support

[Stefan Hajnoczi]

There is currently no way to back an NVDIMM with a read-only file so it can be
safely shared between untrusted guests.

Introduce an -object memory-backend-file,readonly=on|off option.

Julio Montes sent an earlier patch here:
https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/

Eric Ernst requested this feature again for Kata Containers so I gave it a try.

Stefan Hajnoczi (3):
  memory: add readonly support to memory_region_init_ram_from_file()
  hostmem-file: add readonly=on|off option
  nvdimm: honor -object memory-backend-file,readonly=on option

 docs/nvdimm.txt           |  8 +++++++-
 include/exec/memory.h     |  2 ++
 include/exec/ram_addr.h   |  5 +++--
 include/qemu/mmap-alloc.h |  2 ++
 backends/hostmem-file.c   | 26 +++++++++++++++++++++++++-
 exec.c                    | 18 +++++++++++-------
 hw/mem/nvdimm.c           |  4 ++++
 softmmu/memory.c          |  7 +++++--
 util/mmap-alloc.c         | 10 ++++++----
 util/oslib-posix.c        |  2 +-
 qemu-options.hx           |  5 ++++-
 11 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.26.2

[PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Stefan Hajnoczi]

There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
creating a memory region from a file. This functionality is needed since
the underlying host file may not allow writing.

Add a bool readonly argument to memory_region_init_ram_from_file() and
the APIs it calls.

Extend memory_region_init_ram_from_file() rather than introducing a
memory_region_init_rom_from_file() API so that callers can easily make a
choice between read/write and read-only at runtime without calling
different APIs.

No new RAMBlock flag is introduced for read-only because it's unclear
whether RAMBlocks need to know that they are read-only. Pass a bool
readonly argument instead.

Both of these design decisions can be changed in the future. It just
seemed like the simplest approach to me.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/exec/memory.h     |  2 ++
 include/exec/ram_addr.h   |  5 +++--
 include/qemu/mmap-alloc.h |  2 ++
 backends/hostmem-file.c   |  2 +-
 exec.c                    | 18 +++++++++++-------
 softmmu/memory.c          |  7 +++++--
 util/mmap-alloc.c         | 10 ++++++----
 util/oslib-posix.c        |  2 +-
 8 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 307e527835..1ae7b31e3a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -884,6 +884,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
  *             - RAM_PMEM: the memory is persistent memory
  *             Other bits are ignored now.
  * @path: the path in which to allocate the RAM.
+ * @readonly: true to open @path for reading, false for read/write.
  * @errp: pointer to Error*, to store an error if it happens.
  *
  * Note that this function does not do anything to cause the data in the
@@ -896,6 +897,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       uint64_t align,
                                       uint32_t ram_flags,
                                       const char *path,
+                                      bool readonly,
                                       Error **errp);
 
 /**
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 3ef729a23c..2a0360a0f2 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -110,6 +110,7 @@ long qemu_maxrampagesize(void);
  *              - RAM_PMEM: the backend @mem_path or @fd is persistent memory
  *              Other bits are ignored.
  *  @mem_path or @fd: specify the backing file or device
+ *  @readonly: true to open @path for reading, false for read/write.
  *  @errp: pointer to Error*, to store an error if it happens
  *
  * Return:
@@ -118,9 +119,9 @@ long qemu_maxrampagesize(void);
  */
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                    uint32_t ram_flags, const char *mem_path,
-                                   Error **errp);
+                                   bool readonly, Error **errp);
 RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
-                                 uint32_t ram_flags, int fd,
+                                 uint32_t ram_flags, int fd, bool readonly,
                                  Error **errp);
 
 RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
index e786266b92..8b7a5c70f3 100644
--- a/include/qemu/mmap-alloc.h
+++ b/include/qemu/mmap-alloc.h
@@ -14,6 +14,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
  *  @size: the number of bytes to be mmaped
  *  @align: if not zero, specify the alignment of the starting mapping address;
  *          otherwise, the alignment in use will be determined by QEMU.
+ *  @readonly: true for a read-only mapping, false for read/write.
  *  @shared: map has RAM_SHARED flag.
  *  @is_pmem: map has RAM_PMEM flag.
  *
@@ -24,6 +25,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
 void *qemu_ram_mmap(int fd,
                     size_t size,
                     size_t align,
+                    bool readonly,
                     bool shared,
                     bool is_pmem);
 
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 5b819020b4..37c70acfe2 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -57,7 +57,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                      backend->size, fb->align,
                                      (backend->share ? RAM_SHARED : 0) |
                                      (fb->is_pmem ? RAM_PMEM : 0),
-                                     fb->mem_path, errp);
+                                     fb->mem_path, false, errp);
     g_free(name);
 #endif
 }
diff --git a/exec.c b/exec.c
index 6f381f98e2..5874e999ab 100644
--- a/exec.c
+++ b/exec.c
@@ -1769,6 +1769,7 @@ static int64_t get_file_align(int fd)
 
 static int file_ram_open(const char *path,
                          const char *region_name,
+                         bool readonly,
                          bool *created,
                          Error **errp)
 {
@@ -1779,7 +1780,7 @@ static int file_ram_open(const char *path,
 
     *created = false;
     for (;;) {
-        fd = open(path, O_RDWR);
+        fd = open(path, readonly ? O_RDONLY : O_RDWR);
         if (fd >= 0) {
             /* @path names an existing file, use it */
             break;
@@ -1831,6 +1832,7 @@ static int file_ram_open(const char *path,
 static void *file_ram_alloc(RAMBlock *block,
                             ram_addr_t memory,
                             int fd,
+                            bool readonly,
                             bool truncate,
                             Error **errp)
 {
@@ -1881,7 +1883,7 @@ static void *file_ram_alloc(RAMBlock *block,
         perror("ftruncate");
     }
 
-    area = qemu_ram_mmap(fd, memory, block->mr->align,
+    area = qemu_ram_mmap(fd, memory, block->mr->align, readonly,
                          block->flags & RAM_SHARED, block->flags & RAM_PMEM);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
@@ -2313,7 +2315,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
 
 #ifdef CONFIG_POSIX
 RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
-                                 uint32_t ram_flags, int fd,
+                                 uint32_t ram_flags, int fd, bool readonly,
                                  Error **errp)
 {
     RAMBlock *new_block;
@@ -2367,7 +2369,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
     new_block->used_length = size;
     new_block->max_length = size;
     new_block->flags = ram_flags;
-    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
+    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
+                                     !file_size, errp);
     if (!new_block->host) {
         g_free(new_block);
         return NULL;
@@ -2386,18 +2389,19 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
 
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                    uint32_t ram_flags, const char *mem_path,
-                                   Error **errp)
+                                   bool readonly, Error **errp)
 {
     int fd;
     bool created;
     RAMBlock *block;
 
-    fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
+    fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
+                       errp);
     if (fd < 0) {
         return NULL;
     }
 
-    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
+    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, readonly, errp);
     if (!block) {
         if (created) {
             unlink(mem_path);
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..d228635bb3 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       uint64_t align,
                                       uint32_t ram_flags,
                                       const char *path,
+                                      bool readonly,
                                       Error **errp)
 {
     Error *err = NULL;
     memory_region_init(mr, owner, name, size);
     mr->ram = true;
+    mr->readonly = readonly;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
     mr->align = align;
-    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
+    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
+                                             readonly, &err);
     mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
     if (err) {
         mr->size = int128_zero();
@@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
     mr->destructor = memory_region_destructor_ram;
     mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
                                            share ? RAM_SHARED : 0,
-                                           fd, &err);
+                                           fd, false, &err);
     mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
     if (err) {
         mr->size = int128_zero();
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 27dcccd8ec..890fda6a35 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
 void *qemu_ram_mmap(int fd,
                     size_t size,
                     size_t align,
+                    bool readonly,
                     bool shared,
                     bool is_pmem)
 {
+    int prot;
     int flags;
     int map_sync_flags = 0;
     int guardfd;
@@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
 
     offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
 
-    ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
-               flags | map_sync_flags, fd, 0);
+    prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
+
+    ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
 
     if (ptr == MAP_FAILED && map_sync_flags) {
         if (errno == ENOTSUP) {
@@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
          * if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
          * we will remove these flags to handle compatibility.
          */
-        ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
-                   flags, fd, 0);
+        ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
     }
 
     if (ptr == MAP_FAILED) {
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index ad8001a4ad..236b3a88c1 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -227,7 +227,7 @@ void *qemu_memalign(size_t alignment, size_t size)
 void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared)
 {
     size_t align = QEMU_VMALLOC_ALIGN;
-    void *ptr = qemu_ram_mmap(-1, size, align, shared, false);
+    void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false);
 
     if (ptr == MAP_FAILED) {
         return NULL;
-- 
2.26.2

[PATCH 2/3] hostmem-file: add readonly=on|off option

[Stefan Hajnoczi]

Let -object memory-backend-file work on read-only files when the
readonly=on option is given. This can be used to share the contents of a
file between multiple guests while preventing them from consuming
Copy-on-Write memory if guests dirty the pages, for example.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
 qemu-options.hx         |  5 ++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 37c70acfe2..6bd5bf9b91 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
     uint64_t align;
     bool discard_data;
     bool is_pmem;
+    bool readonly;
 };
 
 static void
@@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                      backend->size, fb->align,
                                      (backend->share ? RAM_SHARED : 0) |
                                      (fb->is_pmem ? RAM_PMEM : 0),
-                                     fb->mem_path, false, errp);
+                                     fb->mem_path, fb->readonly, errp);
     g_free(name);
 #endif
 }
@@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
     fb->is_pmem = value;
 }
 
+static bool file_memory_backend_get_readonly(Object *o, Error **errp)
+{
+    return MEMORY_BACKEND_FILE(o)->readonly;
+}
+
+static void file_memory_backend_set_readonly(Object *o, bool value,
+                                             Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "cannot change property 'readonly' of %s.",
+                   object_get_typename(o));
+        return;
+    }
+
+    fb->readonly = value;
+}
+
 static void file_backend_unparent(Object *obj)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -183,6 +204,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
         NULL, NULL);
     object_class_property_add_bool(oc, "pmem",
         file_memory_backend_get_pmem, file_memory_backend_set_pmem);
+    object_class_property_add_bool(oc, "readonly",
+        file_memory_backend_get_readonly,
+        file_memory_backend_set_readonly);
 }
 
 static void file_backend_instance_finalize(Object *o)
diff --git a/qemu-options.hx b/qemu-options.hx
index 708583b4ce..d834e00b0d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4369,7 +4369,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``
+    ``-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,readonly=on|off``
         Creates a memory file backend object, which can be used to back
         the guest RAM with huge pages.
 
@@ -4452,6 +4452,9 @@ SRST
         4.15) and the filesystem of ``mem-path`` mounted with DAX
         option.
 
+        The ``readonly`` option specifies whether the backing file is opened
+        read-only or read-write (default).
+
     ``-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
-- 
2.26.2

[PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option

[Stefan Hajnoczi]

Make it possible to present read-only files to the guest as "unarmed"
NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/nvdimm.txt | 8 +++++++-
 hw/mem/nvdimm.c | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index c2c6e441b3..c0b52de111 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -17,7 +17,7 @@ following command line options:
 
  -machine pc,nvdimm
  -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
- -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
+ -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
  -device nvdimm,id=nvdimm1,memdev=mem1
 
 Where,
@@ -42,6 +42,12 @@ Where,
    "share=off", then guest writes won't be applied to the backend
    file and thus will be invisible to other guests.
 
+   "readonly=on/off" controls whether the the file $PATH is opened read-only or
+   read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping
+   Structure "NVDIMM State Flags" Bit 3 indicating that the device is "unarmed"
+   and cannot accept persistent writes. Linux guest drivers set the device to
+   read-only when this bit is present.
+
  - "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
    device whose storage is provided by above memory backend device.
 
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index e1574bc07c..694223450e 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
                              "nvdimm-memory", mr, 0, pmem_size);
     memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
     nvdimm->nvdimm_mr->align = align;
+
+    if (memory_region_is_rom(mr)) {
+        nvdimm->unarmed = true; /* this device is read-only */
+    }
 }
 
 static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
-- 
2.26.2

Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Philippe Mathieu-Daudé]

Hi Stefan,

On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
> creating a memory region from a file. This functionality is needed since
> the underlying host file may not allow writing.
> 
> Add a bool readonly argument to memory_region_init_ram_from_file() and
> the APIs it calls.
> 
> Extend memory_region_init_ram_from_file() rather than introducing a
> memory_region_init_rom_from_file() API so that callers can easily make a
> choice between read/write and read-only at runtime without calling
> different APIs.

What happens if we call:

 memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
 memory_region_set_readonly(mr, false);

?

> 
> No new RAMBlock flag is introduced for read-only because it's unclear
> whether RAMBlocks need to know that they are read-only. Pass a bool
> readonly argument instead.
> 
> Both of these design decisions can be changed in the future. It just
> seemed like the simplest approach to me.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/exec/memory.h     |  2 ++
>  include/exec/ram_addr.h   |  5 +++--
>  include/qemu/mmap-alloc.h |  2 ++
>  backends/hostmem-file.c   |  2 +-
>  exec.c                    | 18 +++++++++++-------
>  softmmu/memory.c          |  7 +++++--
>  util/mmap-alloc.c         | 10 ++++++----
>  util/oslib-posix.c        |  2 +-
>  8 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 307e527835..1ae7b31e3a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -884,6 +884,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>   *             - RAM_PMEM: the memory is persistent memory
>   *             Other bits are ignored now.
>   * @path: the path in which to allocate the RAM.
> + * @readonly: true to open @path for reading, false for read/write.
>   * @errp: pointer to Error*, to store an error if it happens.
>   *
>   * Note that this function does not do anything to cause the data in the
> @@ -896,6 +897,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>                                        uint64_t align,
>                                        uint32_t ram_flags,
>                                        const char *path,
> +                                      bool readonly,
>                                        Error **errp);
>  
[...]
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..d228635bb3 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>                                        uint64_t align,
>                                        uint32_t ram_flags,
>                                        const char *path,
> +                                      bool readonly,
>                                        Error **errp)
>  {
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->readonly = readonly;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->align = align;
> -    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
> +    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
> +                                             readonly, &err);
>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
> @@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
>                                             share ? RAM_SHARED : 0,
> -                                           fd, &err);
> +                                           fd, false, &err);
>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..890fda6a35 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>  void *qemu_ram_mmap(int fd,
>                      size_t size,
>                      size_t align,
> +                    bool readonly,
>                      bool shared,
>                      bool is_pmem)
>  {
> +    int prot;
>      int flags;
>      int map_sync_flags = 0;
>      int guardfd;
> @@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
>  
>      offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
>  
> -    ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> -               flags | map_sync_flags, fd, 0);
> +    prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
> +
> +    ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
>  
>      if (ptr == MAP_FAILED && map_sync_flags) {
>          if (errno == ENOTSUP) {
> @@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
>           * if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
>           * we will remove these flags to handle compatibility.
>           */
> -        ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> -                   flags, fd, 0);
> +        ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
>      }
>  
>      if (ptr == MAP_FAILED) {
[...]

Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Philippe Mathieu-Daudé]

On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
>> creating a memory region from a file. This functionality is needed since
>> the underlying host file may not allow writing.
>>
>> Add a bool readonly argument to memory_region_init_ram_from_file() and
>> the APIs it calls.
>>
>> Extend memory_region_init_ram_from_file() rather than introducing a
>> memory_region_init_rom_from_file() API so that callers can easily make a
>> choice between read/write and read-only at runtime without calling
>> different APIs.
> 
> What happens if we call:
> 
>  memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
>  memory_region_set_readonly(mr, false);

In case my error is not obvious, I meant:

   memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
   memory_region_set_readonly(mr, false);

> 
> ?
> 
>>
>> No new RAMBlock flag is introduced for read-only because it's unclear
>> whether RAMBlocks need to know that they are read-only. Pass a bool
>> readonly argument instead.
>>
>> Both of these design decisions can be changed in the future. It just
>> seemed like the simplest approach to me.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/exec/memory.h     |  2 ++
>>  include/exec/ram_addr.h   |  5 +++--
>>  include/qemu/mmap-alloc.h |  2 ++
>>  backends/hostmem-file.c   |  2 +-
>>  exec.c                    | 18 +++++++++++-------
>>  softmmu/memory.c          |  7 +++++--
>>  util/mmap-alloc.c         | 10 ++++++----
>>  util/oslib-posix.c        |  2 +-
>>  8 files changed, 31 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 307e527835..1ae7b31e3a 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -884,6 +884,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>>   *             - RAM_PMEM: the memory is persistent memory
>>   *             Other bits are ignored now.
>>   * @path: the path in which to allocate the RAM.
>> + * @readonly: true to open @path for reading, false for read/write.
>>   * @errp: pointer to Error*, to store an error if it happens.
>>   *
>>   * Note that this function does not do anything to cause the data in the
>> @@ -896,6 +897,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>                                        uint64_t align,
>>                                        uint32_t ram_flags,
>>                                        const char *path,
>> +                                      bool readonly,
>>                                        Error **errp);
>>  
> [...]
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index af25987518..d228635bb3 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>                                        uint64_t align,
>>                                        uint32_t ram_flags,
>>                                        const char *path,
>> +                                      bool readonly,
>>                                        Error **errp)
>>  {
>>      Error *err = NULL;
>>      memory_region_init(mr, owner, name, size);
>>      mr->ram = true;
>> +    mr->readonly = readonly;
>>      mr->terminates = true;
>>      mr->destructor = memory_region_destructor_ram;
>>      mr->align = align;
>> -    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
>> +    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
>> +                                             readonly, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>>          mr->size = int128_zero();
>> @@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>>      mr->destructor = memory_region_destructor_ram;
>>      mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
>>                                             share ? RAM_SHARED : 0,
>> -                                           fd, &err);
>> +                                           fd, false, &err);
>>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>>      if (err) {
>>          mr->size = int128_zero();
>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>> index 27dcccd8ec..890fda6a35 100644
>> --- a/util/mmap-alloc.c
>> +++ b/util/mmap-alloc.c
>> @@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>>  void *qemu_ram_mmap(int fd,
>>                      size_t size,
>>                      size_t align,
>> +                    bool readonly,
>>                      bool shared,
>>                      bool is_pmem)
>>  {
>> +    int prot;
>>      int flags;
>>      int map_sync_flags = 0;
>>      int guardfd;
>> @@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
>>  
>>      offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
>>  
>> -    ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
>> -               flags | map_sync_flags, fd, 0);
>> +    prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
>> +
>> +    ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
>>  
>>      if (ptr == MAP_FAILED && map_sync_flags) {
>>          if (errno == ENOTSUP) {
>> @@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
>>           * if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
>>           * we will remove these flags to handle compatibility.
>>           */
>> -        ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
>> -                   flags, fd, 0);
>> +        ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
>>      }
>>  
>>      if (ptr == MAP_FAILED) {
> [...]
> 

Re: [PATCH 0/3] nvdimm: read-only file support

[Michael S. Tsirkin]

On Tue, Aug 04, 2020 at 11:12:41AM +0100, Stefan Hajnoczi wrote:
> There is currently no way to back an NVDIMM with a read-only file so it can be
> safely shared between untrusted guests.
> 
> Introduce an -object memory-backend-file,readonly=on|off option.
> 
> Julio Montes sent an earlier patch here:
> https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/


makes sense:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> Eric Ernst requested this feature again for Kata Containers so I gave it a try.
> 
> Stefan Hajnoczi (3):
>   memory: add readonly support to memory_region_init_ram_from_file()
>   hostmem-file: add readonly=on|off option
>   nvdimm: honor -object memory-backend-file,readonly=on option
> 
>  docs/nvdimm.txt           |  8 +++++++-
>  include/exec/memory.h     |  2 ++
>  include/exec/ram_addr.h   |  5 +++--
>  include/qemu/mmap-alloc.h |  2 ++
>  backends/hostmem-file.c   | 26 +++++++++++++++++++++++++-
>  exec.c                    | 18 +++++++++++-------
>  hw/mem/nvdimm.c           |  4 ++++
>  softmmu/memory.c          |  7 +++++--
>  util/mmap-alloc.c         | 10 ++++++----
>  util/oslib-posix.c        |  2 +-
>  qemu-options.hx           |  5 ++++-
>  11 files changed, 70 insertions(+), 19 deletions(-)
> 
> -- 
> 2.26.2
> 

Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

On Tue, Aug 04, 2020 at 02:26:22PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
> > Hi Stefan,
> > 
> > On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> >> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
> >> creating a memory region from a file. This functionality is needed since
> >> the underlying host file may not allow writing.
> >>
> >> Add a bool readonly argument to memory_region_init_ram_from_file() and
> >> the APIs it calls.
> >>
> >> Extend memory_region_init_ram_from_file() rather than introducing a
> >> memory_region_init_rom_from_file() API so that callers can easily make a
> >> choice between read/write and read-only at runtime without calling
> >> different APIs.
> > 
> > What happens if we call:
> > 
> >  memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
> >  memory_region_set_readonly(mr, false);
> 
> In case my error is not obvious, I meant:
> 
>    memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
>    memory_region_set_readonly(mr, false);

Since the mmap was made using PROT_READ any store instructions to the
memory will fault.

Is there some scenario where memory_region_set_readonly() is called? I
can't find one.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Philippe Mathieu-Daudé]

On 8/4/20 3:47 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 04, 2020 at 02:26:22PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Stefan,
>>>
>>> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>>>> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
>>>> creating a memory region from a file. This functionality is needed since
>>>> the underlying host file may not allow writing.
>>>>
>>>> Add a bool readonly argument to memory_region_init_ram_from_file() and
>>>> the APIs it calls.
>>>>
>>>> Extend memory_region_init_ram_from_file() rather than introducing a
>>>> memory_region_init_rom_from_file() API so that callers can easily make a
>>>> choice between read/write and read-only at runtime without calling
>>>> different APIs.
>>>
>>> What happens if we call:
>>>
>>>  memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
>>>  memory_region_set_readonly(mr, false);
>>
>> In case my error is not obvious, I meant:
>>
>>    memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
>>    memory_region_set_readonly(mr, false);
> 
> Since the mmap was made using PROT_READ any store instructions to the
> memory will fault.
> 
> Is there some scenario where memory_region_set_readonly() is called? I
> can't find one.

Not in the current code base, but I was wondering about the API abuses.

I see in the next patch the property is protected:

    if (host_memory_backend_mr_inited(backend)) {
        error_setg(errp, "cannot change property 'readonly' of %s.",
                   object_get_typename(o));
        return;
    }

By using memory_region_set_readonly() you bypass this protection.

Maybe not something to worry.

Anyway for the patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Stefan
> 

Re: [PATCH 0/3] nvdimm: read-only file support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Tue, Aug 04, 2020 at 11:12:41AM +0100, Stefan Hajnoczi wrote:
> There is currently no way to back an NVDIMM with a read-only file so it can be
> safely shared between untrusted guests.
> 
> Introduce an -object memory-backend-file,readonly=on|off option.
> 
> Julio Montes sent an earlier patch here:
> https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/
> 
> Eric Ernst requested this feature again for Kata Containers so I gave it a try.
> 
> Stefan Hajnoczi (3):
>   memory: add readonly support to memory_region_init_ram_from_file()
>   hostmem-file: add readonly=on|off option
>   nvdimm: honor -object memory-backend-file,readonly=on option
> 
>  docs/nvdimm.txt           |  8 +++++++-
>  include/exec/memory.h     |  2 ++
>  include/exec/ram_addr.h   |  5 +++--
>  include/qemu/mmap-alloc.h |  2 ++
>  backends/hostmem-file.c   | 26 +++++++++++++++++++++++++-
>  exec.c                    | 18 +++++++++++-------
>  hw/mem/nvdimm.c           |  4 ++++
>  softmmu/memory.c          |  7 +++++--
>  util/mmap-alloc.c         | 10 ++++++----
>  util/oslib-posix.c        |  2 +-
>  qemu-options.hx           |  5 ++++-
>  11 files changed, 70 insertions(+), 19 deletions(-)

Ping

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] hostmem-file: add readonly=on|off option

[Philippe Mathieu-Daudé]

On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> Let -object memory-backend-file work on read-only files when the
> readonly=on option is given. This can be used to share the contents of a
> file between multiple guests while preventing them from consuming
> Copy-on-Write memory if guests dirty the pages, for example.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
>  qemu-options.hx         |  5 ++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 37c70acfe2..6bd5bf9b91 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
>      uint64_t align;
>      bool discard_data;
>      bool is_pmem;
> +    bool readonly;
>  };
>  
>  static void
> @@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>                                       backend->size, fb->align,
>                                       (backend->share ? RAM_SHARED : 0) |
>                                       (fb->is_pmem ? RAM_PMEM : 0),
> -                                     fb->mem_path, false, errp);
> +                                     fb->mem_path, fb->readonly, errp);
>      g_free(name);
>  #endif
>  }
> @@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
>      fb->is_pmem = value;
>  }
>  
> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> +{
> +    return MEMORY_BACKEND_FILE(o)->readonly;
> +}
> +
> +static void file_memory_backend_set_readonly(Object *o, bool value,
> +                                             Error **errp)
> +{
> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> +
> +    if (host_memory_backend_mr_inited(backend)) {
> +        error_setg(errp, "cannot change property 'readonly' of %s.",
> +                   object_get_typename(o));


The 'host_memory_backend_mr_inited()' function is not documented;
my understanding is a backend is considered initialized once it has
a MemoryRegion assigned to it.

So this error message is not very helpful, it doesn't explain the
reason. I see all other setters in this file use the same error,
so it is almost a predating issue.

Still I'd rather use a different message, something like:
"'%s' already initialized, can not set it 'readonly'".

Preferably with the error message reworded:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        return;
> +    }
> +
> +    fb->readonly = value;
> +}
> +
>  static void file_backend_unparent(Object *obj)
>  {
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> @@ -183,6 +204,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
>          NULL, NULL);
>      object_class_property_add_bool(oc, "pmem",
>          file_memory_backend_get_pmem, file_memory_backend_set_pmem);
> +    object_class_property_add_bool(oc, "readonly",
> +        file_memory_backend_get_readonly,
> +        file_memory_backend_set_readonly);
>  }
>  
>  static void file_backend_instance_finalize(Object *o)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 708583b4ce..d834e00b0d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4369,7 +4369,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``
> +    ``-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,readonly=on|off``
>          Creates a memory file backend object, which can be used to back
>          the guest RAM with huge pages.
>  
> @@ -4452,6 +4452,9 @@ SRST
>          4.15) and the filesystem of ``mem-path`` mounted with DAX
>          option.
>  
> +        The ``readonly`` option specifies whether the backing file is opened
> +        read-only or read-write (default).
> +
>      ``-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
> 

Re: [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option

[Philippe Mathieu-Daudé]

On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> Make it possible to present read-only files to the guest as "unarmed"
> NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/nvdimm.txt | 8 +++++++-
>  hw/mem/nvdimm.c | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index c2c6e441b3..c0b52de111 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -17,7 +17,7 @@ following command line options:
>  
>   -machine pc,nvdimm
>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
> - -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
> + -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
>   -device nvdimm,id=nvdimm1,memdev=mem1
>  
>  Where,
> @@ -42,6 +42,12 @@ Where,
>     "share=off", then guest writes won't be applied to the backend
>     file and thus will be invisible to other guests.
>  
> +   "readonly=on/off" controls whether the the file $PATH is opened read-only or

Double "the the".

> +   read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping

NFIT acronym for 'NVDIMM Firmware Interface Table', "NVDIMM" sounds
redundant but makes it easier to understand, so OK.

> +   Structure "NVDIMM State Flags" Bit 3 indicating that the device is "unarmed"
> +   and cannot accept persistent writes. Linux guest drivers set the device to
> +   read-only when this bit is present.
> +
>   - "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
>     device whose storage is provided by above memory backend device.
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index e1574bc07c..694223450e 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
>                               "nvdimm-memory", mr, 0, pmem_size);
>      memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
>      nvdimm->nvdimm_mr->align = align;
> +
> +    if (memory_region_is_rom(mr)) {
> +        nvdimm->unarmed = true; /* this device is read-only */
> +    }

Can you move this hunk before the alias creation?
(Just before nvdimm->nvdimm_mr = ...).

>  }
>  
>  static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,

I don't understand why MemoryDeviceClass::get_memory_region() implicitly
calls DeviceClass::realize()...
Anyway unrelated to this patch.

With the comments addressed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/3] hostmem-file: add readonly=on|off option

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]

On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> > Let -object memory-backend-file work on read-only files when the
> > readonly=on option is given. This can be used to share the contents of a
> > file between multiple guests while preventing them from consuming
> > Copy-on-Write memory if guests dirty the pages, for example.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> >  qemu-options.hx         |  5 ++++-
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 37c70acfe2..6bd5bf9b91 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
> >      uint64_t align;
> >      bool discard_data;
> >      bool is_pmem;
> > +    bool readonly;
> >  };
> >  
> >  static void
> > @@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >                                       backend->size, fb->align,
> >                                       (backend->share ? RAM_SHARED : 0) |
> >                                       (fb->is_pmem ? RAM_PMEM : 0),
> > -                                     fb->mem_path, false, errp);
> > +                                     fb->mem_path, fb->readonly, errp);
> >      g_free(name);
> >  #endif
> >  }
> > @@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> >      fb->is_pmem = value;
> >  }
> >  
> > +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> > +{
> > +    return MEMORY_BACKEND_FILE(o)->readonly;
> > +}
> > +
> > +static void file_memory_backend_set_readonly(Object *o, bool value,
> > +                                             Error **errp)
> > +{
> > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> > +
> > +    if (host_memory_backend_mr_inited(backend)) {
> > +        error_setg(errp, "cannot change property 'readonly' of %s.",
> > +                   object_get_typename(o));
> 
> 
> The 'host_memory_backend_mr_inited()' function is not documented;
> my understanding is a backend is considered initialized once it has
> a MemoryRegion assigned to it.
> 
> So this error message is not very helpful, it doesn't explain the
> reason. I see all other setters in this file use the same error,
> so it is almost a predating issue.
> 
> Still I'd rather use a different message, something like:
> "'%s' already initialized, can not set it 'readonly'".
> 
> Preferably with the error message reworded:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I haven't reworded the error message because it's used in
hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
need to change the error messages across these files.

There is no time when users can actually change these QOM properties, so
"cannot change FOO" is a reasonable wording form the user perspective.
Telling the user that there is a pre-initialization state when the
property can be change isn't useful because they cannot observe that
state (the object is created and ->complete is called in a single step).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

On Fri, Aug 21, 2020 at 03:03:50PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> > Make it possible to present read-only files to the guest as "unarmed"
> > NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  docs/nvdimm.txt | 8 +++++++-
> >  hw/mem/nvdimm.c | 4 ++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> > index c2c6e441b3..c0b52de111 100644
> > --- a/docs/nvdimm.txt
> > +++ b/docs/nvdimm.txt
> > @@ -17,7 +17,7 @@ following command line options:
> >  
> >   -machine pc,nvdimm
> >   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
> > - -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
> > + -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
> >   -device nvdimm,id=nvdimm1,memdev=mem1
> >  
> >  Where,
> > @@ -42,6 +42,12 @@ Where,
> >     "share=off", then guest writes won't be applied to the backend
> >     file and thus will be invisible to other guests.
> >  
> > +   "readonly=on/off" controls whether the the file $PATH is opened read-only or
> 
> Double "the the".

Will fix.

> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index e1574bc07c..694223450e 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
> >                               "nvdimm-memory", mr, 0, pmem_size);
> >      memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
> >      nvdimm->nvdimm_mr->align = align;
> > +
> > +    if (memory_region_is_rom(mr)) {
> > +        nvdimm->unarmed = true; /* this device is read-only */
> > +    }
> 
> Can you move this hunk before the alias creation?
> (Just before nvdimm->nvdimm_mr = ...).

Will fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] hostmem-file: add readonly=on|off option

[Philippe Mathieu-Daudé]

On 9/16/20 11:31 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>>> Let -object memory-backend-file work on read-only files when the
>>> readonly=on option is given. This can be used to share the contents of a
>>> file between multiple guests while preventing them from consuming
>>> Copy-on-Write memory if guests dirty the pages, for example.
>>>
[...]

>>> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
>>> +{
>>> +    return MEMORY_BACKEND_FILE(o)->readonly;
>>> +}
>>> +
>>> +static void file_memory_backend_set_readonly(Object *o, bool value,
>>> +                                             Error **errp)
>>> +{
>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
>>> +
>>> +    if (host_memory_backend_mr_inited(backend)) {
>>> +        error_setg(errp, "cannot change property 'readonly' of %s.",
>>> +                   object_get_typename(o));
>>
>>
>> The 'host_memory_backend_mr_inited()' function is not documented;
>> my understanding is a backend is considered initialized once it has
>> a MemoryRegion assigned to it.
>>
>> So this error message is not very helpful, it doesn't explain the
>> reason. I see all other setters in this file use the same error,
>> so it is almost a predating issue.
>>
>> Still I'd rather use a different message, something like:
>> "'%s' already initialized, can not set it 'readonly'".
>>
>> Preferably with the error message reworded:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> I haven't reworded the error message because it's used in
> hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
> need to change the error messages across these files.
> 
> There is no time when users can actually change these QOM properties, so
> "cannot change FOO" is a reasonable wording form the user perspective.
> Telling the user that there is a pre-initialization state when the
> property can be change isn't useful because they cannot observe that
> state (the object is created and ->complete is called in a single step).

OK, understood.