Re: [Qemu-devel] [PATCH v2 02/42] exec: Fix memory allocation when memory path isn't on hugetlbfs

[Paolo Bonzini]

On 07/03/2016 20:25, Markus Armbruster wrote:
> gethugepagesize() works reliably only when its argument is on
> hugetlbfs.  When it's not, it returns the filesystem's "optimal
> transfer block size", which may or may not be the actual page size
> you'll get when you mmap().
> 
> If the value is too small or not a power of two, we fail
> qemu_ram_mmap()'s assertions.  These were added in commit 794e8f3
> (v2.5.0).  The bug's impact before that is currently unknown.  Seems
> fairly unlikely at least when the normal page size is 4KiB.
> 
> Else, if the value is too large, we align more strictly than
> necessary.
> 
> gethugepagesize() goes back to commit c902760 (v0.13).  That commit
> clearly intended gethugepagesize() to be used on hugetlbfs only.  Not
> only was it named accordingly, it also printed a warning when used on
> anything else.  However, the commit neglected to spell out the
> restriction in user documentation of -mem-path.
> 
> Commit bfc2a1a (v2.5.0) dropped the warning as bogus "because QEMU
> functions perfectly well with the path on a regular tmpfs filesystem".
> It sure does when you're sufficiently lucky.  In my testing, I was
> lucky, too.
> 
> Fix by switching to qemu_fd_getpagesize().  Rename the variable
> holding its result from hpagesize to page_size.
> 
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  exec.c | 40 +++++++---------------------------------
>  1 file changed, 7 insertions(+), 33 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 5275ff4..d41194e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1207,27 +1207,6 @@ void qemu_mutex_unlock_ramlist(void)
>  }
>  
>  #ifdef __linux__
> -
> -#include <sys/vfs.h>
> -
> -#define HUGETLBFS_MAGIC       0x958458f6
> -
> -static long gethugepagesize(int fd)
> -{
> -    struct statfs fs;
> -    int ret;
> -
> -    do {
> -        ret = fstatfs(fd, &fs);
> -    } while (ret != 0 && errno == EINTR);
> -
> -    if (ret != 0) {
> -        return -1;
> -    }
> -
> -    return fs.f_bsize;
> -}
> -
>  static void *file_ram_alloc(RAMBlock *block,
>                              ram_addr_t memory,
>                              const char *path,
> @@ -1239,7 +1218,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      char *c;
>      void *area;
>      int fd;
> -    int64_t hpagesize;
> +    int64_t page_size;
>  
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>          error_setg(errp,
> @@ -1294,22 +1273,17 @@ static void *file_ram_alloc(RAMBlock *block,
>           */
>      }
>  
> -    hpagesize = gethugepagesize(fd);
> -    if (hpagesize < 0) {
> -        error_setg_errno(errp, errno, "can't get page size for %s",
> -                         path);
> -        goto error;
> -    }
> -    block->mr->align = hpagesize;
> +    page_size = qemu_fd_getpagesize(fd);
> +    block->mr->align = page_size;
>  
> -    if (memory < hpagesize) {
> +    if (memory < page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>                     "or larger than page size 0x%" PRIx64,
> -                   memory, hpagesize);
> +                   memory, page_size);
>          goto error;
>      }
>  
> -    memory = ROUND_UP(memory, hpagesize);
> +    memory = ROUND_UP(memory, page_size);
>  
>      /*
>       * ftruncate is not supported by hugetlbfs in older
> @@ -1321,7 +1295,7 @@ static void *file_ram_alloc(RAMBlock *block,
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & RAM_SHARED);
> +    area = qemu_ram_mmap(fd, memory, page_size, block->flags & RAM_SHARED);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
>                           "unable to map backing store for guest RAM");
> 

Queued, thanks.

Paolo