On Tue, Nov 27, 2018 at 5:07 PM Ilya Maximets <i.maxim...@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.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);
>
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
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
--
Marc-André Lureau
