Re: [PATCH v2 1/3] exec: fetch the alignment of Linux devdax pmem character device nodes

[Joao Martins]

On 4/15/20 4:35 AM, Jingqi Liu wrote:
> If the backend file is devdax pmem character device, the alignment
> specified by the option 'align=NUM' in the '-object memory-backend-file'
> needs to match the alignment requirement of the devdax pmem character device.
> 
> This patch uses the interfaces of libdaxctl to fetch the devdax pmem file
> 'align', so that we can compare it with the NUM of 'align=NUM'.
> The NUM needs to be larger than or equal to the devdax pmem file 'align'.
> 
> It also fixes the problem that mmap() returns failure in qemu_ram_mmap()
> when the NUM of 'align=NUM' is less than the devdax pmem file 'align'.
> 
> Suggested-by: Dan Williams <address@hidden>
> Signed-off-by: Jingqi Liu <address@hidden>

  Reviewed-by: Joao Martins <address@hidden>

I also no longer see the previous problem we talked in v1 wrt to ABI differences
between dax_pmem_compat (your case) vs dax_pmem|dax_hmem (my case). The @align
validation also occurs as expected.

> ---
>  exec.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index de9d949902..2c3444e47e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -77,6 +77,10 @@
>  
>  #include "monitor/monitor.h"
>  
> +#ifdef CONFIG_LIBDAXCTL
> +#include <daxctl/libdaxctl.h>
> +#endif
> +
>  //#define DEBUG_SUBPAGE
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -1736,6 +1740,46 @@ static int64_t get_file_size(int fd)
>      return size;
>  }
>  
> +static int64_t get_file_align(int fd)
> +{
> +    int64_t align = -1;
> +#if defined(__linux__) && defined(CONFIG_LIBDAXCTL)
> +    struct stat st;
> +
> +    if (fstat(fd, &st) < 0) {
> +        return -errno;
> +    }
> +
> +    /* Special handling for devdax character devices */
> +    if (S_ISCHR(st.st_mode)) {
> +        g_autofree char *path = NULL;
> +        g_autofree char *rpath = NULL;
> +        struct daxctl_ctx *ctx;
> +        struct daxctl_region *region;
> +        int rc = 0;
> +
> +        path = g_strdup_printf("/sys/dev/char/%d:%d",
> +                    major(st.st_rdev), minor(st.st_rdev));
> +        rpath = realpath(path, NULL);
> +
> +        rc = daxctl_new(&ctx);
> +        if (rc) {
> +            return -1;
> +        }
> +
> +        daxctl_region_foreach(ctx, region) {
> +            if (strstr(rpath, daxctl_region_get_path(region))) {
> +                align = daxctl_region_get_align(region);
> +                break;
> +            }
> +        }
> +        daxctl_unref(ctx);
> +    }
> +#endif /* defined(__linux__) && defined(CONFIG_LIBDAXCTL) */
> +
> +    return align;
> +}
> +
>  static int file_ram_open(const char *path,
>                           const char *region_name,
>                           bool *created,
> @@ -2275,7 +2319,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>  {
>      RAMBlock *new_block;
>      Error *local_err = NULL;
> -    int64_t file_size;
> +    int64_t file_size, file_align;
>  
>      /* Just support these ram flags by now. */
>      assert((ram_flags & ~(RAM_SHARED | RAM_PMEM)) == 0);
> @@ -2311,6 +2355,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>          return NULL;
>      }
>  
> +    file_align = get_file_align(fd);
> +    if (file_align > 0 && mr && file_align > mr->align) {
> +        error_setg(errp, "backing store align 0x%" PRIx64
> +                   " is larger than 'align' option 0x" RAM_ADDR_FMT,
> +                   file_align, mr->align);
> +        return NULL;
> +    }
> +
>      new_block = g_malloc0(sizeof(*new_block));
>      new_block->mr = mr;
>      new_block->used_length = size;
>