Hi
On Tue, Nov 24, 2020 at 5:32 PM Marc-André Lureau
<marcandre.lur...@redhat.com> wrote:
>
> Hi
>
> On Tue, Nov 24, 2020 at 3:54 PM Dr. David Alan Gilbert
> <dgilb...@redhat.com> wrote:
> >
> > * marcandre.lur...@redhat.com (marcandre.lur...@redhat.com) wrote:
> > > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> > >
> > > Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
> > > inflight I/O in shared memory") which introduced glib dependency through
> > > osdep.h inclusion.
> > >
> > > libvhost-user.c tries to stay free from glib usage.
> > >
> > > Use glibc memfd_create directly when available (assumed so when
> > > MFD_ALLOW_SEALING is defined).
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > ---
> > > contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
> > > 1 file changed, 43 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/contrib/libvhost-user/libvhost-user.c
> > > b/contrib/libvhost-user/libvhost-user.c
> > > index 1c1cfbf1e7..805521859d 100644
> > > --- a/contrib/libvhost-user/libvhost-user.c
> > > +++ b/contrib/libvhost-user/libvhost-user.c
> > > @@ -42,8 +42,6 @@
> > > #endif
> > >
> > > #include "qemu/atomic.h"
> > > -#include "qemu/osdep.h"
> > > -#include "qemu/memfd.h"
> > >
> > > #include "libvhost-user.h"
> > >
> > > @@ -1615,11 +1613,45 @@ vu_inflight_queue_size(uint16_t queue_size)
> > > sizeof(uint16_t), INFLIGHT_ALIGNMENT);
> > > }
> > >
> > > +#ifdef MFD_ALLOW_SEALING
> > > +static void *
> > > +memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
> > > +{
> > > + void *ptr;
> > > + int ret;
> > > +
> > > + *fd = memfd_create(name, MFD_ALLOW_SEALING);
> > > + if (*fd < 0) {
> > > + return NULL;
> > > + }
> > > +
> > > + ret = ftruncate(*fd, size);
> >
> > Do you need to do any of the page alignment?
>
> We don't do any in util/memfd.c, I don't see an explicit requirement
> in memfd_create(). (however, util/memfd.c did check power of 2 for
> hugetlb usage, but this isn't necessary here)
>
> On mmap(), "if addr is NULL, then the kernel chooses the (page-aligned)
> address"
>
> >
> > > + if (ret < 0) {
> > > + close(*fd);
> > > + return NULL;
> > > + }
> > > +
> > > + ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK |
> > > F_SEAL_SEAL);
> >
> > I think you'd intended to use the 'flags' parameter there.
>
> indeed, thanks
>
> >
> > > + if (ret < 0) {
> > > + close(*fd);
> > > + return NULL;
> > > + }
> > > +
> > > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
> > > + if (ptr == MAP_FAILED) {
> > > + close(*fd);
> > > + return NULL;
> > > + }
> > > +
> > > + return ptr;
> > > +}
> > > +#endif
> > > +
> > > static bool
> > > vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > > {
> > > - int fd;
> > > - void *addr;
> > > + int fd = -1;
> > > + void *addr = NULL;
> > > uint64_t mmap_size;
> > > uint16_t num_queues, queue_size;
> > >
> > > @@ -1637,9 +1669,13 @@ vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > >
> > > mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
> > >
> > > - addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
> > > - F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > - &fd, NULL);
> > > +#ifdef MFD_ALLOW_SEALING
> > > + addr = memfd_alloc("vhost-inflight", mmap_size,
> > > + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > + &fd);
> > > +#else
> > > + vu_panic(dev, "Not implemented: memfd support is missing");
> >
> > Should there be an ifdef somewhere on the declared features, so it
> > doesn't get this far because it wont negotiate the feature?
>
> Sealing grow/shrink came together with memfd, it was one of the main
> selling point. I assume if MFD_ALLOW_SEALING is defined, we have
> memfd_create and basic libc defines. But yes, if we want to handle
> weird corner cases, we should add more ifdef-ry. I'd pass for now.
However, let's avoid compiling code that can panic. I am updating the
series with a standalone meson subproject.
