* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-11-27 13:50 ` [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support Ilya Maximets
@ 2018-11-27 13:56 ` Marc-André Lureau
2018-12-10 16:18 ` Igor Mammedov
2018-12-11 10:53 ` Daniel P. Berrangé
2 siblings, 0 replies; 25+ messages in thread
From: Marc-André Lureau @ 2018-11-27 13:56 UTC (permalink / raw)
To: Maximets, Ilya
Cc: qemu-devel, Bonzini, Paolo, Eduardo Habkost, Igor Mammedov,
Hoffmann, Gerd
Hi
On Tue, Nov 27, 2018 at 5:50 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
>
> and actually breaks the feature on such systems.
>
> Let's restrict memfd backend to systems with sealing support.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
thanks
> ---
> backends/hostmem-memfd.c | 18 ++++++++----------
> tests/vhost-user-test.c | 6 +++---
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index b6836b28e5..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);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-11-27 13:50 ` [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support Ilya Maximets
2018-11-27 13:56 ` Marc-André Lureau
@ 2018-12-10 16:18 ` Igor Mammedov
2018-12-11 10:29 ` Ilya Maximets
2018-12-11 10:53 ` Daniel P. Berrangé
2 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2018-12-10 16:18 UTC (permalink / raw)
To: Ilya Maximets
Cc: Marc-André Lureau, Eduardo Habkost, qemu-devel,
Gerd Hoffmann, Paolo Bonzini
On Tue, 27 Nov 2018 16:50:27 +0300
Ilya Maximets <i.maximets@samsung.com> wrote:
s/wihtout/without/ in subj
> 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
>
> and actually breaks the feature on such systems.
>
> Let's restrict memfd backend to systems with sealing support.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> backends/hostmem-memfd.c | 18 ++++++++----------
> tests/vhost-user-test.c | 6 +++---
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index b6836b28e5..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);
that would either lead to not clear error that type doesn't exist.
it could be better to report sensible error from memfd_backend_memory_alloc() if
the feature is required but not supported by host
> }
> }
> 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);
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-10 16:18 ` Igor Mammedov
@ 2018-12-11 10:29 ` Ilya Maximets
2018-12-11 15:48 ` Igor Mammedov
0 siblings, 1 reply; 25+ messages in thread
From: Ilya Maximets @ 2018-12-11 10:29 UTC (permalink / raw)
To: Igor Mammedov
Cc: Marc-André Lureau, Eduardo Habkost, qemu-devel,
Gerd Hoffmann, Paolo Bonzini
On 10.12.2018 19:18, Igor Mammedov wrote:
> On Tue, 27 Nov 2018 16:50:27 +0300
> Ilya Maximets <i.maximets@samsung.com> wrote:
>
> s/wihtout/without/ in subj
>
>> 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
>>
>> and actually breaks the feature on such systems.
>>
>> Let's restrict memfd backend to systems with sealing support.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> backends/hostmem-memfd.c | 18 ++++++++----------
>> tests/vhost-user-test.c | 6 +++---
>> 2 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index b6836b28e5..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);
> that would either lead to not clear error that type doesn't exist.
> it could be better to report sensible error from memfd_backend_memory_alloc() if
> the feature is required but not supported by host
I'm not sure, but this could break the libvirt capability discovering.
Current patch changes behaviour probably only for RHEL/CentOS 7.2.
All other systems are not affected. Do you think that we need to
change behaviour on all the systems?
>
>> }
>> }
>> 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);
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-11 10:29 ` Ilya Maximets
@ 2018-12-11 15:48 ` Igor Mammedov
2019-01-05 2:43 ` Eduardo Habkost
0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2018-12-11 15:48 UTC (permalink / raw)
To: Ilya Maximets
Cc: Marc-André Lureau, Eduardo Habkost, qemu-devel,
Gerd Hoffmann, Paolo Bonzini
On Tue, 11 Dec 2018 13:29:19 +0300
Ilya Maximets <i.maximets@samsung.com> wrote:
CCing libvirt folk for an opinion
> On 10.12.2018 19:18, Igor Mammedov wrote:
> > On Tue, 27 Nov 2018 16:50:27 +0300
> > Ilya Maximets <i.maximets@samsung.com> wrote:
> >
> > s/wihtout/without/ in subj
> >
> >> 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
> >>
> >> and actually breaks the feature on such systems.
> >>
> >> Let's restrict memfd backend to systems with sealing support.
> >>
[...]
> >> @@ -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);
> > that would either lead to not clear error that type doesn't exist.
> > it could be better to report sensible error from memfd_backend_memory_alloc() if
> > the feature is required but not supported by host
>
> I'm not sure, but this could break the libvirt capability discovering.
>
> Current patch changes behaviour probably only for RHEL/CentOS 7.2.
> All other systems are not affected. Do you think that we need to
> change behaviour on all the systems?
you are changing behavior anyways, so when users start getting
on some of 'All other systems' start getting 'type doesn't exist'
error, they won't have a clue what's wrong. In case where we are
fixing broken defaults, shouldn't we at least do it the way that
would inform user about misconfiguration.
But I'm not insisting since memfd is fairly new, it might be fine
for device to just disappear.
[...]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-11 15:48 ` Igor Mammedov
@ 2019-01-05 2:43 ` Eduardo Habkost
2019-01-16 10:57 ` Ilya Maximets
0 siblings, 1 reply; 25+ messages in thread
From: Eduardo Habkost @ 2019-01-05 2:43 UTC (permalink / raw)
To: Igor Mammedov
Cc: Ilya Maximets, Marc-André Lureau, qemu-devel, Gerd Hoffmann,
Paolo Bonzini
On Tue, Dec 11, 2018 at 04:48:23PM +0100, Igor Mammedov wrote:
> On Tue, 11 Dec 2018 13:29:19 +0300
> Ilya Maximets <i.maximets@samsung.com> wrote:
>
> CCing libvirt folk for an opinion
>
> > On 10.12.2018 19:18, Igor Mammedov wrote:
> > > On Tue, 27 Nov 2018 16:50:27 +0300
> > > Ilya Maximets <i.maximets@samsung.com> wrote:
> > >
> > > s/wihtout/without/ in subj
> > >
> > >> 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
> > >>
> > >> and actually breaks the feature on such systems.
> > >>
> > >> Let's restrict memfd backend to systems with sealing support.
> > >>
> [...]
> > >> @@ -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);
> > > that would either lead to not clear error that type doesn't exist.
> > > it could be better to report sensible error from memfd_backend_memory_alloc() if
> > > the feature is required but not supported by host
> >
> > I'm not sure, but this could break the libvirt capability discovering.
> >
> > Current patch changes behaviour probably only for RHEL/CentOS 7.2.
> > All other systems are not affected. Do you think that we need to
> > change behaviour on all the systems?
> you are changing behavior anyways, so when users start getting
> on some of 'All other systems' start getting 'type doesn't exist'
> error, they won't have a clue what's wrong. In case where we are
> fixing broken defaults, shouldn't we at least do it the way that
> would inform user about misconfiguration.
>
> But I'm not insisting since memfd is fairly new, it might be fine
> for device to just disappear.
(Sorry for taking so long to reply on this series. I couldn't
review the code yet.)
I'd like to make the QOM type hierarchy static, and not depend on
any runtime host capability checks. But this is not a new
problem in the code, so I don't think it should block this
series.
--
Eduardo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-05 2:43 ` Eduardo Habkost
@ 2019-01-16 10:57 ` Ilya Maximets
0 siblings, 0 replies; 25+ messages in thread
From: Ilya Maximets @ 2019-01-16 10:57 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov, Marc-André Lureau
Cc: qemu-devel, Gerd Hoffmann, Paolo Bonzini
So, can we have any conclusion about this patch and the series?
Best regards, Ilya Maximets.
On 05.01.2019 5:43, Eduardo Habkost wrote:
> On Tue, Dec 11, 2018 at 04:48:23PM +0100, Igor Mammedov wrote:
>> On Tue, 11 Dec 2018 13:29:19 +0300
>> Ilya Maximets <i.maximets@samsung.com> wrote:
>>
>> CCing libvirt folk for an opinion
>>
>>> On 10.12.2018 19:18, Igor Mammedov wrote:
>>>> On Tue, 27 Nov 2018 16:50:27 +0300
>>>> Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>
>>>> s/wihtout/without/ in subj
>>>>
>>>>> 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
>>>>>
>>>>> and actually breaks the feature on such systems.
>>>>>
>>>>> Let's restrict memfd backend to systems with sealing support.
>>>>>
>> [...]
>>>>> @@ -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);
>>>> that would either lead to not clear error that type doesn't exist.
>>>> it could be better to report sensible error from memfd_backend_memory_alloc() if
>>>> the feature is required but not supported by host
>>>
>>> I'm not sure, but this could break the libvirt capability discovering.
>>>
>>> Current patch changes behaviour probably only for RHEL/CentOS 7.2.
>>> All other systems are not affected. Do you think that we need to
>>> change behaviour on all the systems?
>> you are changing behavior anyways, so when users start getting
>> on some of 'All other systems' start getting 'type doesn't exist'
>> error, they won't have a clue what's wrong. In case where we are
>> fixing broken defaults, shouldn't we at least do it the way that
>> would inform user about misconfiguration.
>>
>> But I'm not insisting since memfd is fairly new, it might be fine
>> for device to just disappear.
>
> (Sorry for taking so long to reply on this series. I couldn't
> review the code yet.)
>
> I'd like to make the QOM type hierarchy static, and not depend on
> any runtime host capability checks. But this is not a new
> problem in the code, so I don't think it should block this
> series.
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-11-27 13:50 ` [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support Ilya Maximets
2018-11-27 13:56 ` Marc-André Lureau
2018-12-10 16:18 ` Igor Mammedov
@ 2018-12-11 10:53 ` Daniel P. Berrangé
2018-12-11 11:09 ` Ilya Maximets
2 siblings, 1 reply; 25+ messages in thread
From: Daniel P. Berrangé @ 2018-12-11 10:53 UTC (permalink / raw)
To: Ilya Maximets
Cc: Marc-André Lureau, Eduardo Habkost, qemu-devel,
Gerd Hoffmann, Igor Mammedov, Paolo Bonzini
On Tue, Nov 27, 2018 at 04:50:27PM +0300, Ilya Maximets 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.
Isn't the real problem here that memfd_backend_instance_init() has
unconditionally set "m->seal = true"
Surely, if we don't register the '.seal' property, we should default
that flag to false.
>
> 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
>
> and actually breaks the feature on such systems.
>
> Let's restrict memfd backend to systems with sealing support.
I don't think we need todo that - sealing is optional in the QEMU code,
we simply have it set to the wrong default when sealing is not available.
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-11 10:53 ` Daniel P. Berrangé
@ 2018-12-11 11:09 ` Ilya Maximets
2018-12-12 6:49 ` Gerd Hoffmann
0 siblings, 1 reply; 25+ messages in thread
From: Ilya Maximets @ 2018-12-11 11:09 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Marc-André Lureau, Eduardo Habkost, qemu-devel,
Gerd Hoffmann, Igor Mammedov, Paolo Bonzini
On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> On Tue, Nov 27, 2018 at 04:50:27PM +0300, Ilya Maximets 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.
>
> Isn't the real problem here that memfd_backend_instance_init() has
> unconditionally set "m->seal = true"
>
> Surely, if we don't register the '.seal' property, we should default
> that flag to false.
>
>>
>> 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
>>
>> and actually breaks the feature on such systems.
>>
>> Let's restrict memfd backend to systems with sealing support.
>
> I don't think we need todo that - sealing is optional in the QEMU code,
> we simply have it set to the wrong default when sealing is not available.
That was literally what I've fixed in v1:
https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
but 2 people suggested me to disable memfd entirely for this case.
Do you think I need to get patch from v1 back ?
Gerd, Marc-André, what do you think?
>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-11 11:09 ` Ilya Maximets
@ 2018-12-12 6:49 ` Gerd Hoffmann
2019-01-16 15:30 ` Eduardo Habkost
0 siblings, 1 reply; 25+ messages in thread
From: Gerd Hoffmann @ 2018-12-12 6:49 UTC (permalink / raw)
To: Ilya Maximets
Cc: Daniel P. Berrangé,
Marc-André Lureau, Eduardo Habkost, qemu-devel,
Igor Mammedov, Paolo Bonzini
On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> >>
> >> Let's restrict memfd backend to systems with sealing support.
> >
> > I don't think we need todo that - sealing is optional in the QEMU code,
> > we simply have it set to the wrong default when sealing is not available.
>
> That was literally what I've fixed in v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
>
> but 2 people suggested me to disable memfd entirely for this case.
> Do you think I need to get patch from v1 back ?
>
> Gerd, Marc-André, what do you think?
I still think it makes sense to require sealing support. Sealing is
very useful, and there are only a few kernel versions with memfd but
without sealing. So finding such kernels in the wild will become more
rare over time. I wouldn't worry too much about them.
cheers,
Gerd
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2018-12-12 6:49 ` Gerd Hoffmann
@ 2019-01-16 15:30 ` Eduardo Habkost
2019-01-16 15:46 ` Ilya Maximets
0 siblings, 1 reply; 25+ messages in thread
From: Eduardo Habkost @ 2019-01-16 15:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Ilya Maximets, qemu-devel, Paolo Bonzini, Igor Mammedov,
Marc-André Lureau
On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
> > On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> > >>
> > >> Let's restrict memfd backend to systems with sealing support.
> > >
> > > I don't think we need todo that - sealing is optional in the QEMU code,
> > > we simply have it set to the wrong default when sealing is not available.
> >
> > That was literally what I've fixed in v1:
> > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
> >
> > but 2 people suggested me to disable memfd entirely for this case.
> > Do you think I need to get patch from v1 back ?
> >
> > Gerd, Marc-André, what do you think?
>
> I still think it makes sense to require sealing support. Sealing is
> very useful, and there are only a few kernel versions with memfd but
> without sealing. So finding such kernels in the wild will become more
> rare over time. I wouldn't worry too much about them.
-object memory-backend-memfd,id=mem,size=2M,seal=off still
works on those systems, doesn't it? What's the rationale for
breaking a working configuration without following the
deprecation policy?
--
Eduardo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-16 15:30 ` Eduardo Habkost
@ 2019-01-16 15:46 ` Ilya Maximets
2019-01-16 15:48 ` Daniel P. Berrangé
0 siblings, 1 reply; 25+ messages in thread
From: Ilya Maximets @ 2019-01-16 15:46 UTC (permalink / raw)
To: Eduardo Habkost, Gerd Hoffmann
Cc: qemu-devel, Paolo Bonzini, Igor Mammedov, Marc-André Lureau
On 16.01.2019 18:30, Eduardo Habkost wrote:
> On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
>> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
>>> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
>>>>>
>>>>> Let's restrict memfd backend to systems with sealing support.
>>>>
>>>> I don't think we need todo that - sealing is optional in the QEMU code,
>>>> we simply have it set to the wrong default when sealing is not available.
>>>
>>> That was literally what I've fixed in v1:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
>>>
>>> but 2 people suggested me to disable memfd entirely for this case.
>>> Do you think I need to get patch from v1 back ?
>>>
>>> Gerd, Marc-André, what do you think?
>>
>> I still think it makes sense to require sealing support. Sealing is
>> very useful, and there are only a few kernel versions with memfd but
>> without sealing. So finding such kernels in the wild will become more
>> rare over time. I wouldn't worry too much about them.
>
> -object memory-backend-memfd,id=mem,size=2M,seal=off still
> works on those systems, doesn't it? What's the rationale for
> breaking a working configuration without following the
> deprecation policy?
>
See the commit message.
'.seal' property is not registered if sealing is not supported.
So, there is no way to disable sealing on the system that does not support it.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-16 15:46 ` Ilya Maximets
@ 2019-01-16 15:48 ` Daniel P. Berrangé
2019-01-16 15:54 ` Ilya Maximets
2019-01-16 16:10 ` Eduardo Habkost
0 siblings, 2 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2019-01-16 15:48 UTC (permalink / raw)
To: Ilya Maximets
Cc: Eduardo Habkost, Gerd Hoffmann, Paolo Bonzini, qemu-devel,
Marc-André Lureau, Igor Mammedov
On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote:
>
>
> On 16.01.2019 18:30, Eduardo Habkost wrote:
> > On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
> >> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
> >>> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> >>>>>
> >>>>> Let's restrict memfd backend to systems with sealing support.
> >>>>
> >>>> I don't think we need todo that - sealing is optional in the QEMU code,
> >>>> we simply have it set to the wrong default when sealing is not available.
> >>>
> >>> That was literally what I've fixed in v1:
> >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
> >>>
> >>> but 2 people suggested me to disable memfd entirely for this case.
> >>> Do you think I need to get patch from v1 back ?
> >>>
> >>> Gerd, Marc-André, what do you think?
> >>
> >> I still think it makes sense to require sealing support. Sealing is
> >> very useful, and there are only a few kernel versions with memfd but
> >> without sealing. So finding such kernels in the wild will become more
> >> rare over time. I wouldn't worry too much about them.
> >
> > -object memory-backend-memfd,id=mem,size=2M,seal=off still
> > works on those systems, doesn't it? What's the rationale for
> > breaking a working configuration without following the
> > deprecation policy?
> >
>
> See the commit message.
> '.seal' property is not registered if sealing is not supported.
> So, there is no way to disable sealing on the system that does not support it.
As I pointed out a few lines up, this is simply because QEMU has a bug
setting seal=true as the built-in default value even when it isn't
supported.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-16 15:48 ` Daniel P. Berrangé
@ 2019-01-16 15:54 ` Ilya Maximets
2019-01-16 15:56 ` Daniel P. Berrangé
2019-01-16 16:10 ` Eduardo Habkost
1 sibling, 1 reply; 25+ messages in thread
From: Ilya Maximets @ 2019-01-16 15:54 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eduardo Habkost, Gerd Hoffmann, Paolo Bonzini, qemu-devel,
Marc-André Lureau, Igor Mammedov
On 16.01.2019 18:48, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote:
>>
>>
>> On 16.01.2019 18:30, Eduardo Habkost wrote:
>>> On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
>>>> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
>>>>> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
>>>>>>>
>>>>>>> Let's restrict memfd backend to systems with sealing support.
>>>>>>
>>>>>> I don't think we need todo that - sealing is optional in the QEMU code,
>>>>>> we simply have it set to the wrong default when sealing is not available.
>>>>>
>>>>> That was literally what I've fixed in v1:
>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
>>>>>
>>>>> but 2 people suggested me to disable memfd entirely for this case.
>>>>> Do you think I need to get patch from v1 back ?
>>>>>
>>>>> Gerd, Marc-André, what do you think?
>>>>
>>>> I still think it makes sense to require sealing support. Sealing is
>>>> very useful, and there are only a few kernel versions with memfd but
>>>> without sealing. So finding such kernels in the wild will become more
>>>> rare over time. I wouldn't worry too much about them.
>>>
>>> -object memory-backend-memfd,id=mem,size=2M,seal=off still
>>> works on those systems, doesn't it? What's the rationale for
>>> breaking a working configuration without following the
>>> deprecation policy?
>>>
>>
>> See the commit message.
>> '.seal' property is not registered if sealing is not supported.
>> So, there is no way to disable sealing on the system that does not support it.
>
> As I pointed out a few lines up, this is simply because QEMU has a bug
> setting seal=true as the built-in default value even when it isn't
> supported.
So, do you think I need to return to the solution from my v1:
https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-16 15:54 ` Ilya Maximets
@ 2019-01-16 15:56 ` Daniel P. Berrangé
0 siblings, 0 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2019-01-16 15:56 UTC (permalink / raw)
To: Ilya Maximets
Cc: Eduardo Habkost, Gerd Hoffmann, Paolo Bonzini, qemu-devel,
Marc-André Lureau, Igor Mammedov
On Wed, Jan 16, 2019 at 06:54:38PM +0300, Ilya Maximets wrote:
> On 16.01.2019 18:48, Daniel P. Berrangé wrote:
> > On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote:
> >>
> >>
> >> On 16.01.2019 18:30, Eduardo Habkost wrote:
> >>> On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
> >>>> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
> >>>>> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> >>>>>>>
> >>>>>>> Let's restrict memfd backend to systems with sealing support.
> >>>>>>
> >>>>>> I don't think we need todo that - sealing is optional in the QEMU code,
> >>>>>> we simply have it set to the wrong default when sealing is not available.
> >>>>>
> >>>>> That was literally what I've fixed in v1:
> >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
> >>>>>
> >>>>> but 2 people suggested me to disable memfd entirely for this case.
> >>>>> Do you think I need to get patch from v1 back ?
> >>>>>
> >>>>> Gerd, Marc-André, what do you think?
> >>>>
> >>>> I still think it makes sense to require sealing support. Sealing is
> >>>> very useful, and there are only a few kernel versions with memfd but
> >>>> without sealing. So finding such kernels in the wild will become more
> >>>> rare over time. I wouldn't worry too much about them.
> >>>
> >>> -object memory-backend-memfd,id=mem,size=2M,seal=off still
> >>> works on those systems, doesn't it? What's the rationale for
> >>> breaking a working configuration without following the
> >>> deprecation policy?
> >>>
> >>
> >> See the commit message.
> >> '.seal' property is not registered if sealing is not supported.
> >> So, there is no way to disable sealing on the system that does not support it.
> >
> > As I pointed out a few lines up, this is simply because QEMU has a bug
> > setting seal=true as the built-in default value even when it isn't
> > supported.
>
> So, do you think I need to return to the solution from my v1:
> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
That is my preference, but we don't have universal agreement :-(
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hostmem-memfd: disable for systems wihtout sealing support
2019-01-16 15:48 ` Daniel P. Berrangé
2019-01-16 15:54 ` Ilya Maximets
@ 2019-01-16 16:10 ` Eduardo Habkost
1 sibling, 0 replies; 25+ messages in thread
From: Eduardo Habkost @ 2019-01-16 16:10 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Ilya Maximets, Gerd Hoffmann, Paolo Bonzini, qemu-devel,
Marc-André Lureau, Igor Mammedov
On Wed, Jan 16, 2019 at 03:48:57PM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote:
> >
> >
> > On 16.01.2019 18:30, Eduardo Habkost wrote:
> > > On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote:
> > >> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote:
> > >>> On 11.12.2018 13:53, Daniel P. Berrangé wrote:
> > >>>>>
> > >>>>> Let's restrict memfd backend to systems with sealing support.
> > >>>>
> > >>>> I don't think we need todo that - sealing is optional in the QEMU code,
> > >>>> we simply have it set to the wrong default when sealing is not available.
> > >>>
> > >>> That was literally what I've fixed in v1:
> > >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html
> > >>>
> > >>> but 2 people suggested me to disable memfd entirely for this case.
> > >>> Do you think I need to get patch from v1 back ?
> > >>>
> > >>> Gerd, Marc-André, what do you think?
> > >>
> > >> I still think it makes sense to require sealing support. Sealing is
> > >> very useful, and there are only a few kernel versions with memfd but
> > >> without sealing. So finding such kernels in the wild will become more
> > >> rare over time. I wouldn't worry too much about them.
> > >
> > > -object memory-backend-memfd,id=mem,size=2M,seal=off still
> > > works on those systems, doesn't it? What's the rationale for
> > > breaking a working configuration without following the
> > > deprecation policy?
> > >
> >
> > See the commit message.
> > '.seal' property is not registered if sealing is not supported.
> > So, there is no way to disable sealing on the system that does not support it.
>
> As I pointed out a few lines up, this is simply because QEMU has a bug
> setting seal=true as the built-in default value even when it isn't
> supported.
Changing to seal=false by default may make it work on some hosts,
but I don't see the point of increasing our support burden just
for a few kernel versions. I agree with Gerd, I think it's
simpler to keep it unsupported.
--
Eduardo
^ permalink raw reply [flat|nested] 25+ messages in thread