On Wed, 13 Feb 2019 16:53:54 +0800 Wei Yang <richardw.y...@linux.intel.com> wrote:
> On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote: > >Guests started with NVDIMMs larger than the underlying host file produce > >confusing errors inside the guest. This happens because the guest > >accesses pages beyond the end of the file. > > > >Check the pmem file size on startup and print a clear error message if > >the size is invalid. > > > >Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053 > >Cc: Haozhong Zhang <haozhong.zh...@intel.com> > >Cc: Zhang Yi <yi.z.zh...@linux.intel.com> > >Cc: Eduardo Habkost <ehabk...@redhat.com> > >Cc: Igor Mammedov <imamm...@redhat.com> > >Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > >--- > >v2: > > * Propagate qemu_get_pmem_size() errors [Igor] > > > > include/qemu/osdep.h | 13 ++++++++++ > > backends/hostmem-file.c | 22 +++++++++++++++++ > > util/oslib-posix.c | 53 +++++++++++++++++++++++++++++++++++++++++ > > util/oslib-win32.c | 5 ++++ > > 4 files changed, 93 insertions(+) > > > >diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > >index 840af09cb0..303d315c5d 100644 > >--- a/include/qemu/osdep.h > >+++ b/include/qemu/osdep.h > >@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo); > > void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus, > > Error **errp); > > > >+/** > >+ * qemu_get_pmem_size: > >+ * @filename: path to a pmem file > >+ * @errp: pointer to a NULL-initialized error object > >+ * > >+ * Determine the size of a persistent memory file. Besides supporting > >files on > >+ * DAX file systems, this function also supports Linux devdax character > >+ * devices. > >+ * > >+ * Returns: the size or 0 on failure > >+ */ > >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp); > >+ > > /** > > * qemu_get_pid_name: > > * @pid: pid of a process > >diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c > >index ba601ce940..d62689179b 100644 > >--- a/backends/hostmem-file.c > >+++ b/backends/hostmem-file.c > >@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, > >Error **errp) > > gchar *name; > > #endif > > > >+ /* > >+ * Verify pmem file size since starting a guest with an incorrect size > >+ * leads to confusing failures inside the guest. > >+ */ > >+ if (fb->is_pmem && fb->mem_path) { > >+ Error *local_err = NULL; > >+ uint64_t size; > >+ > >+ size = qemu_get_pmem_size(fb->mem_path, &local_err); > >+ if (!size) { > >+ error_propagate(errp, local_err); > >+ return; > >+ } > >+ > >+ if (backend->size > size) { > >+ error_setg(errp, "size property %" PRIu64 " is larger than " > >+ "pmem file \"%s\" size %" PRIu64, backend->size, > >+ fb->mem_path, size); > >+ return; > >+ } > >+ } > >+ > > -mem-path is only supported on POSIX host. > > So how about put this in the last code block before doing the real memory > region init? Indeed, evaluating file would be better done only if we didn't fail size/mempath checks first. > BTW, I suggest to cleanup file_backend_memory_alloc() to make POSIX/non-POSIX > situation clear to understand. that's a separate patch, I'll reply with it here so Sefan could pick it (due to conflicts the change would create) > > > if (!backend->size) { > > error_setg(errp, "can't create backend with size 0"); > > return; > >diff --git a/util/oslib-posix.c b/util/oslib-posix.c > >index 37c5854b9c..10d90d1783 100644 > >--- a/util/oslib-posix.c > >+++ b/util/oslib-posix.c > >@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, > >int smp_cpus, > > } > > } > > > >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp) > >+{ > >+ struct stat st; > >+ > >+ if (stat(filename, &st) < 0) { > >+ error_setg(errp, "unable to stat pmem file \"%s\"", filename); > >+ return 0; > >+ } > >+ > >+#if defined(__linux__) > >+ /* Special handling for devdax character devices */ > >+ if (S_ISCHR(st.st_mode)) { > >+ char *subsystem_path = NULL; > >+ char *subsystem = NULL; > >+ char *size_path = NULL; > >+ char *size_str = NULL; > >+ uint64_t ret = 0; > >+ > >+ 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) { > >+ error_setg(errp, "unable to read subsystem for pmem file > >\"%s\"", > >+ filename); > >+ goto devdax_err; > >+ } > >+ > >+ if (!g_str_has_suffix(subsystem, "/dax")) { > >+ error_setg(errp, "pmem file \"%s\" is not a dax device", > >filename); > >+ goto devdax_err; > >+ } > >+ > >+ size_path = g_strdup_printf("/sys/dev/char/%d:%d/size", > >+ major(st.st_rdev), minor(st.st_rdev)); > >+ if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) { > >+ error_setg(errp, "unable to read size for pmem file \"%s\"", > >+ size_path); > >+ goto devdax_err; > >+ } > >+ > >+ ret = g_ascii_strtoull(size_str, NULL, 0); > >+ > >+devdax_err: > >+ g_free(size_str); > >+ g_free(size_path); > >+ g_free(subsystem); > >+ g_free(subsystem_path); > >+ return ret; > >+ } > >+#endif /* defined(__linux__) */ > >+ > >+ return st.st_size; > >+} > > > > char *qemu_get_pid_name(pid_t pid) > > { > >diff --git a/util/oslib-win32.c b/util/oslib-win32.c > >index b4c17f5dfa..bd633afab6 100644 > >--- a/util/oslib-win32.c > >+++ b/util/oslib-win32.c > >@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, > >int smp_cpus, > > } > > } > > > >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp) > >+{ > >+ error_setg(errp, "pmem support not available"); > >+ return 0; > >+} > > > > char *qemu_get_pid_name(pid_t pid) > > { > >-- > >2.20.1 > > >
