Hi
On Thu, Aug 30, 2018 at 10:20 AM Igor Mammedov <imamm...@redhat.com> wrote:
>
> On Wed, 29 Aug 2018 14:36:00 +0200
> Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
>
> > Hi
> >
> > On Wed, Aug 29, 2018 at 10:50 AM Igor Mammedov <imamm...@redhat.com> wrote:
> > >
> > > On Tue, 28 Aug 2018 18:55:35 +0200
> > > Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
> > >
> > > > On Tue, Aug 28, 2018 at 6:54 PM Marc-André Lureau
> > > > <marcandre.lur...@gmail.com> wrote:
> > > > >
> > > > > Hi
> > > > >
> > > > > On Tue, Aug 28, 2018 at 6:49 PM Igor Mammedov <imamm...@redhat.com>
> > > > > wrote:
> > > > > >
> > > > > > On Tue, 28 Aug 2018 17:38:40 +0200
> > > > > > Marc-André Lureau <marcandre.lur...@redhat.com> wrote:
> > > > > >
> > > > > > > The share=on/off property is used to modified mmap() MAP_SHARED
> > > > > > > setting. Make it on by default for convenience and compatibility
> > > > > > > reasons.
> > > > > > it would be nice to state reasons here.
> > > > > >
> > > > > > also flipping default to on is behavioral change when
> > > > > > used with bind policy which could make backend initialization fail
> > > > > > so CLI that used to work (if there are any users of this without
> > > > > > shared=on)
> > > > > > won't work anymore.
> > > > >
> > > > > memfd backend is passing shared=true to
> > > > > memory_region_init_ram_from_fd().
> > > > >
> > > > > The patch allows to tweak that (instead of ignoring the property),
> > > > > while keeping the value to shared=true.
> > > >
> > > > keeping the default value to shared=trrue.
> > > ok,
> > > another question:
> > > does it make sense to allow "shared=false" at all?
> > > should we bail out if user asks for it?
> >
> > You may want to use memfd for convenience (no need to specify hugetlb
> > mem-path for ex), without the share memory aspects.
> ok, acked.
>
> btw:
> looking at memfd_backend_memory_alloc/file_backend_memory_alloc
> both needlessly are are calling host_memory_backend_mr_inited()
> which creates an illusion that alloc could be called multiple times
> but it isn't, could you clean that up?
ok
>
> In addition a following error path needs fixing
> memfd_backend_memory_alloc()
> if (fd == -1) {
> return;
> }
> it must set errp
The error is already set by qemu_memfd_create()
>
> >
> > (but the patch was mostly for completeness)
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > > > > > ---
> > > > > > > backends/hostmem-memfd.c | 4 +++-
> > > > > > > qemu-options.hx | 4 +++-
> > > > > > > 2 files changed, 6 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> > > > > > > index 1e20fe0ba8..abd52364db 100644
> > > > > > > --- a/backends/hostmem-memfd.c
> > > > > > > +++ b/backends/hostmem-memfd.c
> > > > > > > @@ -59,7 +59,8 @@ memfd_backend_memory_alloc(HostMemoryBackend
> > > > > > > *backend, Error **errp)
> > > > > > >
> > > > > > > name = object_get_canonical_path(OBJECT(backend));
> > > > > > > memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
> > > > > > > - name, backend->size, true,
> > > > > > > fd, errp);
> > > > > > > + name, backend->size,
> > > > > > > + backend->share, fd, errp);
> > > > > > > g_free(name);
> > > > > > > }
> > > > > > >
> > > > > > > @@ -131,6 +132,7 @@ memfd_backend_instance_init(Object *obj)
> > > > > > >
> > > > > > > /* default to sealed file */
> > > > > > > m->seal = true;
> > > > > > > + MEMORY_BACKEND(m)->share = true;
> > > > > > > }
> > > > > > >
> > > > > > > static void
> > > > > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > > > > index f14e423b7c..25c02d72b8 100644
> > > > > > > --- a/qemu-options.hx
> > > > > > > +++ b/qemu-options.hx
> > > > > > > @@ -4029,7 +4029,7 @@ Memory backend objects offer more control
> > > > > > > than the @option{-m} option that is
> > > > > > > traditionally used to define guest RAM. Please refer to
> > > > > > > @option{memory-backend-file} for a description of the options.
> > > > > > >
> > > > > > > -@item -object
> > > > > > > memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
> > > > > > > +@item -object
> > > > > > > memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},share=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
> > > > > > >
> > > > > > > Creates an anonymous memory file backend object, which allows
> > > > > > > QEMU to
> > > > > > > share the memory with an external process (e.g. when using
> > > > > > > @@ -4051,6 +4051,8 @@ with the @option{seal} option (requires at
> > > > > > > least Linux 4.16).
> > > > > > > Please refer to @option{memory-backend-file} for a description
> > > > > > > of the
> > > > > > > other options.
> > > > > > >
> > > > > > > +The @option{share} boolean option is @var{on} by default with
> > > > > > > memfd.
> > > > > > > +
> > > > > > > @item -object rng-random,id=@var{id},filename=@var{/dev/random}
> > > > > > >
> > > > > > > Creates a random number generator backend which obtains entropy
> > > > > > > from
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Marc-André Lureau
> > > >
> > > >
> > > >
> > >
> >
> >
>
thanks
--
Marc-André Lureau
