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

[David Hildenbrand]

On 11.08.23 07:49, ThinerLogoer wrote:
> At 2023-08-11 05:24:43, "Peter Xu" <peterx@redhat.com> wrote:
> > On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:
> > > > I think we have the following options (there might be more)
> > > >
> > > > 1) This patch.
> > > >
> > > > 2) New flag for memory-backend-file. We already have "readonly" and
> > > > "share=". I'm having a hard time coming up with a good name that really
> > > > describes the subtle difference.
> > > >
> > > > 3) Glue behavior to the QEMU machine
> > >
> > > 4) '-deny-private-discard' argv, or environment variable, or both
> >
> > I'd personally vote for (2).  How about "fdperm"?  To describe when we want
> > to use different rw permissions on the file (besides the access permission
> > of the memory we already provided with "readonly"=XXX).  IIUC the only sane
> > value will be ro/rw/default, where "default" should just use the same rw
> > permission as the memory ("readonly"=XXX).
> >
> > Would that be relatively clean and also work in this use case?
> >
> > (the other thing I'd wish we don't have that fallback is, as long as we
> > have any of that "fallback" we'll need to be compatible with it since
> > then, and for ever...)
>
> If it must be (2), I would vote (2) + (4), with (4) adjust the default behavior 
> of said `fdperm`.
> Mainly because (private+discard) is itself not a good practice and (4) serves
> as a good tool to help catch existing (private+discard) problems.

Instead of fdperm, maybe we could find a better name.

The man page of "open" says: The argument flags must include one of the 
following access modes: O_RDONLY, O_WRONLY, or O_RDWR.  These request 
opening the file read-only, write-only, or read/write, respectively.

So maybe something a bit more mouthful like "file-access-mode" would be 
better.

We could have the options
*readwrite
*readonly
*auto

Whereby "auto" is the default.

Specifying:

* "readonly=true,file-access-mode=readwrite" would fail.
* "shared=true,readonly=false,file-access-mode=readonly" would fail.
* "shared=false,readonly=false,file-access-mode=readonly" would work.

In theory, we could adjust the mode of "auto" with compat machines. But 
maybe it would be sufficient to do something like the following

1) shared=true,readonly=false
        -> readwrite
2) shared=true,readonly=true
        > readonly
2) shared=false,readonly=true
        -> readonly
3) shared=false,readonly=true
        hugetlb? -> readwrite
        otherwise -> readonly

Reason being the traditional usage of hugetlb with MAP_PRIVATE where I 
can see possible users with postcopy. Further, VM templating with 
hugetlb might not be that common ... users could still modify the 
behavior if they really have to.

That would make your use case work automatically with file-backed memory.

--
Cheers,

David / dhildenb