[PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()

Thomas Huth posted 1 patch 1 year, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220810063204.3589543-1-thuth@redhat.com
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>
include/sysemu/hostmem.h |  8 +++++++-
backends/hostmem-memfd.c |  2 --
backends/hostmem.c       | 27 +++++++++++++++++----------
3 files changed, 24 insertions(+), 13 deletions(-)
[PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()
Posted by Thomas Huth 1 year, 7 months ago
It is currently not possible yet to use "memory-backend-memfd" on s390x
with hugepages enabled. This problem is caused by qemu_maxrampagesize()
not taking memory-backend-memfd objects into account yet, so the code
in s390_memory_init() fails to enable the huge page support there via
s390_set_max_pagesize(). Fix it by looking at the memory-backend-memfd
in the host_memory_backend_pagesize() function, too.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/sysemu/hostmem.h |  8 +++++++-
 backends/hostmem-memfd.c |  2 --
 backends/hostmem.c       | 27 +++++++++++++++++----------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 9ff5c16963..d983ae6c01 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -34,10 +34,16 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
 /* hostmem-file.c */
 /**
  * @TYPE_MEMORY_BACKEND_FILE:
- * name of backend that uses mmap on a file descriptor
+ * name of backend that uses mmap on a file
  */
 #define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
 
+/* hostmem-memfd.c */
+/**
+ * @TYPE_MEMORY_BACKEND_MEMFD:
+ * name of backend that uses mmap on a memfd file descriptor
+ */
+#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
 
 /**
  * HostMemoryBackendClass:
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 3fc85c3db8..1ab2085e49 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -18,8 +18,6 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 
-#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
-
 OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD)
 
 
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 624bb7ecd3..ebce887105 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -306,22 +306,29 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
     return backend->is_mapped;
 }
 
-#ifdef __linux__
 size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
 {
+    size_t pagesize = 0;
+
+#ifdef __linux__
     Object *obj = OBJECT(memdev);
-    char *path = object_property_get_str(obj, "mem-path", NULL);
-    size_t pagesize = qemu_mempath_getpagesize(path);
 
-    g_free(path);
+    if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE)) {
+        char *path = object_property_get_str(obj, "mem-path", NULL);
+        pagesize = qemu_mempath_getpagesize(path);
+        g_free(path);
+    } else if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_MEMFD) &&
+               object_property_get_bool(obj, "hugetlb", &error_abort)) {
+        pagesize = object_property_get_int(obj, "hugetlbsize", &error_abort);
+    }
+#endif
+
+    if (!pagesize) {
+        pagesize = qemu_real_host_page_size();
+    }
+
     return pagesize;
 }
-#else
-size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
-{
-    return qemu_real_host_page_size();
-}
-#endif
 
 static void
 host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
-- 
2.31.1
Re: [PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()
Posted by David Hildenbrand 1 year, 7 months ago
On 10.08.22 08:32, Thomas Huth wrote:
> It is currently not possible yet to use "memory-backend-memfd" on s390x
> with hugepages enabled. This problem is caused by qemu_maxrampagesize()
> not taking memory-backend-memfd objects into account yet, so the code
> in s390_memory_init() fails to enable the huge page support there via
> s390_set_max_pagesize(). Fix it by looking at the memory-backend-memfd
> in the host_memory_backend_pagesize() function, too.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/sysemu/hostmem.h |  8 +++++++-
>  backends/hostmem-memfd.c |  2 --
>  backends/hostmem.c       | 27 +++++++++++++++++----------
>  3 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 9ff5c16963..d983ae6c01 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -34,10 +34,16 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
>  /* hostmem-file.c */
>  /**
>   * @TYPE_MEMORY_BACKEND_FILE:
> - * name of backend that uses mmap on a file descriptor
> + * name of backend that uses mmap on a file
>   */
>  #define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
>  
> +/* hostmem-memfd.c */
> +/**
> + * @TYPE_MEMORY_BACKEND_MEMFD:
> + * name of backend that uses mmap on a memfd file descriptor
> + */
> +#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>  
>  /**
>   * HostMemoryBackendClass:
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index 3fc85c3db8..1ab2085e49 100644
> --- a/backends/hostmem-memfd.c
> +++ b/backends/hostmem-memfd.c
> @@ -18,8 +18,6 @@
>  #include "qapi/error.h"
>  #include "qom/object.h"
>  
> -#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
> -
>  OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD)
>  
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 624bb7ecd3..ebce887105 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -306,22 +306,29 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>      return backend->is_mapped;
>  }
>  
> -#ifdef __linux__
>  size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>  {
> +    size_t pagesize = 0;
> +
> +#ifdef __linux__
>      Object *obj = OBJECT(memdev);
> -    char *path = object_property_get_str(obj, "mem-path", NULL);
> -    size_t pagesize = qemu_mempath_getpagesize(path);
>  
> -    g_free(path);
> +    if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE)) {
> +        char *path = object_property_get_str(obj, "mem-path", NULL);
> +        pagesize = qemu_mempath_getpagesize(path);
> +        g_free(path);
> +    } else if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_MEMFD) &&
> +               object_property_get_bool(obj, "hugetlb", &error_abort)) {
> +        pagesize = object_property_get_int(obj, "hugetlbsize", &error_abort);
> +    }
> +#endif

Why can't we simply rely on

qemu_ram_pagesize(memdev->mr.ram_block);

?

-- 
Thanks,

David / dhildenb
Re: [PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()
Posted by Thomas Huth 1 year, 7 months ago
On 10/08/2022 09.32, David Hildenbrand wrote:
> On 10.08.22 08:32, Thomas Huth wrote:
>> It is currently not possible yet to use "memory-backend-memfd" on s390x
>> with hugepages enabled. This problem is caused by qemu_maxrampagesize()
>> not taking memory-backend-memfd objects into account yet, so the code
>> in s390_memory_init() fails to enable the huge page support there via
>> s390_set_max_pagesize(). Fix it by looking at the memory-backend-memfd
>> in the host_memory_backend_pagesize() function, too.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/sysemu/hostmem.h |  8 +++++++-
>>   backends/hostmem-memfd.c |  2 --
>>   backends/hostmem.c       | 27 +++++++++++++++++----------
>>   3 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>> index 9ff5c16963..d983ae6c01 100644
>> --- a/include/sysemu/hostmem.h
>> +++ b/include/sysemu/hostmem.h
>> @@ -34,10 +34,16 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
>>   /* hostmem-file.c */
>>   /**
>>    * @TYPE_MEMORY_BACKEND_FILE:
>> - * name of backend that uses mmap on a file descriptor
>> + * name of backend that uses mmap on a file
>>    */
>>   #define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
>>   
>> +/* hostmem-memfd.c */
>> +/**
>> + * @TYPE_MEMORY_BACKEND_MEMFD:
>> + * name of backend that uses mmap on a memfd file descriptor
>> + */
>> +#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>>   
>>   /**
>>    * HostMemoryBackendClass:
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index 3fc85c3db8..1ab2085e49 100644
>> --- a/backends/hostmem-memfd.c
>> +++ b/backends/hostmem-memfd.c
>> @@ -18,8 +18,6 @@
>>   #include "qapi/error.h"
>>   #include "qom/object.h"
>>   
>> -#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>> -
>>   OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD)
>>   
>>   
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 624bb7ecd3..ebce887105 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -306,22 +306,29 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>>       return backend->is_mapped;
>>   }
>>   
>> -#ifdef __linux__
>>   size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>>   {
>> +    size_t pagesize = 0;
>> +
>> +#ifdef __linux__
>>       Object *obj = OBJECT(memdev);
>> -    char *path = object_property_get_str(obj, "mem-path", NULL);
>> -    size_t pagesize = qemu_mempath_getpagesize(path);
>>   
>> -    g_free(path);
>> +    if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE)) {
>> +        char *path = object_property_get_str(obj, "mem-path", NULL);
>> +        pagesize = qemu_mempath_getpagesize(path);
>> +        g_free(path);
>> +    } else if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_MEMFD) &&
>> +               object_property_get_bool(obj, "hugetlb", &error_abort)) {
>> +        pagesize = object_property_get_int(obj, "hugetlbsize", &error_abort);
>> +    }
>> +#endif
> 
> Why can't we simply rely on
> 
> qemu_ram_pagesize(memdev->mr.ram_block);

Good idea! That way, we could even get rid of the "#ifdef __linux__" macros 
here, I guess ... I'll give it a try and send a v2 if it works.

  Thomas
Re: [PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()
Posted by David Hildenbrand 1 year, 7 months ago
On 10.08.22 10:11, Thomas Huth wrote:
> On 10/08/2022 09.32, David Hildenbrand wrote:
>> On 10.08.22 08:32, Thomas Huth wrote:
>>> It is currently not possible yet to use "memory-backend-memfd" on s390x
>>> with hugepages enabled. This problem is caused by qemu_maxrampagesize()
>>> not taking memory-backend-memfd objects into account yet, so the code
>>> in s390_memory_init() fails to enable the huge page support there via
>>> s390_set_max_pagesize(). Fix it by looking at the memory-backend-memfd
>>> in the host_memory_backend_pagesize() function, too.
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   include/sysemu/hostmem.h |  8 +++++++-
>>>   backends/hostmem-memfd.c |  2 --
>>>   backends/hostmem.c       | 27 +++++++++++++++++----------
>>>   3 files changed, 24 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>>> index 9ff5c16963..d983ae6c01 100644
>>> --- a/include/sysemu/hostmem.h
>>> +++ b/include/sysemu/hostmem.h
>>> @@ -34,10 +34,16 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
>>>   /* hostmem-file.c */
>>>   /**
>>>    * @TYPE_MEMORY_BACKEND_FILE:
>>> - * name of backend that uses mmap on a file descriptor
>>> + * name of backend that uses mmap on a file
>>>    */
>>>   #define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
>>>   
>>> +/* hostmem-memfd.c */
>>> +/**
>>> + * @TYPE_MEMORY_BACKEND_MEMFD:
>>> + * name of backend that uses mmap on a memfd file descriptor
>>> + */
>>> +#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>>>   
>>>   /**
>>>    * HostMemoryBackendClass:
>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>>> index 3fc85c3db8..1ab2085e49 100644
>>> --- a/backends/hostmem-memfd.c
>>> +++ b/backends/hostmem-memfd.c
>>> @@ -18,8 +18,6 @@
>>>   #include "qapi/error.h"
>>>   #include "qom/object.h"
>>>   
>>> -#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>>> -
>>>   OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD)
>>>   
>>>   
>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>> index 624bb7ecd3..ebce887105 100644
>>> --- a/backends/hostmem.c
>>> +++ b/backends/hostmem.c
>>> @@ -306,22 +306,29 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>>>       return backend->is_mapped;
>>>   }
>>>   
>>> -#ifdef __linux__
>>>   size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>>>   {
>>> +    size_t pagesize = 0;
>>> +
>>> +#ifdef __linux__
>>>       Object *obj = OBJECT(memdev);
>>> -    char *path = object_property_get_str(obj, "mem-path", NULL);
>>> -    size_t pagesize = qemu_mempath_getpagesize(path);
>>>   
>>> -    g_free(path);
>>> +    if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE)) {
>>> +        char *path = object_property_get_str(obj, "mem-path", NULL);
>>> +        pagesize = qemu_mempath_getpagesize(path);
>>> +        g_free(path);
>>> +    } else if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_MEMFD) &&
>>> +               object_property_get_bool(obj, "hugetlb", &error_abort)) {
>>> +        pagesize = object_property_get_int(obj, "hugetlbsize", &error_abort);
>>> +    }
>>> +#endif
>>
>> Why can't we simply rely on
>>
>> qemu_ram_pagesize(memdev->mr.ram_block);
> 
> Good idea! That way, we could even get rid of the "#ifdef __linux__" macros 
> here, I guess ... I'll give it a try and send a v2 if it works.

At first I wondered if we could have to deal with semi-iniutialized
backendds here, but I think we should simply always already have the
MR/RamBlock here.


-- 
Thanks,

David / dhildenb
Re: [PATCH] backends/hostmem: Fix support of memory-backend-memfd in qemu_maxrampagesize()
Posted by Thomas Huth 1 year, 7 months ago
On 10/08/2022 10.31, David Hildenbrand wrote:
> On 10.08.22 10:11, Thomas Huth wrote:
>> On 10/08/2022 09.32, David Hildenbrand wrote:
>>> On 10.08.22 08:32, Thomas Huth wrote:
>>>> It is currently not possible yet to use "memory-backend-memfd" on s390x
>>>> with hugepages enabled. This problem is caused by qemu_maxrampagesize()
>>>> not taking memory-backend-memfd objects into account yet, so the code
>>>> in s390_memory_init() fails to enable the huge page support there via
>>>> s390_set_max_pagesize(). Fix it by looking at the memory-backend-memfd
>>>> in the host_memory_backend_pagesize() function, too.
>>>>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    include/sysemu/hostmem.h |  8 +++++++-
>>>>    backends/hostmem-memfd.c |  2 --
>>>>    backends/hostmem.c       | 27 +++++++++++++++++----------
>>>>    3 files changed, 24 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>>>> index 9ff5c16963..d983ae6c01 100644
>>>> --- a/include/sysemu/hostmem.h
>>>> +++ b/include/sysemu/hostmem.h
>>>> @@ -34,10 +34,16 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
>>>>    /* hostmem-file.c */
>>>>    /**
>>>>     * @TYPE_MEMORY_BACKEND_FILE:
>>>> - * name of backend that uses mmap on a file descriptor
>>>> + * name of backend that uses mmap on a file
>>>>     */
>>>>    #define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
>>>>    
>>>> +/* hostmem-memfd.c */
>>>> +/**
>>>> + * @TYPE_MEMORY_BACKEND_MEMFD:
>>>> + * name of backend that uses mmap on a memfd file descriptor
>>>> + */
>>>> +#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>>>>    
>>>>    /**
>>>>     * HostMemoryBackendClass:
>>>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>>>> index 3fc85c3db8..1ab2085e49 100644
>>>> --- a/backends/hostmem-memfd.c
>>>> +++ b/backends/hostmem-memfd.c
>>>> @@ -18,8 +18,6 @@
>>>>    #include "qapi/error.h"
>>>>    #include "qom/object.h"
>>>>    
>>>> -#define TYPE_MEMORY_BACKEND_MEMFD "memory-backend-memfd"
>>>> -
>>>>    OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendMemfd, MEMORY_BACKEND_MEMFD)
>>>>    
>>>>    
>>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>>> index 624bb7ecd3..ebce887105 100644
>>>> --- a/backends/hostmem.c
>>>> +++ b/backends/hostmem.c
>>>> @@ -306,22 +306,29 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>>>>        return backend->is_mapped;
>>>>    }
>>>>    
>>>> -#ifdef __linux__
>>>>    size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>>>>    {
>>>> +    size_t pagesize = 0;
>>>> +
>>>> +#ifdef __linux__
>>>>        Object *obj = OBJECT(memdev);
>>>> -    char *path = object_property_get_str(obj, "mem-path", NULL);
>>>> -    size_t pagesize = qemu_mempath_getpagesize(path);
>>>>    
>>>> -    g_free(path);
>>>> +    if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE)) {
>>>> +        char *path = object_property_get_str(obj, "mem-path", NULL);
>>>> +        pagesize = qemu_mempath_getpagesize(path);
>>>> +        g_free(path);
>>>> +    } else if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_MEMFD) &&
>>>> +               object_property_get_bool(obj, "hugetlb", &error_abort)) {
>>>> +        pagesize = object_property_get_int(obj, "hugetlbsize", &error_abort);
>>>> +    }
>>>> +#endif
>>>
>>> Why can't we simply rely on
>>>
>>> qemu_ram_pagesize(memdev->mr.ram_block);
>>
>> Good idea! That way, we could even get rid of the "#ifdef __linux__" macros
>> here, I guess ... I'll give it a try and send a v2 if it works.
> 
> At first I wondered if we could have to deal with semi-iniutialized
> backendds here, but I think we should simply always already have the
> MR/RamBlock here.

I'll add an assert(pagesize >= getpagesize()) there, just to be sure.

  Thomas