[PATCH v2 0/3] Fix hugepages with memfd on s390x and clean up related code

[PATCH v2 0/3] Fix hugepages with memfd on s390x and clean up related code

[Thomas Huth]

The first patch fixes the problem that hugepages cannot be used via
the "memory-backend-memfd" object on s390x. The second and third patch
are some clean-ups that can be done after generalizing the code in the
first patch.

v2:
 - Use qemu_ram_pagesize(memdev->mr.ram_block) instead of adding
   additional code for the memfd object
 - Added the two clean-up patches on top to simplify the code

Thomas Huth (3):
  backends/hostmem: Fix support of memory-backend-memfd in
    qemu_maxrampagesize()
  softmmu/physmem: Remove the ifdef __linux__  around the pagesize
    functions
  util/mmap-alloc: Remove qemu_mempath_getpagesize()

 include/qemu/mmap-alloc.h |  2 --
 backends/hostmem.c        | 14 ++------------
 softmmu/physmem.c         | 17 -----------------
 util/mmap-alloc.c         | 31 -------------------------------
 4 files changed, 2 insertions(+), 62 deletions(-)

-- 
2.31.1

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

[Thomas Huth]

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 generalizing the code, so that it
looks at qemu_ram_pagesize(memdev->mr.ram_block) instead of re-trying
to get the information from the filesystem.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 backends/hostmem.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 624bb7ecd3..4428e06738 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -306,22 +306,12 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
     return backend->is_mapped;
 }
 
-#ifdef __linux__
 size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
 {
-    Object *obj = OBJECT(memdev);
-    char *path = object_property_get_str(obj, "mem-path", NULL);
-    size_t pagesize = qemu_mempath_getpagesize(path);
-
-    g_free(path);
+    size_t pagesize = qemu_ram_pagesize(memdev->mr.ram_block);
+    g_assert(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

[PATCH v2 2/3] softmmu/physmem: Remove the ifdef __linux__ around the pagesize functions

[Thomas Huth]

Now that host_memory_backend_pagesize() is not depending on the hugetlb
memory path handling anymore, we can also remove the #ifdef and the
TOCTTOU comment from the calling functions - the code should now work
equally well on all host architectures.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 softmmu/physmem.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index dc3c3e5f2e..50231bab30 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1331,13 +1331,6 @@ GString *ram_block_format(void)
     return buf;
 }
 
-#ifdef __linux__
-/*
- * FIXME TOCTTOU: this iterates over memory backends' mem-path, which
- * may or may not name the same files / on the same filesystem now as
- * when we actually open and map them.  Iterate over the file
- * descriptors instead, and use qemu_fd_getpagesize().
- */
 static int find_min_backend_pagesize(Object *obj, void *opaque)
 {
     long *hpsize_min = opaque;
@@ -1391,16 +1384,6 @@ long qemu_maxrampagesize(void)
     object_child_foreach(memdev_root, find_max_backend_pagesize, &pagesize);
     return pagesize;
 }
-#else
-long qemu_minrampagesize(void)
-{
-    return qemu_real_host_page_size();
-}
-long qemu_maxrampagesize(void)
-{
-    return qemu_real_host_page_size();
-}
-#endif
 
 #ifdef CONFIG_POSIX
 static int64_t get_file_size(int fd)
-- 
2.31.1

[PATCH v2 3/3] util/mmap-alloc: Remove qemu_mempath_getpagesize()

[Thomas Huth]

The last user of this function has just been removed, so we can
drop this function now, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qemu/mmap-alloc.h |  2 --
 util/mmap-alloc.c         | 31 -------------------------------
 2 files changed, 33 deletions(-)

diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
index 5076695cc8..2825e231a7 100644
--- a/include/qemu/mmap-alloc.h
+++ b/include/qemu/mmap-alloc.h
@@ -4,8 +4,6 @@
 
 size_t qemu_fd_getpagesize(int fd);
 
-size_t qemu_mempath_getpagesize(const char *mem_path);
-
 /**
  * qemu_ram_mmap: mmap anonymous memory, the specified file or device.
  *
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 5b90cb68ea..5ed7d29183 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -53,37 +53,6 @@ size_t qemu_fd_getpagesize(int fd)
     return qemu_real_host_page_size();
 }
 
-size_t qemu_mempath_getpagesize(const char *mem_path)
-{
-#ifdef CONFIG_LINUX
-    struct statfs fs;
-    int ret;
-
-    if (mem_path) {
-        do {
-            ret = statfs(mem_path, &fs);
-        } while (ret != 0 && errno == EINTR);
-
-        if (ret != 0) {
-            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
-                    strerror(errno));
-            exit(1);
-        }
-
-        if (fs.f_type == HUGETLBFS_MAGIC) {
-            /* It's hugepage, return the huge page size */
-            return fs.f_bsize;
-        }
-    }
-#ifdef __sparc__
-    /* SPARC Linux needs greater alignment than the pagesize */
-    return QEMU_VMALLOC_ALIGN;
-#endif
-#endif
-
-    return qemu_real_host_page_size();
-}
-
 #define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory"
 static bool map_noreserve_effective(int fd, uint32_t qemu_map_flags)
 {
-- 
2.31.1

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

[David Hildenbrand]

On 10.08.22 14:57, 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 generalizing the code, so that it
> looks at qemu_ram_pagesize(memdev->mr.ram_block) instead of re-trying
> to get the information from the filesystem.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  backends/hostmem.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 624bb7ecd3..4428e06738 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -306,22 +306,12 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>      return backend->is_mapped;
>  }
>  
> -#ifdef __linux__
>  size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>  {
> -    Object *obj = OBJECT(memdev);
> -    char *path = object_property_get_str(obj, "mem-path", NULL);
> -    size_t pagesize = qemu_mempath_getpagesize(path);
> -
> -    g_free(path);
> +    size_t pagesize = qemu_ram_pagesize(memdev->mr.ram_block);
> +    g_assert(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)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] softmmu/physmem: Remove the ifdef __linux__ around the pagesize functions

[David Hildenbrand]

On 10.08.22 14:57, Thomas Huth wrote:
> Now that host_memory_backend_pagesize() is not depending on the hugetlb
> memory path handling anymore, we can also remove the #ifdef and the
> TOCTTOU comment from the calling functions - the code should now work
> equally well on all host architectures.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  softmmu/physmem.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index dc3c3e5f2e..50231bab30 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1331,13 +1331,6 @@ GString *ram_block_format(void)
>      return buf;
>  }
>  
> -#ifdef __linux__
> -/*
> - * FIXME TOCTTOU: this iterates over memory backends' mem-path, which
> - * may or may not name the same files / on the same filesystem now as
> - * when we actually open and map them.  Iterate over the file
> - * descriptors instead, and use qemu_fd_getpagesize().
> - */
>  static int find_min_backend_pagesize(Object *obj, void *opaque)
>  {
>      long *hpsize_min = opaque;
> @@ -1391,16 +1384,6 @@ long qemu_maxrampagesize(void)
>      object_child_foreach(memdev_root, find_max_backend_pagesize, &pagesize);
>      return pagesize;
>  }
> -#else
> -long qemu_minrampagesize(void)
> -{
> -    return qemu_real_host_page_size();
> -}
> -long qemu_maxrampagesize(void)
> -{
> -    return qemu_real_host_page_size();
> -}
> -#endif
>  
>  #ifdef CONFIG_POSIX
>  static int64_t get_file_size(int fd)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/3] util/mmap-alloc: Remove qemu_mempath_getpagesize()

[David Hildenbrand]

On 10.08.22 14:57, Thomas Huth wrote:
> The last user of this function has just been removed, so we can
> drop this function now, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[Claudio Imbrenda]

On Wed, 10 Aug 2022 14:57:18 +0200
Thomas Huth <thuth@redhat.com> 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 generalizing the code, so that it
> looks at qemu_ram_pagesize(memdev->mr.ram_block) instead of re-trying
> to get the information from the filesystem.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2116496
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

more removed lines than added, I like it

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  backends/hostmem.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 624bb7ecd3..4428e06738 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -306,22 +306,12 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
>      return backend->is_mapped;
>  }
>  
> -#ifdef __linux__
>  size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>  {
> -    Object *obj = OBJECT(memdev);
> -    char *path = object_property_get_str(obj, "mem-path", NULL);
> -    size_t pagesize = qemu_mempath_getpagesize(path);
> -
> -    g_free(path);
> +    size_t pagesize = qemu_ram_pagesize(memdev->mr.ram_block);
> +    g_assert(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)

Re: [PATCH v2 3/3] util/mmap-alloc: Remove qemu_mempath_getpagesize()

[Claudio Imbrenda]

On Wed, 10 Aug 2022 14:57:20 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The last user of this function has just been removed, so we can
> drop this function now, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  include/qemu/mmap-alloc.h |  2 --
>  util/mmap-alloc.c         | 31 -------------------------------
>  2 files changed, 33 deletions(-)
> 
> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> index 5076695cc8..2825e231a7 100644
> --- a/include/qemu/mmap-alloc.h
> +++ b/include/qemu/mmap-alloc.h
> @@ -4,8 +4,6 @@
>  
>  size_t qemu_fd_getpagesize(int fd);
>  
> -size_t qemu_mempath_getpagesize(const char *mem_path);
> -
>  /**
>   * qemu_ram_mmap: mmap anonymous memory, the specified file or device.
>   *
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 5b90cb68ea..5ed7d29183 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -53,37 +53,6 @@ size_t qemu_fd_getpagesize(int fd)
>      return qemu_real_host_page_size();
>  }
>  
> -size_t qemu_mempath_getpagesize(const char *mem_path)
> -{
> -#ifdef CONFIG_LINUX
> -    struct statfs fs;
> -    int ret;
> -
> -    if (mem_path) {
> -        do {
> -            ret = statfs(mem_path, &fs);
> -        } while (ret != 0 && errno == EINTR);
> -
> -        if (ret != 0) {
> -            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
> -                    strerror(errno));
> -            exit(1);
> -        }
> -
> -        if (fs.f_type == HUGETLBFS_MAGIC) {
> -            /* It's hugepage, return the huge page size */
> -            return fs.f_bsize;
> -        }
> -    }
> -#ifdef __sparc__
> -    /* SPARC Linux needs greater alignment than the pagesize */
> -    return QEMU_VMALLOC_ALIGN;
> -#endif
> -#endif
> -
> -    return qemu_real_host_page_size();
> -}
> -
>  #define OVERCOMMIT_MEMORY_PATH "/proc/sys/vm/overcommit_memory"
>  static bool map_noreserve_effective(int fd, uint32_t qemu_map_flags)
>  {

Re: [PATCH v2 2/3] softmmu/physmem: Remove the ifdef __linux__ around the pagesize functions

[Claudio Imbrenda]

On Wed, 10 Aug 2022 14:57:19 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Now that host_memory_backend_pagesize() is not depending on the hugetlb
> memory path handling anymore, we can also remove the #ifdef and the
> TOCTTOU comment from the calling functions - the code should now work
> equally well on all host architectures.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  softmmu/physmem.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index dc3c3e5f2e..50231bab30 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1331,13 +1331,6 @@ GString *ram_block_format(void)
>      return buf;
>  }
>  
> -#ifdef __linux__
> -/*
> - * FIXME TOCTTOU: this iterates over memory backends' mem-path, which
> - * may or may not name the same files / on the same filesystem now as
> - * when we actually open and map them.  Iterate over the file
> - * descriptors instead, and use qemu_fd_getpagesize().
> - */
>  static int find_min_backend_pagesize(Object *obj, void *opaque)
>  {
>      long *hpsize_min = opaque;
> @@ -1391,16 +1384,6 @@ long qemu_maxrampagesize(void)
>      object_child_foreach(memdev_root, find_max_backend_pagesize, &pagesize);
>      return pagesize;
>  }
> -#else
> -long qemu_minrampagesize(void)
> -{
> -    return qemu_real_host_page_size();
> -}
> -long qemu_maxrampagesize(void)
> -{
> -    return qemu_real_host_page_size();
> -}
> -#endif
>  
>  #ifdef CONFIG_POSIX
>  static int64_t get_file_size(int fd)

Re: [PATCH v2 0/3] Fix hugepages with memfd on s390x and clean up related code

[Claudio Imbrenda]

On Wed, 10 Aug 2022 14:57:17 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The first patch fixes the problem that hugepages cannot be used via
> the "memory-backend-memfd" object on s390x. The second and third patch
> are some clean-ups that can be done after generalizing the code in the
> first patch.

thanks for fixing this

> 
> v2:
>  - Use qemu_ram_pagesize(memdev->mr.ram_block) instead of adding
>    additional code for the memfd object
>  - Added the two clean-up patches on top to simplify the code
> 
> Thomas Huth (3):
>   backends/hostmem: Fix support of memory-backend-memfd in
>     qemu_maxrampagesize()
>   softmmu/physmem: Remove the ifdef __linux__  around the pagesize
>     functions
>   util/mmap-alloc: Remove qemu_mempath_getpagesize()
> 
>  include/qemu/mmap-alloc.h |  2 --
>  backends/hostmem.c        | 14 ++------------
>  softmmu/physmem.c         | 17 -----------------
>  util/mmap-alloc.c         | 31 -------------------------------
>  4 files changed, 2 insertions(+), 62 deletions(-)
> 

Re: [PATCH v2 0/3] Fix hugepages with memfd on s390x and clean up related code

[Thomas Huth]

On 10/08/2022 14.57, Thomas Huth wrote:
> The first patch fixes the problem that hugepages cannot be used via
> the "memory-backend-memfd" object on s390x. The second and third patch
> are some clean-ups that can be done after generalizing the code in the
> first patch.
> 
> v2:
>   - Use qemu_ram_pagesize(memdev->mr.ram_block) instead of adding
>     additional code for the memfd object
>   - Added the two clean-up patches on top to simplify the code
> 
> Thomas Huth (3):
>    backends/hostmem: Fix support of memory-backend-memfd in
>      qemu_maxrampagesize()
>    softmmu/physmem: Remove the ifdef __linux__  around the pagesize
>      functions
>    util/mmap-alloc: Remove qemu_mempath_getpagesize()
> 
>   include/qemu/mmap-alloc.h |  2 --
>   backends/hostmem.c        | 14 ++------------
>   softmmu/physmem.c         | 17 -----------------
>   util/mmap-alloc.c         | 31 -------------------------------
>   4 files changed, 2 insertions(+), 62 deletions(-)
> 

Thanks to David and Claudio for the reviews! FWIW, I'll take this through my 
s390x-next branch since it fixes a s390x-related problem:

  https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas