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

["Liu, Jingqi" <jingqi.liu@intel.com>]

On 4/7/2020 4:08 PM, Paolo Bonzini wrote:
> On 07/04/20 09:29, Liu, Jingqi wrote:
>> Ping.
>>
>> Any comments are appreciated.
>>
>> Hi Paolo, Richard,
>>
>> Any comments about this ?
> I was hoping to get a review from someone else because I have no way to
> test it.  But I've now queued the patch, thanks.

Hi Paolo,

Thanks for your comments.

Hi Dan,

Can you help review this patch ?

Thanks,

Jingqi

> Paolo
>
>> Thanks,
>>
>> Jingqi
>>
>> On 4/1/2020 11:13 AM, Liu, Jingqi 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 fetches 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'.
>>>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
>>> ---
>>>    exec.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index de9d949902..8221abffec 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -1736,6 +1736,42 @@ 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__)
>>> +    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 *subsystem_path = NULL;
>>> +        g_autofree char *subsystem = NULL;
>>> +
>>> +        subsystem_path =
>>> g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
>>> +                                         major(st.st_rdev),
>>> minor(st.st_rdev));
>>> +        subsystem = g_file_read_link(subsystem_path, NULL);
>>> +
>>> +        if (subsystem && g_str_has_suffix(subsystem, "/dax")) {
>>> +            g_autofree char *align_path = NULL;
>>> +            g_autofree char *align_str = NULL;
>>> +
>>> +            align_path =
>>> g_strdup_printf("/sys/dev/char/%d:%d/device/align",
>>> +                                    major(st.st_rdev),
>>> minor(st.st_rdev));
>>> +
>>> +            if (g_file_get_contents(align_path, &align_str, NULL,
>>> NULL)) {
>>> +                return g_ascii_strtoll(align_str, NULL, 0);
>>> +            }
>>> +        }
>>> +    }
>>> +#endif /* defined(__linux__) */
>>> +
>>> +    return align;
>>> +}
>>> +
>>>    static int file_ram_open(const char *path,
>>>                             const char *region_name,
>>>                             bool *created,
>>> @@ -2275,7 +2311,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 +2347,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;