[Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Dominik Dingel]

While in the anonymous ram case we already take care of the right alignment
such an alignment gurantee does not exist for file backed ram allocation.

Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
as we need to satisfy an alignment up to segments.

Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>

---

v2 -> v3:
Skipping additional variable and just use alignment of memory region.
As memory will not be backpropagated it is enough to round up to page_sizes.

v1 -> v2:
While enforcing alignments we allow memory sizes on page_size.
On mmap the memory size will be round up to alignments.

I thought about moving this alignment into qemu_ram_mmap but the result
was a lot of code churn, the other possibility was to create an additional
define ending up with two defines with the same semantics.
---
 exec.c               |  5 +++--
 include/qemu/osdep.h | 13 +++++++++++++
 util/oslib-posix.c   | 13 -------------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/exec.c b/exec.c
index c4f9036..fc75266 100644
--- a/exec.c
+++ b/exec.c
@@ -1296,7 +1296,7 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 
     page_size = qemu_fd_getpagesize(fd);
-    block->mr->align = page_size;
+    block->mr->align = MAX(page_size, QEMU_VMALLOC_ALIGN);
 
     if (memory < page_size) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
@@ -1317,7 +1317,8 @@ static void *file_ram_alloc(RAMBlock *block,
         perror("ftruncate");
     }
 
-    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
+    area = qemu_ram_mmap(fd, memory, block->mr->align,
+                         block->flags & RAM_SHARED);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
                          "unable to map backing store for guest RAM");
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 408783f..783270f 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -247,6 +247,19 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 
 #endif
 
+#if defined(__linux__) && \
+    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
+   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
+      Valgrind does not support alignments larger than 1 MiB,
+      therefore we need special code which handles running on Valgrind. */
+#  define QEMU_VMALLOC_ALIGN (512 * 4096)
+#elif defined(__linux__) && defined(__s390x__)
+   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
+#  define QEMU_VMALLOC_ALIGN (256 * 4096)
+#else
+#  define QEMU_VMALLOC_ALIGN getpagesize()
+#endif
+
 int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 6cc4b8f..4adde93 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -26,19 +26,6 @@
  * THE SOFTWARE.
  */
 
-#if defined(__linux__) && \
-    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
-   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
-      Valgrind does not support alignments larger than 1 MiB,
-      therefore we need special code which handles running on Valgrind. */
-#  define QEMU_VMALLOC_ALIGN (512 * 4096)
-#elif defined(__linux__) && defined(__s390x__)
-   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
-#  define QEMU_VMALLOC_ALIGN (256 * 4096)
-#else
-#  define QEMU_VMALLOC_ALIGN getpagesize()
-#endif
-
 #include "qemu/osdep.h"
 #include <termios.h>
 #include <termios.h>
-- 
2.6.6

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Fam Zheng]

On Mon, 04/25 13:55, Dominik Dingel wrote:
> While in the anonymous ram case we already take care of the right alignment
> such an alignment gurantee does not exist for file backed ram allocation.

s/gurantee/guarantee/

Otherwise looks good to me,

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
> as we need to satisfy an alignment up to segments.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> 
> ---
> 
> v2 -> v3:
> Skipping additional variable and just use alignment of memory region.
> As memory will not be backpropagated it is enough to round up to page_sizes.
> 
> v1 -> v2:
> While enforcing alignments we allow memory sizes on page_size.
> On mmap the memory size will be round up to alignments.
> 
> I thought about moving this alignment into qemu_ram_mmap but the result
> was a lot of code churn, the other possibility was to create an additional
> define ending up with two defines with the same semantics.
> ---
>  exec.c               |  5 +++--
>  include/qemu/osdep.h | 13 +++++++++++++
>  util/oslib-posix.c   | 13 -------------
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index c4f9036..fc75266 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1296,7 +1296,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  
>      page_size = qemu_fd_getpagesize(fd);
> -    block->mr->align = page_size;
> +    block->mr->align = MAX(page_size, QEMU_VMALLOC_ALIGN);
>  
>      if (memory < page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> @@ -1317,7 +1317,8 @@ static void *file_ram_alloc(RAMBlock *block,
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
> +    area = qemu_ram_mmap(fd, memory, block->mr->align,
> +                         block->flags & RAM_SHARED);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
>                           "unable to map backing store for guest RAM");
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 408783f..783270f 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -247,6 +247,19 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  
>  #endif
>  
> +#if defined(__linux__) && \
> +    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> +   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> +      Valgrind does not support alignments larger than 1 MiB,
> +      therefore we need special code which handles running on Valgrind. */
> +#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> +#elif defined(__linux__) && defined(__s390x__)
> +   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> +#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> +#else
> +#  define QEMU_VMALLOC_ALIGN getpagesize()
> +#endif
> +
>  int qemu_madvise(void *addr, size_t len, int advice);
>  
>  int qemu_open(const char *name, int flags, ...);
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 6cc4b8f..4adde93 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -26,19 +26,6 @@
>   * THE SOFTWARE.
>   */
>  
> -#if defined(__linux__) && \
> -    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> -   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> -      Valgrind does not support alignments larger than 1 MiB,
> -      therefore we need special code which handles running on Valgrind. */
> -#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> -#elif defined(__linux__) && defined(__s390x__)
> -   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> -#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> -#else
> -#  define QEMU_VMALLOC_ALIGN getpagesize()
> -#endif
> -
>  #include "qemu/osdep.h"
>  #include <termios.h>
>  #include <termios.h>
> -- 
> 2.6.6
> 
> 

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Dominik Dingel]

On Fri, 29 Apr 2016 15:32:22 +0800
Fam Zheng <famz@redhat.com> wrote:

> On Mon, 04/25 13:55, Dominik Dingel wrote:
> > While in the anonymous ram case we already take care of the right alignment
> > such an alignment gurantee does not exist for file backed ram allocation.
> 
> s/gurantee/guarantee/
> 
> Otherwise looks good to me,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Thank you very much!

Paolo do you want me to fix the typo, add the r-b and resend the patch?

> > 
> > Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
> > as we need to satisfy an alignment up to segments.
> > 
> > Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> > 
> > ---
> > 
> > v2 -> v3:
> > Skipping additional variable and just use alignment of memory region.
> > As memory will not be backpropagated it is enough to round up to page_sizes.
> > 
> > v1 -> v2:
> > While enforcing alignments we allow memory sizes on page_size.
> > On mmap the memory size will be round up to alignments.
> > 
> > I thought about moving this alignment into qemu_ram_mmap but the result
> > was a lot of code churn, the other possibility was to create an additional
> > define ending up with two defines with the same semantics.
> > ---
> >  exec.c               |  5 +++--
> >  include/qemu/osdep.h | 13 +++++++++++++
> >  util/oslib-posix.c   | 13 -------------
> >  3 files changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index c4f9036..fc75266 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1296,7 +1296,7 @@ static void *file_ram_alloc(RAMBlock *block,
> >      }
> >  
> >      page_size = qemu_fd_getpagesize(fd);
> > -    block->mr->align = page_size;
> > +    block->mr->align = MAX(page_size, QEMU_VMALLOC_ALIGN);
> >  
> >      if (memory < page_size) {
> >          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> > @@ -1317,7 +1317,8 @@ static void *file_ram_alloc(RAMBlock *block,
> >          perror("ftruncate");
> >      }
> >  
> > -    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
> > +    area = qemu_ram_mmap(fd, memory, block->mr->align,
> > +                         block->flags & RAM_SHARED);
> >      if (area == MAP_FAILED) {
> >          error_setg_errno(errp, errno,
> >                           "unable to map backing store for guest RAM");
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 408783f..783270f 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -247,6 +247,19 @@ void qemu_anon_ram_free(void *ptr, size_t size);
> >  
> >  #endif
> >  
> > +#if defined(__linux__) && \
> > +    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> > +   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> > +      Valgrind does not support alignments larger than 1 MiB,
> > +      therefore we need special code which handles running on Valgrind. */
> > +#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> > +#elif defined(__linux__) && defined(__s390x__)
> > +   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> > +#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> > +#else
> > +#  define QEMU_VMALLOC_ALIGN getpagesize()
> > +#endif
> > +
> >  int qemu_madvise(void *addr, size_t len, int advice);
> >  
> >  int qemu_open(const char *name, int flags, ...);
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 6cc4b8f..4adde93 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -26,19 +26,6 @@
> >   * THE SOFTWARE.
> >   */
> >  
> > -#if defined(__linux__) && \
> > -    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> > -   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> > -      Valgrind does not support alignments larger than 1 MiB,
> > -      therefore we need special code which handles running on Valgrind. */
> > -#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> > -#elif defined(__linux__) && defined(__s390x__)
> > -   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> > -#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> > -#else
> > -#  define QEMU_VMALLOC_ALIGN getpagesize()
> > -#endif
> > -
> >  #include "qemu/osdep.h"
> >  #include <termios.h>
> >  #include <termios.h>
> > -- 
> > 2.6.6
> > 
> > 
> 

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Fam Zheng]

On Fri, 04/29 10:26, Dominik Dingel wrote:
> On Fri, 29 Apr 2016 15:32:22 +0800
> Fam Zheng <famz@redhat.com> wrote:
> 
> > On Mon, 04/25 13:55, Dominik Dingel wrote:
> > > While in the anonymous ram case we already take care of the right alignment
> > > such an alignment gurantee does not exist for file backed ram allocation.
> > 
> > s/gurantee/guarantee/
> > 
> > Otherwise looks good to me,
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> Thank you very much!
> 
> Paolo do you want me to fix the typo, add the r-b and resend the patch?

Paolo is still travelling this week, you can do that if you feel like to, or I
guess he can fix it himself when applying later.

Fam

> 
> > > 
> > > Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
> > > as we need to satisfy an alignment up to segments.
> > > 
> > > Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > > Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> > > 
> > > ---
> > > 
> > > v2 -> v3:
> > > Skipping additional variable and just use alignment of memory region.
> > > As memory will not be backpropagated it is enough to round up to page_sizes.
> > > 
> > > v1 -> v2:
> > > While enforcing alignments we allow memory sizes on page_size.
> > > On mmap the memory size will be round up to alignments.
> > > 
> > > I thought about moving this alignment into qemu_ram_mmap but the result
> > > was a lot of code churn, the other possibility was to create an additional
> > > define ending up with two defines with the same semantics.
> > > ---
> > >  exec.c               |  5 +++--
> > >  include/qemu/osdep.h | 13 +++++++++++++
> > >  util/oslib-posix.c   | 13 -------------
> > >  3 files changed, 16 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index c4f9036..fc75266 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1296,7 +1296,7 @@ static void *file_ram_alloc(RAMBlock *block,
> > >      }
> > >  
> > >      page_size = qemu_fd_getpagesize(fd);
> > > -    block->mr->align = page_size;
> > > +    block->mr->align = MAX(page_size, QEMU_VMALLOC_ALIGN);
> > >  
> > >      if (memory < page_size) {
> > >          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> > > @@ -1317,7 +1317,8 @@ static void *file_ram_alloc(RAMBlock *block,
> > >          perror("ftruncate");
> > >      }
> > >  
> > > -    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
> > > +    area = qemu_ram_mmap(fd, memory, block->mr->align,
> > > +                         block->flags & RAM_SHARED);
> > >      if (area == MAP_FAILED) {
> > >          error_setg_errno(errp, errno,
> > >                           "unable to map backing store for guest RAM");
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 408783f..783270f 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -247,6 +247,19 @@ void qemu_anon_ram_free(void *ptr, size_t size);
> > >  
> > >  #endif
> > >  
> > > +#if defined(__linux__) && \
> > > +    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> > > +   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> > > +      Valgrind does not support alignments larger than 1 MiB,
> > > +      therefore we need special code which handles running on Valgrind. */
> > > +#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> > > +#elif defined(__linux__) && defined(__s390x__)
> > > +   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> > > +#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> > > +#else
> > > +#  define QEMU_VMALLOC_ALIGN getpagesize()
> > > +#endif
> > > +
> > >  int qemu_madvise(void *addr, size_t len, int advice);
> > >  
> > >  int qemu_open(const char *name, int flags, ...);
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index 6cc4b8f..4adde93 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -26,19 +26,6 @@
> > >   * THE SOFTWARE.
> > >   */
> > >  
> > > -#if defined(__linux__) && \
> > > -    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> > > -   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> > > -      Valgrind does not support alignments larger than 1 MiB,
> > > -      therefore we need special code which handles running on Valgrind. */
> > > -#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> > > -#elif defined(__linux__) && defined(__s390x__)
> > > -   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> > > -#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> > > -#else
> > > -#  define QEMU_VMALLOC_ALIGN getpagesize()
> > > -#endif
> > > -
> > >  #include "qemu/osdep.h"
> > >  #include <termios.h>
> > >  #include <termios.h>
> > > -- 
> > > 2.6.6
> > > 
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Paolo Bonzini]

On 25/04/2016 13:55, Dominik Dingel wrote:
> While in the anonymous ram case we already take care of the right alignment
> such an alignment gurantee does not exist for file backed ram allocation.
> 
> Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
> as we need to satisfy an alignment up to segments.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>

Looks good now, thanks.

Cc: qemu-stable <qemu-stable@nongnu.org>

Paolo

> ---
> 
> v2 -> v3:
> Skipping additional variable and just use alignment of memory region.
> As memory will not be backpropagated it is enough to round up to page_sizes.
> 
> v1 -> v2:
> While enforcing alignments we allow memory sizes on page_size.
> On mmap the memory size will be round up to alignments.
> 
> I thought about moving this alignment into qemu_ram_mmap but the result
> was a lot of code churn, the other possibility was to create an additional
> define ending up with two defines with the same semantics.
> ---
>  exec.c               |  5 +++--
>  include/qemu/osdep.h | 13 +++++++++++++
>  util/oslib-posix.c   | 13 -------------
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index c4f9036..fc75266 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1296,7 +1296,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  
>      page_size = qemu_fd_getpagesize(fd);
> -    block->mr->align = page_size;
> +    block->mr->align = MAX(page_size, QEMU_VMALLOC_ALIGN);
>  
>      if (memory < page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> @@ -1317,7 +1317,8 @@ static void *file_ram_alloc(RAMBlock *block,
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
> +    area = qemu_ram_mmap(fd, memory, block->mr->align,
> +                         block->flags & RAM_SHARED);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
>                           "unable to map backing store for guest RAM");
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 408783f..783270f 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -247,6 +247,19 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  
>  #endif
>  
> +#if defined(__linux__) && \
> +    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> +   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> +      Valgrind does not support alignments larger than 1 MiB,
> +      therefore we need special code which handles running on Valgrind. */
> +#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> +#elif defined(__linux__) && defined(__s390x__)
> +   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> +#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> +#else
> +#  define QEMU_VMALLOC_ALIGN getpagesize()
> +#endif
> +
>  int qemu_madvise(void *addr, size_t len, int advice);
>  
>  int qemu_open(const char *name, int flags, ...);
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 6cc4b8f..4adde93 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -26,19 +26,6 @@
>   * THE SOFTWARE.
>   */
>  
> -#if defined(__linux__) && \
> -    (defined(__x86_64__) || defined(__arm__) || defined(__aarch64__))
> -   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
> -      Valgrind does not support alignments larger than 1 MiB,
> -      therefore we need special code which handles running on Valgrind. */
> -#  define QEMU_VMALLOC_ALIGN (512 * 4096)
> -#elif defined(__linux__) && defined(__s390x__)
> -   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
> -#  define QEMU_VMALLOC_ALIGN (256 * 4096)
> -#else
> -#  define QEMU_VMALLOC_ALIGN getpagesize()
> -#endif
> -
>  #include "qemu/osdep.h"
>  #include <termios.h>
>  #include <termios.h>
> 

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Paolo Bonzini]

On 29/03/2016 11:29, Dominik Dingel wrote:
>> > Why is this part necessary?  On x86 you can have 1 megabyte of RAM,
>> > but QEMU_VMALLOC_ALIGN is 2MB.
> You are right, I changed this to keep the change consistent and thought 
> the use case of x86 guests with 1 MB RAM might be a little bit obscure.
> 
> Will change this, and keep the page_size check here, and resend a new version
> in the next days.

Thanks, this is obviously okay for hard freeze so take your time.

Paolo

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Dominik Dingel]

On Thu, 24 Mar 2016 12:35:10 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 23/03/2016 22:32, Dominik Dingel wrote:
> > -    page_size = qemu_fd_getpagesize(fd);
> > -    block->mr->align = page_size;
> > +    alignment = MAX(qemu_fd_getpagesize(fd), QEMU_VMALLOC_ALIGN);
> > +    block->mr->align = alignment;
> >  
> > -    if (memory < page_size) {
> > +    if (memory < alignment) {
> >          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> > -                   "or larger than page size 0x%" PRIx64,
> > -                   memory, page_size);
> > +                   "or larger than needed alignment 0x%" PRIx64,
> > +                   memory, alignment);
> >          goto error;
> >      }
> 
> Why is this part necessary?  On x86 you can have 1 megabyte of RAM,
> but QEMU_VMALLOC_ALIGN is 2MB.

You are right, I changed this to keep the change consistent and thought 
the use case of x86 guests with 1 MB RAM might be a little bit obscure.

Will change this, and keep the page_size check here, and resend a new version
in the next days.

Thanks
	Dominik

Re: [Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Paolo Bonzini]

On 23/03/2016 22:32, Dominik Dingel wrote:
> -    page_size = qemu_fd_getpagesize(fd);
> -    block->mr->align = page_size;
> +    alignment = MAX(qemu_fd_getpagesize(fd), QEMU_VMALLOC_ALIGN);
> +    block->mr->align = alignment;
>  
> -    if (memory < page_size) {
> +    if (memory < alignment) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> -                   "or larger than page size 0x%" PRIx64,
> -                   memory, page_size);
> +                   "or larger than needed alignment 0x%" PRIx64,
> +                   memory, alignment);
>          goto error;
>      }

Why is this part necessary?  On x86 you can have 1 megabyte of RAM,
but QEMU_VMALLOC_ALIGN is 2MB.

Paolo

> -    memory = ROUND_UP(memory, page_size);
> +    memory = ROUND_UP(memory, alignment);
>  

[Qemu-devel] [PATCH] exec.c: Ensure right alignment also for file backed ram

[Dominik Dingel]

While in the anonymous ram case we already take care of the right alignment
such an alignment gurantee does not exist for file backed ram allocation.

Instead, pagesize is used for alignment. On s390 this is not enough for gmap,
as we need to satisfy an alignment up to segments.

Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>

---

I thought about moving this alignment into qemu_ram_mmap but the result
was a lot of code churn, the other possibility was to create an additional
define ending up with two defines with the same semantics.

---
 exec.c               | 16 ++++++++--------
 include/qemu/osdep.h | 12 ++++++++++++
 util/oslib-posix.c   | 12 ------------
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/exec.c b/exec.c
index f398d21..2c4583f 100644
--- a/exec.c
+++ b/exec.c
@@ -1239,7 +1239,7 @@ static void *file_ram_alloc(RAMBlock *block,
     char *c;
     void *area;
     int fd;
-    int64_t page_size;
+    int64_t alignment;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_setg(errp,
@@ -1294,17 +1294,17 @@ static void *file_ram_alloc(RAMBlock *block,
          */
     }
 
-    page_size = qemu_fd_getpagesize(fd);
-    block->mr->align = page_size;
+    alignment = MAX(qemu_fd_getpagesize(fd), QEMU_VMALLOC_ALIGN);
+    block->mr->align = alignment;
 
-    if (memory < page_size) {
+    if (memory < alignment) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
-                   "or larger than page size 0x%" PRIx64,
-                   memory, page_size);
+                   "or larger than needed alignment 0x%" PRIx64,
+                   memory, alignment);
         goto error;
     }
 
-    memory = ROUND_UP(memory, page_size);
+    memory = ROUND_UP(memory, alignment);
 
     /*
      * ftruncate is not supported by hugetlbfs in older
@@ -1316,7 +1316,7 @@ static void *file_ram_alloc(RAMBlock *block,
         perror("ftruncate");
     }
 
-    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
+    area = qemu_ram_mmap(fd, memory, alignment, block->flags & RAM_SHARED);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
                          "unable to map backing store for guest RAM");
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 5bb374c..3d81672 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -231,6 +231,18 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 
 #endif
 
+#if defined(__linux__) && (defined(__x86_64__) || defined(__arm__))
+   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
+      Valgrind does not support alignments larger than 1 MiB,
+      therefore we need special code which handles running on Valgrind. */
+#  define QEMU_VMALLOC_ALIGN (512 * 4096)
+#elif defined(__linux__) && defined(__s390x__)
+   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
+#  define QEMU_VMALLOC_ALIGN (256 * 4096)
+#else
+#  define QEMU_VMALLOC_ALIGN getpagesize()
+#endif
+
 int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 05c44ed..52c621f 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -26,18 +26,6 @@
  * THE SOFTWARE.
  */
 
-#if defined(__linux__) && (defined(__x86_64__) || defined(__arm__))
-   /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
-      Valgrind does not support alignments larger than 1 MiB,
-      therefore we need special code which handles running on Valgrind. */
-#  define QEMU_VMALLOC_ALIGN (512 * 4096)
-#elif defined(__linux__) && defined(__s390x__)
-   /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */
-#  define QEMU_VMALLOC_ALIGN (256 * 4096)
-#else
-#  define QEMU_VMALLOC_ALIGN getpagesize()
-#endif
-
 #include "qemu/osdep.h"
 #include <termios.h>
 #include <termios.h>
-- 
2.6.5