Re: [Qemu-devel] [PATCH 1/4] hostmem-memfd: enable seals only if supported

[Ilya Maximets]

On 27.11.2018 15:56, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Nov 27, 2018 at 4:37 PM Ilya Maximets <address@hidden> wrote:
>>
>> On 27.11.2018 15:29, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Nov 27, 2018 at 4:02 PM Ilya Maximets <address@hidden> wrote:
>>>>
>>>> On 27.11.2018 15:00, Marc-André Lureau wrote:
>>>>> Hi
>>>>> On Tue, Nov 27, 2018 at 3:56 PM Ilya Maximets <address@hidden> wrote:
>>>>>>
>>>>>> On 27.11.2018 14:49, Marc-André Lureau wrote:
>>>>>>> Hi
>>>>>>> On Tue, Nov 27, 2018 at 3:11 PM Ilya Maximets <address@hidden> 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 <address@hidden>
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>