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

[David Hildenbrand]

On 10.08.23 23:24, Peter Xu 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).

Hmm, I'm not particularly happy about that.

> Would that be relatively clean and also work in this use case?

I get the feeling that we are over-engineering something that probably 
should never have been allowed: MAP_PRIVATE mapping of a file and 
opening it rw because someone might punch holes into it.

Once we start adding new parameters just for that, I get a bit skeptical 
that this is what we want. The number of people that care about that are 
probably close to 0.

The only real use case where this used to make sense (by accident I 
assume) was with hugetlb. And somehow, we decided that it was a good 
idea for "-mem-path" to use MAP_PRIVATE.

So, what stops us from

a) Leaving -mem-path alone. Keep opening files rw.
b) Make memory-backend-file with shared=off,readonly=off open the file
   read-only
c) Gluing that behavior to a QEMU compat machine

fallocate(PUNCH_HOLE) will fail, and we can probably let 
virtio-mem/virtio-balloon and postcopy refuse to even start (virtio-mem 
already does that) as early as possible.

People that care about any such use case would already get a warning 
when punching a hole today.

If we ever support discarding RAM in that configuration, we can simply 
unlock it again.

Am I missing any important use case?

--
Cheers,

David / dhildenb