| Message ID | 20200804101244.1283503-1-stefanha@redhat.com |
|---|---|
| Headers | show |
| Series | nvdimm: read-only file support | expand |
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).
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.
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.