Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand <david@redhat.com>]

@Stefan, see below on a R/O NVDIMM question.

We're discussing how to get MAPR_PRIVATE R/W mapping of a
memory-backend-file running when using R/O files.

> 
> This seems a good idea. I am good with the solution you proposed
> here as well.

I was just going to get started working on that, when I realized
something important:


"@readonly: if true, the backing file is opened read-only; if false,
  it is opened read-write.  (default: false)"

"@share: if false, the memory is private to QEMU; if true, it is
  shared (default: false)"

So readonly is *all* about the file access mode already ... the mmap()
parameters are just a side-effect of that. Adding a new
"file-access-mode" or similar would be wrong.


Here are the combinations we have right now:

-object memory-backend-file,share=on,readonly=on

  -> Existing behavior: Open readonly, mmap readonly shared
  -> Makes sense, mmap'ing readwrite would fail

-object memory-backend-file,share=on,readonly=off

  -> Existing behavior: Open readwrite, mmap readwrite shared
  -> Mostly makes sense: why open a shared file R/W and not mmap it
     R/W?

-object memory-backend-file,share=off,readonly=off
  -> Existing behavior: Open readwrite, mmap readwrite private
  -> Mostly makes sense: why open a file R/W and not map it R/W (even if
     private)?

-object memory-backend-file,share=off,readonly=on
  -> Existing behavior: Open readonly, mmap readonly private
  -> That's the problematic one


So for your use case (VM templating using a readonly file), you
would actually want to use:

-object memory-backend-file,share=off,readonly=on

BUT, have the mmap be writable (instead of currently readonly).

Assuming we would change the current behavior, what if someone would
specify:

-object memory-backend-file,readonly=on

(because the default is share=off ...) and using it for a R/O NVDIMM,
where we expect any write access to fail.


But let's look at the commit that added the "readonly" parameter:

commit 86635aa4e9d627d5142b81c57a33dd1f36627d07
Author: Stefan Hajnoczi <stefanha@redhat.com>
Date:   Mon Jan 4 17:13:19 2021 +0000

     hostmem-file: add readonly=on|off option
     
     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.

That was part of

https://lore.kernel.org/all/20210104171320.575838-3-stefanha@redhat.com/T/#m712f995e6dcfdde433958bae5095b145dd0ee640

 From what I understand, for NVDIMMs we always use
"-object memory-backend-file,share=on", even when we want a
readonly NVDIMM.


So we have two options:

1) Change the current behavior of -object memory-backend-file,share=off,readonly=on:

-> Open the file r/o but mmap it writable

2) Add a new property to configure the mmap accessibility. Not a big fan of that.


@Stefan, do you have any concern when we would do 1) ?

As far as I can tell, we have to set the nvdimm to "unarmed=on" either way:

+   "unarmed" controls 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. Set unarmed to on when the memdev has readonly=on.

So changing the behavior would not really break the nvdimm use case.

Further, we could warn in nvdimm code when we stumble over this configuration with
unarmed=on.

-- 
Cheers,

David / dhildenb