On Tue, Apr 7, 2020 at 1:08 AM Paolo Bonzini <pbonz...@redhat.com> 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.
Does qemu run tests in a nested VM? The difficult aspect of testing
devdax is that you need to boot your kernel with a special option or
have existing memory ranges assigned to the device. Although, Joao had
thoughts about allowing dynamic creation of device-dax instance by hot
unplugging memory.
>
> 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.willi...@intel.com>
> >> Signed-off-by: Jingqi Liu <jingqi....@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;
Is there any downside to just making the alignment value be the max of
the device-dax instance align and the command line option? Why force
someone to debug the option unnecessarily?
