On 27.11.2018 15:56, Marc-André Lureau wrote: > Hi > > On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <i.maxim...@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.maxim...@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.maxim...@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.maxim...@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.maxim...@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 >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>> >>> >>> > > >