Re: [PATCH v2 1/4] softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file mapping

[Xiaoyao Li]

On 10/18/2023 3:42 PM, David Hildenbrand wrote:
> On 18.10.23 05:02, Xiaoyao Li wrote:
> > David,
> >
> > On 7/6/2023 3:56 PM, David Hildenbrand wrote:
> > > ram_block_discard_range() cannot possibly do the right thing in
> > > MAP_PRIVATE file mappings in the general case.
> > >
> > > To achieve the documented semantics, we also have to punch a hole into
> > > the file, possibly messing with other MAP_PRIVATE/MAP_SHARED mappings
> > > of such a file.
> > >
> > > For example, using VM templating -- see commit b17fbbe55cba ("migration:
> > > allow private destination ram with x-ignore-shared") -- in 
> > > combination with
> > > any mechanism that relies on discarding of RAM is problematic. This
> > > includes:
> > > * Postcopy live migration
> > > * virtio-balloon inflation/deflation or free-page-reporting
> > > * virtio-mem
> > >
> > > So at least warn that there is something possibly dangerous is going on
> > > when using ram_block_discard_range() in these cases.
> > >
> > > Acked-by: Peter Xu <peterx@redhat.com>
> > > Tested-by: Mario Casquero <mcasquer@redhat.com>
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > ---
> > >    softmmu/physmem.c | 18 ++++++++++++++++++
> > >    1 file changed, 18 insertions(+)
> > >
> > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > > index bda475a719..4ee157bda4 100644
> > > --- a/softmmu/physmem.c
> > > +++ b/softmmu/physmem.c
> > > @@ -3456,6 +3456,24 @@ int ram_block_discard_range(RAMBlock *rb, 
> > > uint64_t start, size_t length)
> > >                 * so a userfault will trigger.
> > >                 */
> > >    #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> > > +            /*
> > > +             * We'll discard data from the actual file, even though 
> > > we only
> > > +             * have a MAP_PRIVATE mapping, possibly messing with other
> > > +             * MAP_PRIVATE/MAP_SHARED mappings. There is no easy way to
> > > +             * change that behavior whithout violating the promised
> > > +             * semantics of ram_block_discard_range().
> > > +             *
> > > +             * Only warn, because it work as long as nobody else 
> > > uses that
> > > +             * file.
> > > +             */
> > > +            if (!qemu_ram_is_shared(rb)) {
> > > +                warn_report_once("ram_block_discard_range: 
> > > Discarding RAM"
> > > +                                 " in private file mappings is 
> > > possibly"
> > > +                                 " dangerous, because it will modify 
> > > the"
> > > +                                 " underlying file and will affect 
> > > other"
> > > +                                 " users of the file");
> > > +            }
> > > +
> >
> > TDX has two types of memory backend for each RAM, shared memory and
> > private memory. Private memory is serviced by guest memfd and shared
> > memory can also be backed with a fd.
> >
> > At any time, only one type needs to be valid, which means the opposite
> > can be discarded. We do implement the memory discard when TDX converts
> > the memory[1]. It will trigger this warning 100% because by default the
> > guest memfd is not mapped as shared (MAP_SHARED).
>
> If MAP_PRIVATE is not involved and you are taking the pages directly out 
> of the memfd, you should mark that thing as shared. 

Is it the general rule of Linux? Of just the rule of QEMU memory discard?

> Anonymous memory is never involved.

Could you please elaborate more on this? What do you want to express 
here regrading anonymous memory? (Sorry that I'm newbie for mmap stuff)

> "Private memory" is only private from the guest POV, not from a mmap() 
> point of view.
>
> Two different concepts of "private".
>
> > Simply remove the warning will fail the purpose of this patch. The other
> > option is to skip the warning for TDX case, which looks vary hacky. Do
> > you have any idea?
>
> For TDX, all memory backends / RAMBlocks should be marked as "shared", 
> and you should fail if that is not provided by the user.

As I asked above, I want to understand the logic clearly. Is mapped as 
shared is a must to support the memory discard? i.e., if we want to 
support memory discard after memory type change, then the memory must be 
mapped with MAP_SHARED?