| Message ID | 20181127111050.18453-2-i.maximets@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memfd fixes. | expand |
Hi On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > If seals are not supported, memfd_create() will fail. > Furthermore, there is no way to disable it in this case because > '.seal' property is not registered. > > This issue leads to vhost-user-test failures on RHEL 7.2: > > qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > failed to create memfd: Invalid argument > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> This will change the default behaviour of memfd backend, and may thus me considered a break. Instead, memfd vhost-user-test should skipped (or tuned with sealed=false if unsupported) > --- > backends/hostmem-memfd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > index b6836b28e5..ee39bdbde6 100644 > --- a/backends/hostmem-memfd.c > +++ b/backends/hostmem-memfd.c > @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) > { > HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); > > - /* default to sealed file */ > - m->seal = true; > + /* default to sealed file if supported */ > + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); > } > > static void > -- > 2.17.1 >
On 27.11.2018 14:49, Marc-André Lureau wrote: > Hi > On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: >> >> If seals are not supported, memfd_create() will fail. >> Furthermore, there is no way to disable it in this case because >> '.seal' property is not registered. >> >> This issue leads to vhost-user-test failures on RHEL 7.2: >> >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >> failed to create memfd: Invalid argument >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > This will change the default behaviour of memfd backend, and may thus > me considered a break. This will change the default behaviour only on systems without sealing support. But current implementation is broken there anyway and does not work. > > Instead, memfd vhost-user-test should skipped (or tuned with > sealed=false if unsupported) vhost-user-test is just an example here. In fact memfd could not be used at all on system without sealing support. And there is no way to disable seals. > >> --- >> backends/hostmem-memfd.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c >> index b6836b28e5..ee39bdbde6 100644 >> --- a/backends/hostmem-memfd.c >> +++ b/backends/hostmem-memfd.c >> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) >> { >> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); >> >> - /* default to sealed file */ >> - m->seal = true; >> + /* default to sealed file if supported */ >> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); >> } >> >> static void >> -- >> 2.17.1 >> > >
Hi On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > On 27.11.2018 14:49, Marc-André Lureau wrote: > > Hi > > On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >> > >> If seals are not supported, memfd_create() will fail. > >> Furthermore, there is no way to disable it in this case because > >> '.seal' property is not registered. > >> > >> This issue leads to vhost-user-test failures on RHEL 7.2: > >> > >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > >> failed to create memfd: Invalid argument > >> > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > > > > This will change the default behaviour of memfd backend, and may thus > > me considered a break. > > This will change the default behaviour only on systems without sealing > support. But current implementation is broken there anyway and does not > work. > > > > > Instead, memfd vhost-user-test should skipped (or tuned with > > sealed=false if unsupported) > > vhost-user-test is just an example here. In fact memfd could not be > used at all on system without sealing support. And there is no way > to disable seals. which system supports memfd without sealing? > > > > >> --- > >> backends/hostmem-memfd.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > >> index b6836b28e5..ee39bdbde6 100644 > >> --- a/backends/hostmem-memfd.c > >> +++ b/backends/hostmem-memfd.c > >> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) > >> { > >> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); > >> > >> - /* default to sealed file */ > >> - m->seal = true; > >> + /* default to sealed file if supported */ > >> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); > >> } > >> > >> static void > >> -- > >> 2.17.1 > >> > > > > >
On 27.11.2018 15:00, Marc-André Lureau wrote: > Hi > On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: >> >> On 27.11.2018 14:49, Marc-André Lureau wrote: >>> Hi >>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>> >>>> If seals are not supported, memfd_create() will fail. >>>> Furthermore, there is no way to disable it in this case because >>>> '.seal' property is not registered. >>>> >>>> This issue leads to vhost-user-test failures on RHEL 7.2: >>>> >>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >>>> failed to create memfd: Invalid argument >>>> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> >>> >>> This will change the default behaviour of memfd backend, and may thus >>> me considered a break. >> >> This will change the default behaviour only on systems without sealing >> support. But current implementation is broken there anyway and does not >> work. >> >>> >>> Instead, memfd vhost-user-test should skipped (or tuned with >>> sealed=false if unsupported) >> >> vhost-user-test is just an example here. In fact memfd could not be >> used at all on system without sealing support. And there is no way >> to disable seals. > > which system supports memfd without sealing? RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 > >> >>> >>>> --- >>>> backends/hostmem-memfd.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c >>>> index b6836b28e5..ee39bdbde6 100644 >>>> --- a/backends/hostmem-memfd.c >>>> +++ b/backends/hostmem-memfd.c >>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) >>>> { >>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); >>>> >>>> - /* default to sealed file */ >>>> - m->seal = true; >>>> + /* default to sealed file if supported */ >>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); >>>> } >>>> >>>> static void >>>> -- >>>> 2.17.1 >>>> >>> >>> >> > >
Hi On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > On 27.11.2018 15:00, Marc-André Lureau wrote: > > Hi > > On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >> > >> On 27.11.2018 14:49, Marc-André Lureau wrote: > >>> Hi > >>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>> > >>>> If seals are not supported, memfd_create() will fail. > >>>> Furthermore, there is no way to disable it in this case because > >>>> '.seal' property is not registered. > >>>> > >>>> This issue leads to vhost-user-test failures on RHEL 7.2: > >>>> > >>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > >>>> failed to create memfd: Invalid argument > >>>> > >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>> > >>> > >>> This will change the default behaviour of memfd backend, and may thus > >>> me considered a break. > >> > >> This will change the default behaviour only on systems without sealing > >> support. But current implementation is broken there anyway and does not > >> work. > >> > >>> > >>> Instead, memfd vhost-user-test should skipped (or tuned with > >>> sealed=false if unsupported) > >> > >> vhost-user-test is just an example here. In fact memfd could not be > >> used at all on system without sealing support. And there is no way > >> to disable seals. > > > > which system supports memfd without sealing? > > RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 Correct, it was backported without sealing for some reason. I would rather have an explicit seal=off argument on such system (because sealing is expected to be available with memfd in general) > > > > >> > >>> > >>>> --- > >>>> backends/hostmem-memfd.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > >>>> index b6836b28e5..ee39bdbde6 100644 > >>>> --- a/backends/hostmem-memfd.c > >>>> +++ b/backends/hostmem-memfd.c > >>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) > >>>> { > >>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); > >>>> > >>>> - /* default to sealed file */ > >>>> - m->seal = true; > >>>> + /* default to sealed file if supported */ > >>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); > >>>> } > >>>> > >>>> static void > >>>> -- > >>>> 2.17.1 > >>>> > >>> > >>> > >> > > > >
On 27.11.2018 15:29, Marc-André Lureau wrote: > Hi > > On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote: >> >> On 27.11.2018 15:00, Marc-André Lureau wrote: >>> Hi >>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>> >>>> On 27.11.2018 14:49, Marc-André Lureau wrote: >>>>> Hi >>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>>>> >>>>>> If seals are not supported, memfd_create() will fail. >>>>>> Furthermore, there is no way to disable it in this case because >>>>>> '.seal' property is not registered. >>>>>> >>>>>> This issue leads to vhost-user-test failures on RHEL 7.2: >>>>>> >>>>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >>>>>> failed to create memfd: Invalid argument >>>>>> >>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>>> >>>>> >>>>> This will change the default behaviour of memfd backend, and may thus >>>>> me considered a break. >>>> >>>> This will change the default behaviour only on systems without sealing >>>> support. But current implementation is broken there anyway and does not >>>> work. >>>> >>>>> >>>>> Instead, memfd vhost-user-test should skipped (or tuned with >>>>> sealed=false if unsupported) >>>> >>>> vhost-user-test is just an example here. In fact memfd could not be >>>> used at all on system without sealing support. And there is no way >>>> to disable seals. >>> >>> which system supports memfd without sealing? >> >> RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 > > Correct, it was backported without sealing for some reason. > > I would rather have an explicit seal=off argument on such system > (because sealing is expected to be available with memfd in general) > But '.seal' property registering is guarded by 'qemu_memfd_check(MFD_ALLOW_SEALING)'. And you can not disable it: qemu-system-x86_64: -object memory-backend-memfd,seal=off,id=mem,size=2M,: Property '.seal' not found Enabling this option on system that does not support sealing will probably break some libvirt feature discovering or something similar. What about adding some warning to 'memfd_backend_instance_init' if sealing not supported before disabling it ? >> >>> >>>> >>>>> >>>>>> --- >>>>>> backends/hostmem-memfd.c | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c >>>>>> index b6836b28e5..ee39bdbde6 100644 >>>>>> --- a/backends/hostmem-memfd.c >>>>>> +++ b/backends/hostmem-memfd.c >>>>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) >>>>>> { >>>>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); >>>>>> >>>>>> - /* default to sealed file */ >>>>>> - m->seal = true; >>>>>> + /* default to sealed file if supported */ >>>>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); >>>>>> } >>>>>> >>>>>> static void >>>>>> -- >>>>>> 2.17.1 >>>>>> >>>>> >>>>> >>>> >>> >>> > > >
Hi, > > > which system supports memfd without sealing? > > > > RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 > > Correct, it was backported without sealing for some reason. > > I would rather have an explicit seal=off argument on such system > (because sealing is expected to be available with memfd in general) Or just drop support for memfd without sealing. cheers, Gerd
Hi On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > On 27.11.2018 15:29, Marc-André Lureau wrote: > > Hi > > > > On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >> > >> On 27.11.2018 15:00, Marc-André Lureau wrote: > >>> Hi > >>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>> > >>>> On 27.11.2018 14:49, Marc-André Lureau wrote: > >>>>> Hi > >>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>>>> > >>>>>> If seals are not supported, memfd_create() will fail. > >>>>>> Furthermore, there is no way to disable it in this case because > >>>>>> '.seal' property is not registered. > >>>>>> > >>>>>> This issue leads to vhost-user-test failures on RHEL 7.2: > >>>>>> > >>>>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > >>>>>> failed to create memfd: Invalid argument > >>>>>> > >>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>>>> > >>>>> > >>>>> This will change the default behaviour of memfd backend, and may thus > >>>>> me considered a break. > >>>> > >>>> This will change the default behaviour only on systems without sealing > >>>> support. But current implementation is broken there anyway and does not > >>>> work. > >>>> > >>>>> > >>>>> Instead, memfd vhost-user-test should skipped (or tuned with > >>>>> sealed=false if unsupported) > >>>> > >>>> vhost-user-test is just an example here. In fact memfd could not be > >>>> used at all on system without sealing support. And there is no way > >>>> to disable seals. > >>> > >>> which system supports memfd without sealing? > >> > >> RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 > > > > Correct, it was backported without sealing for some reason. > > > > I would rather have an explicit seal=off argument on such system > > (because sealing is expected to be available with memfd in general) > > > > But '.seal' property registering is guarded by 'qemu_memfd_check(MFD_ALLOW_SEALING)'. > And you can not disable it: > > qemu-system-x86_64: -object memory-backend-memfd,seal=off,id=mem,size=2M,: Property '.seal' not found Right > > Enabling this option on system that does not support sealing will > probably break some libvirt feature discovering or something similar. > > What about adding some warning to 'memfd_backend_instance_init' if > sealing not supported before disabling it ? What do you think of Gerd suggestion, and disable memfd backend if sealing is not available? (the sealing property check can be removed then). > > >> > >>> > >>>> > >>>>> > >>>>>> --- > >>>>>> backends/hostmem-memfd.c | 4 ++-- > >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > >>>>>> index b6836b28e5..ee39bdbde6 100644 > >>>>>> --- a/backends/hostmem-memfd.c > >>>>>> +++ b/backends/hostmem-memfd.c > >>>>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) > >>>>>> { > >>>>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); > >>>>>> > >>>>>> - /* default to sealed file */ > >>>>>> - m->seal = true; > >>>>>> + /* default to sealed file if supported */ > >>>>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); > >>>>>> } > >>>>>> > >>>>>> static void > >>>>>> -- > >>>>>> 2.17.1 > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > > > > > >
On 27.11.2018 15:56, Marc-André Lureau wrote: > Hi > > On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote: >> >> On 27.11.2018 15:29, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>> >>>> On 27.11.2018 15:00, Marc-André Lureau wrote: >>>>> Hi >>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>>>> >>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote: >>>>>>> Hi >>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: >>>>>>>> >>>>>>>> If seals are not supported, memfd_create() will fail. >>>>>>>> Furthermore, there is no way to disable it in this case because >>>>>>>> '.seal' property is not registered. >>>>>>>> >>>>>>>> This issue leads to vhost-user-test failures on RHEL 7.2: >>>>>>>> >>>>>>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >>>>>>>> failed to create memfd: Invalid argument >>>>>>>> >>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>>>>> >>>>>>> >>>>>>> This will change the default behaviour of memfd backend, and may thus >>>>>>> me considered a break. >>>>>> >>>>>> This will change the default behaviour only on systems without sealing >>>>>> support. But current implementation is broken there anyway and does not >>>>>> work. >>>>>> >>>>>>> >>>>>>> Instead, memfd vhost-user-test should skipped (or tuned with >>>>>>> sealed=false if unsupported) >>>>>> >>>>>> vhost-user-test is just an example here. In fact memfd could not be >>>>>> used at all on system without sealing support. And there is no way >>>>>> to disable seals. >>>>> >>>>> which system supports memfd without sealing? >>>> >>>> RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 >>> >>> Correct, it was backported without sealing for some reason. >>> >>> I would rather have an explicit seal=off argument on such system >>> (because sealing is expected to be available with memfd in general) >>> >> >> But '.seal' property registering is guarded by 'qemu_memfd_check(MFD_ALLOW_SEALING)'. >> And you can not disable it: >> >> qemu-system-x86_64: -object memory-backend-memfd,seal=off,id=mem,size=2M,: Property '.seal' not found > > Right > >> >> Enabling this option on system that does not support sealing will >> probably break some libvirt feature discovering or something similar. >> >> What about adding some warning to 'memfd_backend_instance_init' if >> sealing not supported before disabling it ? > > What do you think of Gerd suggestion, and disable memfd backend if > sealing is not available? (the sealing property check can be removed > then). It's OK in general for me. What about something like this: --- diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index ee39bdbde6..a3455da9c9 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) "Huge pages size (ex: 2M, 1G)", &error_abort); } - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { - object_class_property_add_bool(oc, "seal", - memfd_backend_get_seal, - memfd_backend_set_seal, - &error_abort); - object_class_property_set_description(oc, "seal", - "Seal growing & shrinking", - &error_abort); - } + object_class_property_add_bool(oc, "seal", + memfd_backend_get_seal, + memfd_backend_set_seal, + &error_abort); + object_class_property_set_description(oc, "seal", + "Seal growing & shrinking", + &error_abort); } static const TypeInfo memfd_backend_info = { @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { static void register_types(void) { - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { type_register_static(&memfd_backend_info); } } diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 45d58d8ea2..e3e9a33580 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, int mem, enum test_memfd memfd, const char *mem_path, const char *chr_opts, const char *extra) { - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { memfd = TEST_MEMFD_YES; } @@ -903,7 +903,7 @@ static void test_multiqueue(void) s->queues = 2; test_server_listen(s); - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { cmd = g_strdup_printf( QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", @@ -963,7 +963,7 @@ int main(int argc, char **argv) /* run the main loop thread so the chardev may operate */ thread = g_thread_new(NULL, thread_function, loop); - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { qtest_add_data_func("/vhost-user/read-guest-mem/memfd", GINT_TO_POINTER(TEST_MEMFD_YES), test_read_guest_mem); --- ? > >> >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> --- >>>>>>>> backends/hostmem-memfd.c | 4 ++-- >>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c >>>>>>>> index b6836b28e5..ee39bdbde6 100644 >>>>>>>> --- a/backends/hostmem-memfd.c >>>>>>>> +++ b/backends/hostmem-memfd.c >>>>>>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) >>>>>>>> { >>>>>>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); >>>>>>>> >>>>>>>> - /* default to sealed file */ >>>>>>>> - m->seal = true; >>>>>>>> + /* default to sealed file if supported */ >>>>>>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); >>>>>>>> } >>>>>>>> >>>>>>>> static void >>>>>>>> -- >>>>>>>> 2.17.1 >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>> >>> >>> > > >
On Tue, Nov 27, 2018 at 5:07 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > On 27.11.2018 15:56, Marc-André Lureau wrote: > > Hi > > > > On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >> > >> On 27.11.2018 15:29, Marc-André Lureau wrote: > >>> Hi > >>> > >>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>> > >>>> On 27.11.2018 15:00, Marc-André Lureau wrote: > >>>>> Hi > >>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>>>> > >>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote: > >>>>>>> Hi > >>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <i.maximets@samsung.com> wrote: > >>>>>>>> > >>>>>>>> If seals are not supported, memfd_create() will fail. > >>>>>>>> Furthermore, there is no way to disable it in this case because > >>>>>>>> '.seal' property is not registered. > >>>>>>>> > >>>>>>>> This issue leads to vhost-user-test failures on RHEL 7.2: > >>>>>>>> > >>>>>>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > >>>>>>>> failed to create memfd: Invalid argument > >>>>>>>> > >>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>>>>>> > >>>>>>> > >>>>>>> This will change the default behaviour of memfd backend, and may thus > >>>>>>> me considered a break. > >>>>>> > >>>>>> This will change the default behaviour only on systems without sealing > >>>>>> support. But current implementation is broken there anyway and does not > >>>>>> work. > >>>>>> > >>>>>>> > >>>>>>> Instead, memfd vhost-user-test should skipped (or tuned with > >>>>>>> sealed=false if unsupported) > >>>>>> > >>>>>> vhost-user-test is just an example here. In fact memfd could not be > >>>>>> used at all on system without sealing support. And there is no way > >>>>>> to disable seals. > >>>>> > >>>>> which system supports memfd without sealing? > >>>> > >>>> RHEL 7.2. kernel version 3.10.0-327.el7.x86_64 > >>> > >>> Correct, it was backported without sealing for some reason. > >>> > >>> I would rather have an explicit seal=off argument on such system > >>> (because sealing is expected to be available with memfd in general) > >>> > >> > >> But '.seal' property registering is guarded by 'qemu_memfd_check(MFD_ALLOW_SEALING)'. > >> And you can not disable it: > >> > >> qemu-system-x86_64: -object memory-backend-memfd,seal=off,id=mem,size=2M,: Property '.seal' not found > > > > Right > > > >> > >> Enabling this option on system that does not support sealing will > >> probably break some libvirt feature discovering or something similar. > >> > >> What about adding some warning to 'memfd_backend_instance_init' if > >> sealing not supported before disabling it ? > > > > What do you think of Gerd suggestion, and disable memfd backend if > > sealing is not available? (the sealing property check can be removed > > then). > > It's OK in general for me. > What about something like this: > > --- > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > index ee39bdbde6..a3455da9c9 100644 > --- a/backends/hostmem-memfd.c > +++ b/backends/hostmem-memfd.c > @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) > "Huge pages size (ex: 2M, 1G)", > &error_abort); > } > - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > - object_class_property_add_bool(oc, "seal", > - memfd_backend_get_seal, > - memfd_backend_set_seal, > - &error_abort); > - object_class_property_set_description(oc, "seal", > - "Seal growing & shrinking", > - &error_abort); > - } > + object_class_property_add_bool(oc, "seal", > + memfd_backend_get_seal, > + memfd_backend_set_seal, > + &error_abort); > + object_class_property_set_description(oc, "seal", > + "Seal growing & shrinking", > + &error_abort); > } > > static const TypeInfo memfd_backend_info = { > @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { > > static void register_types(void) > { > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > type_register_static(&memfd_backend_info); > } > } > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > index 45d58d8ea2..e3e9a33580 100644 > --- a/tests/vhost-user-test.c > +++ b/tests/vhost-user-test.c > @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, > int mem, enum test_memfd memfd, const char *mem_path, > const char *chr_opts, const char *extra) > { > - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { > + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { > memfd = TEST_MEMFD_YES; > } > > @@ -903,7 +903,7 @@ static void test_multiqueue(void) > s->queues = 2; > test_server_listen(s); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > cmd = g_strdup_printf( > QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " > "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", > @@ -963,7 +963,7 @@ int main(int argc, char **argv) > /* run the main loop thread so the chardev may operate */ > thread = g_thread_new(NULL, thread_function, loop); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > qtest_add_data_func("/vhost-user/read-guest-mem/memfd", > GINT_TO_POINTER(TEST_MEMFD_YES), > test_read_guest_mem); > looks fine, waiting for your v2 series > --- > > ? > > > > >> > >>>> > >>>>> > >>>>>> > >>>>>>> > >>>>>>>> --- > >>>>>>>> backends/hostmem-memfd.c | 4 ++-- > >>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > >>>>>>>> index b6836b28e5..ee39bdbde6 100644 > >>>>>>>> --- a/backends/hostmem-memfd.c > >>>>>>>> +++ b/backends/hostmem-memfd.c > >>>>>>>> @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) > >>>>>>>> { > >>>>>>>> HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); > >>>>>>>> > >>>>>>>> - /* default to sealed file */ > >>>>>>>> - m->seal = true; > >>>>>>>> + /* default to sealed file if supported */ > >>>>>>>> + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); > >>>>>>>> } > >>>>>>>> > >>>>>>>> static void > >>>>>>>> -- > >>>>>>>> 2.17.1 > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index b6836b28e5..ee39bdbde6 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -129,8 +129,8 @@ memfd_backend_instance_init(Object *obj) { HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(obj); - /* default to sealed file */ - m->seal = true; + /* default to sealed file if supported */ + m->seal = qemu_memfd_check(MFD_ALLOW_SEALING); } static void
If seals are not supported, memfd_create() will fail. Furthermore, there is no way to disable it in this case because '.seal' property is not registered. This issue leads to vhost-user-test failures on RHEL 7.2: qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ failed to create memfd: Invalid argument Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- backends/hostmem-memfd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)