[PATCH 0/7] kvmtool: Cleanup kernel loading

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Andre Przywara]

Hi,

this series cleans up kvmtool's kernel loading functionality a bit.
It has been broken out of a previous series I sent [1] and contains
just the cleanup and bug fix parts, which should be less controversial
and thus easier to merge ;-)
I will resend the pipe loading part later on as a separate series.

The first patch properly abstracts kernel loading to move
responsibility into each architecture's code. It removes quite some
ugly code from the generic kvm.c file.
The later patches address the naive usage of read(2) to, well, read
data from files. Doing this without coping with the subtleties of
the UNIX read semantics (returning with less or none data read is not
an error) can provoke hard to debug failures.
So these patches make use of the existing and one new wrapper function
to make sure we read everything we actually wanted to.
The last patch moves the ARM kernel loading code into the proper
location to be in line with the other architectures.

Please have a look and give some comments!

Find the branch on my kvmtool git tree on:
git://linux-arm.org/kvmtool.git (kern_load-v2 branch)
http://www.linux-arm.org/git?p=kvmtool.git;a=shortlog;h=refs/heads/kern_load-v2

Cheers,
Andre.

[1] http://marc.info/?l=kvm&m=143825354808135&w=2

Andre Przywara (7):
  Refactor kernel image loading
  provide generic read_file() implementation
  powerpc: use read_file() in kernel and initrd loading
  MIPS: use read wrappers in kernel loading
  x86: use read wrappers in kernel loading
  arm/arm64: use read_file() in kernel and initrd loading
  arm: move kernel loading into arm/kvm.c

 arm/fdt.c                | 99 +-----------------------------------------------
 arm/kvm.c                | 88 ++++++++++++++++++++++++++++++++++++++++++
 include/kvm/kvm.h        |  5 +--
 include/kvm/read-write.h |  2 +
 kvm.c                    | 42 ++------------------
 mips/kvm.c               | 57 ++++++++++++++++++----------
 powerpc/kvm.c            | 39 ++++++++++---------
 util/read-write.c        | 21 ++++++++++
 x86/kvm.c                | 62 +++++++++++++++---------------
 9 files changed, 207 insertions(+), 208 deletions(-)

-- 
2.5.1

[PATCH 1/7] Refactor kernel image loading

[Andre Przywara]

Let's face it: Kernel loading is quite architecture specific. Don't
claim otherwise and move the loading routines into each
architecture's responsibility.
This introduces kvm__arch_load_kernel(), which each architecture can
implement accordingly.
Provide bzImage loading for x86 and ELF loading for MIPS as special
cases for those architectures (removing the arch specific code from
the generic kvm.c file on the way) and rename the existing "flat binary"
loader functions for the other architectures to the new name.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c         |  4 ++--
 include/kvm/kvm.h |  5 ++---
 kvm.c             | 42 ++++--------------------------------------
 mips/kvm.c        | 23 +++++++++++++++++++----
 powerpc/kvm.c     |  3 ++-
 x86/kvm.c         | 27 +++++++++++++++++----------
 6 files changed, 46 insertions(+), 58 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 3657108..ec7453f 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -239,8 +239,8 @@ static int read_image(int fd, void **pos, void *limit)
 
 #define FDT_ALIGN	SZ_2M
 #define INITRD_ALIGN	4
-int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd,
-		     const char *kernel_cmdline)
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline)
 {
 	void *pos, *kernel_end, *limit;
 	unsigned long guest_addr;
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 37155db..055a7a2 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -111,9 +111,8 @@ void kvm__arch_read_term(struct kvm *kvm);
 void *guest_flat_to_host(struct kvm *kvm, u64 offset);
 u64 host_to_guest_flat(struct kvm *kvm, void *ptr);
 
-int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline);
-int load_elf_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline);
-bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline);
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline);
 
 /*
  * Debugging
diff --git a/kvm.c b/kvm.c
index 10ed230..ca7dfee 100644
--- a/kvm.c
+++ b/kvm.c
@@ -341,18 +341,6 @@ static bool initrd_check(int fd)
 		!memcmp(id, CPIO_MAGIC, 4);
 }
 
-int __attribute__((__weak__)) load_elf_binary(struct kvm *kvm, int fd_kernel,
-				int fd_initrd, const char *kernel_cmdline)
-{
-	return false;
-}
-
-bool __attribute__((__weak__)) load_bzimage(struct kvm *kvm, int fd_kernel,
-				int fd_initrd, const char *kernel_cmdline)
-{
-	return false;
-}
-
 bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 		const char *initrd_filename, const char *kernel_cmdline)
 {
@@ -372,40 +360,18 @@ bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 			die("%s is not an initrd", initrd_filename);
 	}
 
-#ifdef CONFIG_X86
-	ret = load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline);
-
-	if (ret)
-		goto found_kernel;
-
-	pr_warning("%s is not a bzImage. Trying to load it as a flat binary...", kernel_filename);
-#endif
-
-	ret = load_elf_binary(kvm, fd_kernel, fd_initrd, kernel_cmdline);
-
-	if (ret)
-		goto found_kernel;
-
-	ret = load_flat_binary(kvm, fd_kernel, fd_initrd, kernel_cmdline);
-
-	if (ret)
-		goto found_kernel;
+	ret = kvm__arch_load_kernel_image(kvm, fd_kernel, fd_initrd,
+					  kernel_cmdline);
 
 	if (initrd_filename)
 		close(fd_initrd);
 	close(fd_kernel);
 
-	die("%s is not a valid bzImage or flat binary", kernel_filename);
-
-found_kernel:
-	if (initrd_filename)
-		close(fd_initrd);
-	close(fd_kernel);
-
+	if (!ret)
+		die("%s is not a valid kernel image", kernel_filename);
 	return ret;
 }
 
-
 void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size, int debug_fd)
 {
 	unsigned char *p;
diff --git a/mips/kvm.c b/mips/kvm.c
index 1925f38..c1c596c 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -163,7 +163,8 @@ static void kvm__mips_install_cmdline(struct kvm *kvm)
 
 /* Load at the 1M point. */
 #define KERNEL_LOAD_ADDR 0x1000000
-int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline)
+
+static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
 {
 	void *p;
 	void *k_start;
@@ -281,7 +282,7 @@ static bool kvm__arch_get_elf_32_info(Elf32_Ehdr *ehdr, int fd_kernel,
 	return true;
 }
 
-int load_elf_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline)
+static bool load_elf_binary(struct kvm *kvm, int fd_kernel)
 {
 	union {
 		Elf64_Ehdr ehdr;
@@ -342,11 +343,25 @@ int load_elf_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *k
 		ei.len -= nr;
 	} while (ei.len);
 
-	kvm__mips_install_cmdline(kvm);
-
 	return true;
 }
 
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline)
+{
+	if (fd_initrd != -1) {
+		pr_err("Initrd not supported on MIPS.");
+		return false;
+	}
+
+	if (load_elf_binary(kvm, fd_kernel)) {
+		kvm__mips_install_cmdline(kvm);
+		return true;
+	}
+
+	return load_flat_binary(kvm, fd_kernel);
+}
+
 void ioport__map_irq(u8 *irq)
 {
 }
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index b4c3310..13bba30 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -157,7 +157,8 @@ void kvm__arch_read_term(struct kvm *kvm)
 	spapr_hvcons_poll(kvm);
 }
 
-int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline)
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline)
 {
 	void *p;
 	void *k_start;
diff --git a/x86/kvm.c b/x86/kvm.c
index 512ad67..a0204b8 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -206,18 +206,11 @@ static inline void *guest_real_to_host(struct kvm *kvm, u16 selector, u16 offset
 	return guest_flat_to_host(kvm, flat);
 }
 
-int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *kernel_cmdline)
+static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
 {
 	void *p;
 	int nr;
 
-	/*
-	 * Some architectures may support loading an initrd alongside the flat kernel,
-	 * but we do not.
-	 */
-	if (fd_initrd != -1)
-		pr_warning("Loading initrd with flat binary not supported.");
-
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
 
@@ -235,8 +228,8 @@ int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const char *
 
 static const char *BZIMAGE_MAGIC = "HdrS";
 
-bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
-		  const char *kernel_cmdline)
+static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
+			 const char *kernel_cmdline)
 {
 	struct boot_params *kern_boot;
 	unsigned long setup_sects;
@@ -352,6 +345,20 @@ bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 	return true;
 }
 
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline)
+{
+	if (load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline))
+		return true;
+	pr_warning("Kernel image is not a bzImage.");
+	pr_warning("Trying to load it as a flat binary (no cmdline support)");
+
+	if (fd_initrd != -1)
+		pr_warning("Loading initrd with flat binary not supported.");
+
+	return load_flat_binary(kvm, fd_kernel);
+}
+
 /**
  * kvm__arch_setup_firmware - inject BIOS into guest system memory
  * @kvm - guest system descriptor
-- 
2.5.1

[PATCH 2/7] provide generic read_file() implementation

[Andre Przywara]

In various parts of kvmtool we simply try to read files into memory,
but fail to do so in a safe way. The read(2) syscall can return early
having only parts of the file read, or it may return -1 due to being
interrupted by a signal (in which case we should simply retry).
The ARM code seems to provide the only safe implementation, so take
that as an inspiration to provide a generic read_file() function
usable by every part of kvmtool.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/read-write.h |  2 ++
 util/read-write.c        | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/kvm/read-write.h b/include/kvm/read-write.h
index 67571f9..acbd6f0 100644
--- a/include/kvm/read-write.h
+++ b/include/kvm/read-write.h
@@ -12,6 +12,8 @@
 ssize_t xread(int fd, void *buf, size_t count);
 ssize_t xwrite(int fd, const void *buf, size_t count);
 
+ssize_t read_file(int fd, char *buf, size_t max_size);
+
 ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 
diff --git a/util/read-write.c b/util/read-write.c
index 44709df..bf6fb2f 100644
--- a/util/read-write.c
+++ b/util/read-write.c
@@ -32,6 +32,27 @@ restart:
 	return nr;
 }
 
+/*
+ * Read in the whole file while not exceeding max_size bytes of the buffer.
+ * Returns -1 (with errno set) in case of an error (ENOMEM if buffer was
+ * too small) or the filesize if the whole file could be read.
+ */
+ssize_t read_file(int fd, char *buf, size_t max_size)
+{
+	ssize_t ret;
+	char dummy;
+
+	errno = 0;
+	ret = read_in_full(fd, buf, max_size);
+
+	/* Probe whether we reached EOF. */
+	if (xread(fd, &dummy, 1) == 0)
+		return ret;
+
+	errno = ENOMEM;
+	return -1;
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	ssize_t total = 0;
-- 
2.5.1

[PATCH 3/7] powerpc: use read_file() in kernel and initrd loading

[Andre Przywara]

Replace the unsafe read-loops in the powerpc kernel image loading
function with our new and safe read_file() wrapper.
This should fix random fails in kernel image loading, especially
from pipes and sockets.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 powerpc/kvm.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 13bba30..2b0bddd 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -162,19 +162,22 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 {
 	void *p;
 	void *k_start;
-	void *i_start;
-	int nr;
+	ssize_t filesize;
 
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
 
 	p = k_start = guest_flat_to_host(kvm, KERNEL_LOAD_ADDR);
 
-	while ((nr = read(fd_kernel, p, 65536)) > 0)
-		p += nr;
-
-	pr_info("Loaded kernel to 0x%x (%ld bytes)", KERNEL_LOAD_ADDR, (long)(p-k_start));
+	filesize = read_file(fd_kernel, p, INITRD_LOAD_ADDR - KERNEL_LOAD_ADDR);
+	if (filesize < 0) {
+		if (errno == ENOMEM)
+			die("Kernel overlaps initrd!");
 
+		die_perror("kernel read");
+	}
+	pr_info("Loaded kernel to 0x%x (%ld bytes)", KERNEL_LOAD_ADDR,
+		filesize);
 	if (fd_initrd != -1) {
 		if (lseek(fd_initrd, 0, SEEK_SET) < 0)
 			die_perror("lseek");
@@ -183,19 +186,20 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 			die("Kernel overlaps initrd!");
 
 		/* Round up kernel size to 8byte alignment, and load initrd right after. */
-		i_start = p = guest_flat_to_host(kvm, INITRD_LOAD_ADDR);
-
-		while (((nr = read(fd_initrd, p, 65536)) > 0) &&
-		       p < (kvm->ram_start + kvm->ram_size))
-			p += nr;
-
-		if (p >= (kvm->ram_start + kvm->ram_size))
-			die("initrd too big to contain in guest RAM.\n");
+		p = guest_flat_to_host(kvm, INITRD_LOAD_ADDR);
+
+		filesize = read_file(fd_initrd, p,
+			       (kvm->ram_start + kvm->ram_size) - p);
+		if (filesize < 0) {
+			if (errno == ENOMEM)
+				die("initrd too big to contain in guest RAM.\n");
+			die_perror("initrd read");
+		}
 
 		pr_info("Loaded initrd to 0x%x (%ld bytes)",
-			INITRD_LOAD_ADDR, (long)(p-i_start));
+			INITRD_LOAD_ADDR, filesize);
 		kvm->arch.initrd_gra = INITRD_LOAD_ADDR;
-		kvm->arch.initrd_size = p-i_start;
+		kvm->arch.initrd_size = filesize;
 	} else {
 		kvm->arch.initrd_size = 0;
 	}
-- 
2.5.1

[PATCH 4/7] MIPS: use read wrappers in kernel loading

[Andre Przywara]

Replace the unsafe read-loops used in the MIPS kernel image loading
with our safe read_file() and read_in_full() wrappers.
This should fix random fails in kernel image loading, especially
from pipes and sockets.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 mips/kvm.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/mips/kvm.c b/mips/kvm.c
index c1c596c..8fbf8de 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -168,20 +168,27 @@ static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
 {
 	void *p;
 	void *k_start;
-	int nr;
+	ssize_t kernel_size;
 
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
 
 	p = k_start = guest_flat_to_host(kvm, KERNEL_LOAD_ADDR);
 
-	while ((nr = read(fd_kernel, p, 65536)) > 0)
-		p += nr;
+	kernel_size = read_file(fd_kernel, p,
+				kvm->cfg.ram_size - KERNEL_LOAD_ADDR);
+	if (kernel_size == -1) {
+		if (errno == ENOMEM)
+			die("kernel too big for guest memory");
+		else
+			die_perror("kernel read");
+	}
 
 	kvm->arch.is64bit = true;
 	kvm->arch.entry_point = 0xffffffff81000000ull;
 
-	pr_info("Loaded kernel to 0x%x (%ld bytes)", KERNEL_LOAD_ADDR, (long int)(p - k_start));
+	pr_info("Loaded kernel to 0x%x (%zd bytes)", KERNEL_LOAD_ADDR,
+		kernel_size);
 
 	return true;
 }
@@ -197,7 +204,6 @@ static bool kvm__arch_get_elf_64_info(Elf64_Ehdr *ehdr, int fd_kernel,
 				      struct kvm__arch_elf_info *ei)
 {
 	int i;
-	size_t nr;
 	Elf64_Phdr phdr;
 
 	if (ehdr->e_phentsize != sizeof(phdr)) {
@@ -212,8 +218,7 @@ static bool kvm__arch_get_elf_64_info(Elf64_Ehdr *ehdr, int fd_kernel,
 
 	phdr.p_type = PT_NULL;
 	for (i = 0; i < ehdr->e_phnum; i++) {
-		nr = read(fd_kernel, &phdr, sizeof(phdr));
-		if (nr != sizeof(phdr)) {
+		if (read_in_full(fd_kernel, &phdr, sizeof(phdr)) != sizeof(phdr)) {
 			pr_info("Couldn't read %d bytes for ELF PHDR.", (int)sizeof(phdr));
 			return false;
 		}
@@ -243,7 +248,6 @@ static bool kvm__arch_get_elf_32_info(Elf32_Ehdr *ehdr, int fd_kernel,
 				      struct kvm__arch_elf_info *ei)
 {
 	int i;
-	size_t nr;
 	Elf32_Phdr phdr;
 
 	if (ehdr->e_phentsize != sizeof(phdr)) {
@@ -258,8 +262,7 @@ static bool kvm__arch_get_elf_32_info(Elf32_Ehdr *ehdr, int fd_kernel,
 
 	phdr.p_type = PT_NULL;
 	for (i = 0; i < ehdr->e_phnum; i++) {
-		nr = read(fd_kernel, &phdr, sizeof(phdr));
-		if (nr != sizeof(phdr)) {
+		if (read_in_full(fd_kernel, &phdr, sizeof(phdr)) != sizeof(phdr)) {
 			pr_info("Couldn't read %d bytes for ELF PHDR.", (int)sizeof(phdr));
 			return false;
 		}
@@ -334,14 +337,11 @@ static bool load_elf_binary(struct kvm *kvm, int fd_kernel)
 	p = guest_flat_to_host(kvm, ei.load_addr);
 
 	pr_info("ELF Loading 0x%lx bytes from 0x%llx to 0x%llx",
-		(unsigned long)ei.len, (unsigned long long)ei.offset, (unsigned long long)ei.load_addr);
-	do {
-		nr = read(fd_kernel, p, ei.len);
-		if (nr < 0)
-			die_perror("read");
-		p += nr;
-		ei.len -= nr;
-	} while (ei.len);
+		(unsigned long)ei.len, (unsigned long long)ei.offset,
+		(unsigned long long)ei.load_addr);
+
+	if (read_in_full(fd_kernel, p, ei.len) != (ssize_t)ei.len)
+		die_perror("read");
 
 	return true;
 }
-- 
2.5.1

[PATCH 5/7] x86: use read wrappers in kernel loading

[Andre Przywara]

Replace the unsafe read-loops in the x86 kernel image loading
functions with our safe read_file() and read_in_full() wrappers.
This should fix random fails in kernel image loading, especially
from pipes and sockets.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 x86/kvm.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/x86/kvm.c b/x86/kvm.c
index a0204b8..ae430a0 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -9,6 +9,7 @@
 
 #include <asm/bootparam.h>
 #include <linux/kvm.h>
+#include <linux/kernel.h>
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
@@ -209,15 +210,14 @@ static inline void *guest_real_to_host(struct kvm *kvm, u16 selector, u16 offset
 static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
 {
 	void *p;
-	int nr;
 
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
 
 	p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
 
-	while ((nr = read(fd_kernel, p, 65536)) > 0)
-		p += nr;
+	if (read_file(fd_kernel, p, kvm->cfg.ram_size) < 0)
+		die_perror("read");
 
 	kvm->arch.boot_selector	= BOOT_LOADER_SELECTOR;
 	kvm->arch.boot_ip	= BOOT_LOADER_IP;
@@ -232,12 +232,10 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 			 const char *kernel_cmdline)
 {
 	struct boot_params *kern_boot;
-	unsigned long setup_sects;
 	struct boot_params boot;
 	size_t cmdline_size;
-	ssize_t setup_size;
+	ssize_t file_size;
 	void *p;
-	int nr;
 	u16 vidmode;
 
 	/*
@@ -248,7 +246,7 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
 
-	if (read(fd_kernel, &boot, sizeof(boot)) != sizeof(boot))
+	if (read_in_full(fd_kernel, &boot, sizeof(boot)) != sizeof(boot))
 		return false;
 
 	if (memcmp(&boot.hdr.header, BZIMAGE_MAGIC, strlen(BZIMAGE_MAGIC)))
@@ -262,20 +260,17 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 
 	if (!boot.hdr.setup_sects)
 		boot.hdr.setup_sects = BZ_DEFAULT_SETUP_SECTS;
-	setup_sects = boot.hdr.setup_sects + 1;
-
-	setup_size = setup_sects << 9;
+	file_size = (boot.hdr.setup_sects + 1) << 9;
 	p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
+	if (read_in_full(fd_kernel, p, file_size) != file_size)
+		die_perror("kernel setup read");
 
-	/* copy setup.bin to mem*/
-	if (read(fd_kernel, p, setup_size) != setup_size)
-		die_perror("read");
-
-	/* copy vmlinux.bin to BZ_KERNEL_START*/
+	/* read actual kernel image (vmlinux.bin) to BZ_KERNEL_START */
 	p = guest_flat_to_host(kvm, BZ_KERNEL_START);
-
-	while ((nr = read(fd_kernel, p, 65536)) > 0)
-		p += nr;
+	file_size = read_file(fd_kernel, p,
+			      kvm->cfg.ram_size - BZ_KERNEL_START);
+	if (file_size < 0)
+		die_perror("kernel read");
 
 	p = guest_flat_to_host(kvm, BOOT_CMDLINE_OFFSET);
 	if (kernel_cmdline) {
@@ -287,7 +282,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 		memcpy(p, kernel_cmdline, cmdline_size - 1);
 	}
 
-
 	/* vidmode should be either specified or set by default */
 	if (kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk) {
 		if (!kvm->cfg.arch.vidmode)
@@ -326,8 +320,7 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
 		}
 
 		p = guest_flat_to_host(kvm, addr);
-		nr = read(fd_initrd, p, initrd_stat.st_size);
-		if (nr != initrd_stat.st_size)
+		if (read_in_full(fd_initrd, p, initrd_stat.st_size) < 0)
 			die("Failed to read initrd");
 
 		kern_boot->hdr.ramdisk_image	= addr;
-- 
2.5.1

[PATCH 6/7] arm/arm64: use read_file() in kernel and initrd loading

[Andre Przywara]

Use the new read_file() wrapper in our arm/arm64 kernel image loading
function instead of the private implementation.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index ec7453f..19d7ed9 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -224,19 +224,6 @@ static int setup_fdt(struct kvm *kvm)
 }
 late_init(setup_fdt);
 
-static int read_image(int fd, void **pos, void *limit)
-{
-	int count;
-
-	while (((count = xread(fd, *pos, SZ_64K)) > 0) && *pos <= limit)
-		*pos += count;
-
-	if (pos < 0)
-		die_perror("xread");
-
-	return *pos < limit ? 0 : -ENOMEM;
-}
-
 #define FDT_ALIGN	SZ_2M
 #define INITRD_ALIGN	4
 bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
@@ -244,6 +231,7 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 {
 	void *pos, *kernel_end, *limit;
 	unsigned long guest_addr;
+	ssize_t file_size;
 
 	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
 		die_perror("lseek");
@@ -256,13 +244,16 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 
 	pos = kvm->ram_start + ARM_KERN_OFFSET(kvm);
 	kvm->arch.kern_guest_start = host_to_guest_flat(kvm, pos);
-	if (read_image(fd_kernel, &pos, limit) == -ENOMEM)
-		die("kernel image too big to contain in guest memory.");
+	file_size = read_file(fd_kernel, pos, limit - pos);
+	if (file_size < 0) {
+		if (errno == ENOMEM)
+			die("kernel image too big to contain in guest memory.");
 
-	kernel_end = pos;
-	pr_info("Loaded kernel to 0x%llx (%llu bytes)",
-		kvm->arch.kern_guest_start,
-		host_to_guest_flat(kvm, pos) - kvm->arch.kern_guest_start);
+		die_perror("kernel read");
+	}
+	kernel_end = pos + file_size;
+	pr_info("Loaded kernel to 0x%llx (%zd bytes)",
+		kvm->arch.kern_guest_start, file_size);
 
 	/*
 	 * Now load backwards from the end of memory so the kernel
@@ -300,11 +291,16 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 			die("initrd overlaps with kernel image.");
 
 		initrd_start = guest_addr;
-		if (read_image(fd_initrd, &pos, limit) == -ENOMEM)
-			die("initrd too big to contain in guest memory.");
+		file_size = read_file(fd_initrd, pos, limit - pos);
+		if (file_size == -1) {
+			if (errno == ENOMEM)
+				die("initrd too big to contain in guest memory.");
+
+			die_perror("initrd read");
+		}
 
 		kvm->arch.initrd_guest_start = initrd_start;
-		kvm->arch.initrd_size = host_to_guest_flat(kvm, pos) - initrd_start;
+		kvm->arch.initrd_size = file_size;
 		pr_info("Loaded initrd to 0x%llx (%llu bytes)",
 			kvm->arch.initrd_guest_start,
 			kvm->arch.initrd_size);
-- 
2.5.1

[PATCH 7/7] arm: move kernel loading into arm/kvm.c

[Andre Przywara]

For some reasons (probably to have easy access to the command line)
the kernel loading for arm and arm64 was located in arm/fdt.c.
Move the routines to kvm.c (where other architectures put it) to
only have real device tree code in fdt.c. We use the pointer in
struct kvm to access the command line string.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c | 95 +--------------------------------------------------------------
 arm/kvm.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 94 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 19d7ed9..381d48f 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -9,14 +9,11 @@
 
 #include <stdbool.h>
 
-#include <asm/setup.h>
 #include <linux/byteorder.h>
 #include <linux/kernel.h>
 #include <linux/sizes.h>
 #include <linux/psci.h>
 
-static char kern_cmdline[COMMAND_LINE_SIZE];
-
 bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
 {
 	return false;
@@ -145,7 +142,7 @@ static int setup_fdt(struct kvm *kvm)
 	/* /chosen */
 	_FDT(fdt_begin_node(fdt, "chosen"));
 	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
-	_FDT(fdt_property_string(fdt, "bootargs", kern_cmdline));
+	_FDT(fdt_property_string(fdt, "bootargs", kvm->cfg.real_cmdline));
 
 	/* Initrd */
 	if (kvm->arch.initrd_size != 0) {
@@ -223,93 +220,3 @@ static int setup_fdt(struct kvm *kvm)
 	return 0;
 }
 late_init(setup_fdt);
-
-#define FDT_ALIGN	SZ_2M
-#define INITRD_ALIGN	4
-bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
-				 const char *kernel_cmdline)
-{
-	void *pos, *kernel_end, *limit;
-	unsigned long guest_addr;
-	ssize_t file_size;
-
-	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
-		die_perror("lseek");
-
-	/*
-	 * Linux requires the initrd and dtb to be mapped inside lowmem,
-	 * so we can't just place them@the top of memory.
-	 */
-	limit = kvm->ram_start + min(kvm->ram_size, (u64)SZ_256M) - 1;
-
-	pos = kvm->ram_start + ARM_KERN_OFFSET(kvm);
-	kvm->arch.kern_guest_start = host_to_guest_flat(kvm, pos);
-	file_size = read_file(fd_kernel, pos, limit - pos);
-	if (file_size < 0) {
-		if (errno == ENOMEM)
-			die("kernel image too big to contain in guest memory.");
-
-		die_perror("kernel read");
-	}
-	kernel_end = pos + file_size;
-	pr_info("Loaded kernel to 0x%llx (%zd bytes)",
-		kvm->arch.kern_guest_start, file_size);
-
-	/*
-	 * Now load backwards from the end of memory so the kernel
-	 * decompressor has plenty of space to work with. First up is
-	 * the device tree blob...
-	 */
-	pos = limit;
-	pos -= (FDT_MAX_SIZE + FDT_ALIGN);
-	guest_addr = ALIGN(host_to_guest_flat(kvm, pos), FDT_ALIGN);
-	pos = guest_flat_to_host(kvm, guest_addr);
-	if (pos < kernel_end)
-		die("fdt overlaps with kernel image.");
-
-	kvm->arch.dtb_guest_start = guest_addr;
-	pr_info("Placing fdt at 0x%llx - 0x%llx",
-		kvm->arch.dtb_guest_start,
-		host_to_guest_flat(kvm, limit));
-	limit = pos;
-
-	/* ... and finally the initrd, if we have one. */
-	if (fd_initrd != -1) {
-		struct stat sb;
-		unsigned long initrd_start;
-
-		if (lseek(fd_initrd, 0, SEEK_SET) < 0)
-			die_perror("lseek");
-
-		if (fstat(fd_initrd, &sb))
-			die_perror("fstat");
-
-		pos -= (sb.st_size + INITRD_ALIGN);
-		guest_addr = ALIGN(host_to_guest_flat(kvm, pos), INITRD_ALIGN);
-		pos = guest_flat_to_host(kvm, guest_addr);
-		if (pos < kernel_end)
-			die("initrd overlaps with kernel image.");
-
-		initrd_start = guest_addr;
-		file_size = read_file(fd_initrd, pos, limit - pos);
-		if (file_size == -1) {
-			if (errno == ENOMEM)
-				die("initrd too big to contain in guest memory.");
-
-			die_perror("initrd read");
-		}
-
-		kvm->arch.initrd_guest_start = initrd_start;
-		kvm->arch.initrd_size = file_size;
-		pr_info("Loaded initrd to 0x%llx (%llu bytes)",
-			kvm->arch.initrd_guest_start,
-			kvm->arch.initrd_size);
-	} else {
-		kvm->arch.initrd_size = 0;
-	}
-
-	strncpy(kern_cmdline, kernel_cmdline, COMMAND_LINE_SIZE);
-	kern_cmdline[COMMAND_LINE_SIZE - 1] = '\0';
-
-	return true;
-}
diff --git a/arm/kvm.c b/arm/kvm.c
index d0e4a20..433a7bc 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -3,6 +3,7 @@
 #include "kvm/util.h"
 #include "kvm/8250-serial.h"
 #include "kvm/virtio-console.h"
+#include "kvm/fdt.h"
 
 #include "arm-common/gic.h"
 
@@ -85,3 +86,90 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
 }
+
+#define FDT_ALIGN	SZ_2M
+#define INITRD_ALIGN	4
+bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
+				 const char *kernel_cmdline)
+{
+	void *pos, *kernel_end, *limit;
+	unsigned long guest_addr;
+	ssize_t file_size;
+
+	if (lseek(fd_kernel, 0, SEEK_SET) < 0)
+		die_perror("lseek");
+
+	/*
+	 * Linux requires the initrd and dtb to be mapped inside lowmem,
+	 * so we can't just place them@the top of memory.
+	 */
+	limit = kvm->ram_start + min(kvm->ram_size, (u64)SZ_256M) - 1;
+
+	pos = kvm->ram_start + ARM_KERN_OFFSET(kvm);
+	kvm->arch.kern_guest_start = host_to_guest_flat(kvm, pos);
+	file_size = read_file(fd_kernel, pos, limit - pos);
+	if (file_size < 0) {
+		if (errno == ENOMEM)
+			die("kernel image too big to contain in guest memory.");
+
+		die_perror("kernel read");
+	}
+	kernel_end = pos + file_size;
+	pr_info("Loaded kernel to 0x%llx (%zd bytes)",
+		kvm->arch.kern_guest_start, file_size);
+
+	/*
+	 * Now load backwards from the end of memory so the kernel
+	 * decompressor has plenty of space to work with. First up is
+	 * the device tree blob...
+	 */
+	pos = limit;
+	pos -= (FDT_MAX_SIZE + FDT_ALIGN);
+	guest_addr = ALIGN(host_to_guest_flat(kvm, pos), FDT_ALIGN);
+	pos = guest_flat_to_host(kvm, guest_addr);
+	if (pos < kernel_end)
+		die("fdt overlaps with kernel image.");
+
+	kvm->arch.dtb_guest_start = guest_addr;
+	pr_info("Placing fdt at 0x%llx - 0x%llx",
+		kvm->arch.dtb_guest_start,
+		host_to_guest_flat(kvm, limit));
+	limit = pos;
+
+	/* ... and finally the initrd, if we have one. */
+	if (fd_initrd != -1) {
+		struct stat sb;
+		unsigned long initrd_start;
+
+		if (lseek(fd_initrd, 0, SEEK_SET) < 0)
+			die_perror("lseek");
+
+		if (fstat(fd_initrd, &sb))
+			die_perror("fstat");
+
+		pos -= (sb.st_size + INITRD_ALIGN);
+		guest_addr = ALIGN(host_to_guest_flat(kvm, pos), INITRD_ALIGN);
+		pos = guest_flat_to_host(kvm, guest_addr);
+		if (pos < kernel_end)
+			die("initrd overlaps with kernel image.");
+
+		initrd_start = guest_addr;
+		file_size = read_file(fd_initrd, pos, limit - pos);
+		if (file_size == -1) {
+			if (errno == ENOMEM)
+				die("initrd too big to contain in guest memory.");
+
+			die_perror("initrd read");
+		}
+
+		kvm->arch.initrd_guest_start = initrd_start;
+		kvm->arch.initrd_size = file_size;
+		pr_info("Loaded initrd to 0x%llx (%llu bytes)",
+			kvm->arch.initrd_guest_start,
+			kvm->arch.initrd_size);
+	} else {
+		kvm->arch.initrd_size = 0;
+	}
+
+	return true;
+}
-- 
2.5.1

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Will Deacon]

On Fri, Oct 30, 2015 at 06:26:53PM +0000, Andre Przywara wrote:
> Hi,

Hello Andre,

> this series cleans up kvmtool's kernel loading functionality a bit.
> It has been broken out of a previous series I sent [1] and contains
> just the cleanup and bug fix parts, which should be less controversial
> and thus easier to merge ;-)
> I will resend the pipe loading part later on as a separate series.
> 
> The first patch properly abstracts kernel loading to move
> responsibility into each architecture's code. It removes quite some
> ugly code from the generic kvm.c file.
> The later patches address the naive usage of read(2) to, well, read
> data from files. Doing this without coping with the subtleties of
> the UNIX read semantics (returning with less or none data read is not
> an error) can provoke hard to debug failures.
> So these patches make use of the existing and one new wrapper function
> to make sure we read everything we actually wanted to.
> The last patch moves the ARM kernel loading code into the proper
> location to be in line with the other architectures.
> 
> Please have a look and give some comments!

Looks good to me, but I'd like to see some comments from some mips/ppc/x86
people on the changes you're making over there.

Will

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Dimitri John Ledkov]

On 2 November 2015 at 14:58, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 30, 2015 at 06:26:53PM +0000, Andre Przywara wrote:
>> Hi,
>
> Hello Andre,
>
>> this series cleans up kvmtool's kernel loading functionality a bit.
>> It has been broken out of a previous series I sent [1] and contains
>> just the cleanup and bug fix parts, which should be less controversial
>> and thus easier to merge ;-)
>> I will resend the pipe loading part later on as a separate series.
>>
>> The first patch properly abstracts kernel loading to move
>> responsibility into each architecture's code. It removes quite some
>> ugly code from the generic kvm.c file.
>> The later patches address the naive usage of read(2) to, well, read
>> data from files. Doing this without coping with the subtleties of
>> the UNIX read semantics (returning with less or none data read is not
>> an error) can provoke hard to debug failures.
>> So these patches make use of the existing and one new wrapper function
>> to make sure we read everything we actually wanted to.
>> The last patch moves the ARM kernel loading code into the proper
>> location to be in line with the other architectures.
>>
>> Please have a look and give some comments!
>
> Looks good to me, but I'd like to see some comments from some mips/ppc/x86
> people on the changes you're making over there.

Looks mostly good to me, as one of the kvmtool down streams. Over at
https://github.com/clearlinux/kvmtool we have some patches to tweak
the x86 boot flow, which will need rebasing/retweaking) specifically
this commit here -
https://github.com/clearlinux/kvmtool/commit/a8dee709f85735d16739d0eda0cc00d3c1b17477

-- 
Regards,

Dimitri.
53 sleeps till Christmas, or less

https://clearlinux.org
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Andre Przywara]

Hi Dimitri,

On 02/11/15 15:17, Dimitri John Ledkov wrote:
> On 2 November 2015 at 14:58, Will Deacon <will.deacon@arm.com> wrote:
>> On Fri, Oct 30, 2015 at 06:26:53PM +0000, Andre Przywara wrote:
>>> Hi,
>>
>> Hello Andre,
>>
>>> this series cleans up kvmtool's kernel loading functionality a bit.
>>> It has been broken out of a previous series I sent [1] and contains
>>> just the cleanup and bug fix parts, which should be less controversial
>>> and thus easier to merge ;-)
>>> I will resend the pipe loading part later on as a separate series.
>>>
>>> The first patch properly abstracts kernel loading to move
>>> responsibility into each architecture's code. It removes quite some
>>> ugly code from the generic kvm.c file.
>>> The later patches address the naive usage of read(2) to, well, read
>>> data from files. Doing this without coping with the subtleties of
>>> the UNIX read semantics (returning with less or none data read is not
>>> an error) can provoke hard to debug failures.
>>> So these patches make use of the existing and one new wrapper function
>>> to make sure we read everything we actually wanted to.
>>> The last patch moves the ARM kernel loading code into the proper
>>> location to be in line with the other architectures.
>>>
>>> Please have a look and give some comments!
>>
>> Looks good to me, but I'd like to see some comments from some mips/ppc/x86
>> people on the changes you're making over there.
> 
> Looks mostly good to me, as one of the kvmtool down streams. Over at
> https://github.com/clearlinux/kvmtool we have some patches to tweak
> the x86 boot flow, which will need rebasing/retweaking) specifically
> this commit here -
> https://github.com/clearlinux/kvmtool/commit/a8dee709f85735d16739d0eda0cc00d3c1b17477

Awesome - I was actually thinking about coding something like this!
In the last week I move the MIPS ELF loading out of mips/ into /elf.c to
be able to load kvm-unit-tests' tests (which are Multiboot/ELF). As
multiboot requires entering in protected mode, I was thinking about
changing kvmtool to support entering a guest directly in protected mode
- seems like this is mostly what you've done here already.

Looks like we should both post our patches to merge them somehow ;-)

Cheers,
Andre.

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Andre Przywara]

Hi Will,

On 02/11/15 14:58, Will Deacon wrote:
> On Fri, Oct 30, 2015 at 06:26:53PM +0000, Andre Przywara wrote:
>> Hi,
> 
> Hello Andre,
> 
>> this series cleans up kvmtool's kernel loading functionality a bit.
>> It has been broken out of a previous series I sent [1] and contains
>> just the cleanup and bug fix parts, which should be less controversial
>> and thus easier to merge ;-)
>> I will resend the pipe loading part later on as a separate series.
>>
>> The first patch properly abstracts kernel loading to move
>> responsibility into each architecture's code. It removes quite some
>> ugly code from the generic kvm.c file.
>> The later patches address the naive usage of read(2) to, well, read
>> data from files. Doing this without coping with the subtleties of
>> the UNIX read semantics (returning with less or none data read is not
>> an error) can provoke hard to debug failures.
>> So these patches make use of the existing and one new wrapper function
>> to make sure we read everything we actually wanted to.
>> The last patch moves the ARM kernel loading code into the proper
>> location to be in line with the other architectures.
>>
>> Please have a look and give some comments!
> 
> Looks good to me, but I'd like to see some comments from some mips/ppc/x86
> people on the changes you're making over there.

Sounds reasonable, but no answers yet.

Can you take at least patch 1 and 2 meanwhile, preferably 6 and 7 (the
ARM parts) also if you are OK with it?
I have other patches that depend on 1/7 and 2/7, so having them upstream
would help me and reduce further dependency churn.
I am happy to resend the remaining patches for further discussion later.

Cheers,
Andre.

[PATCH 0/7] kvmtool: Cleanup kernel loading

[Will Deacon]

On Wed, Nov 18, 2015 at 10:29:30AM +0000, Andre Przywara wrote:
> On 02/11/15 14:58, Will Deacon wrote:
> > On Fri, Oct 30, 2015 at 06:26:53PM +0000, Andre Przywara wrote:
> >> this series cleans up kvmtool's kernel loading functionality a bit.
> >> It has been broken out of a previous series I sent [1] and contains
> >> just the cleanup and bug fix parts, which should be less controversial
> >> and thus easier to merge ;-)
> >> I will resend the pipe loading part later on as a separate series.
> >>
> >> The first patch properly abstracts kernel loading to move
> >> responsibility into each architecture's code. It removes quite some
> >> ugly code from the generic kvm.c file.
> >> The later patches address the naive usage of read(2) to, well, read
> >> data from files. Doing this without coping with the subtleties of
> >> the UNIX read semantics (returning with less or none data read is not
> >> an error) can provoke hard to debug failures.
> >> So these patches make use of the existing and one new wrapper function
> >> to make sure we read everything we actually wanted to.
> >> The last patch moves the ARM kernel loading code into the proper
> >> location to be in line with the other architectures.
> >>
> >> Please have a look and give some comments!
> > 
> > Looks good to me, but I'd like to see some comments from some mips/ppc/x86
> > people on the changes you're making over there.
> 
> Sounds reasonable, but no answers yet.
> 
> Can you take at least patch 1 and 2 meanwhile, preferably 6 and 7 (the
> ARM parts) also if you are OK with it?
> I have other patches that depend on 1/7 and 2/7, so having them upstream
> would help me and reduce further dependency churn.
> I am happy to resend the remaining patches for further discussion later.

We let them sit on the list for a while with no comments, so I just pushed
out your series. If a bug report shows up, then we can always revert the
offending patch if there's no quick fix.

Will