Re: [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.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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.dea...@arm.com> wrote:
>> On Fri, Oct 30, 2015 at 06:26:53PM +, 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.przyw...@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, , 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, , 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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.przyw...@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 
 
-#include 
 #include 
 #include 
 #include 
 #include 
 
-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 at 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, ))
-   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/ar

[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.przyw...@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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.przyw...@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 
 #include 
+#include 
 
 #include 
 #include 
@@ -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, , sizeof(boot)) != sizeof(boot))
+   if (read_in_full(fd_kernel, , sizeof(boot)) != sizeof(boot))
return false;
 
if (memcmp(, 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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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=143825354808135=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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.przyw...@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 0x100
-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

[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.przyw...@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 = 0x8100ull;
 
-   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, , sizeof(phdr));
-   if (nr != sizeof(phdr)) {
+   if (read_in_full(fd_kernel, , 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, , sizeof(phdr));
-   if (nr != sizeof(phdr)) {
+   if (read_in_full(fd_kernel, , 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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.przyw...@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, , 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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/14] MIPS: use pseek() in ELF kernel image loading

[Andre Przywara]

Use the newly introduced pseek() function when skipping to the start
offset in the ELF file.
The layout of an ELF file should satisfy the constraints of pseek, so
that we should be able to use a pipe file descriptor as well.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 mips/kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mips/kvm.c b/mips/kvm.c
index c1c596c..4d08b20 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -328,8 +328,8 @@ static bool load_elf_binary(struct kvm *kvm, int fd_kernel)
 
kvm-arch.entry_point = ei.entry_point;
 
-   if (lseek(fd_kernel, ei.offset, SEEK_SET)  0)
-   die_perror(lseek);
+   if (!pseek(fd_kernel, ei.offset - sizeof(union ElfHeaders)))
+   die_perror(seek);
 
p = guest_flat_to_host(kvm, ei.load_addr);
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/14] 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.przyw...@arm.com
---
 powerpc/kvm.c | 36 
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 87d0f9e..9888bf1 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -162,16 +162,19 @@ 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;
 
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);
@@ -180,19 +183,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.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/14] kvmtool: Refactor kernel image loading to allow pipes

[Andre Przywara]

Currently kvmtool uses rewinds (lseeks to position 0) on kernel image
files. This prevents non-regular files (for instance pipes in the -k
parameter) from being used as a kernel image file.

This series reworks the kernel loading to avoid any seeks and allows
to pipe in kernel images on the command line. This basically gives us
decompression and direct kernel downloading for free in a neat UNIX
way:
$ lkvm run -k (zcat zImage.gz) ...
$ lkvm run -k (dd if=uImage bs=64 skip=1) ...
$ lkvm run -k (wget -O - http://foo.com/guest.zImage) ...
$ lkvm run -k (curl -s tftp://server/guest.zImage) ...

The first patch refactors the kernel image loading, which currently
tries to be architecture agnostic in a very intricate way. As the
actual implementations are very much architecture specific, make this
clear in the code by introducing separate functions for each arch.

Allowing pipes is quite easy for arm/arm64 and powerpc, since they
only do a (now pointless) rewind in the beginning, which we simply
drop in patch 2.
x86 requires some more love, patch 3 and 4 take care of that for
bzImage and flat binaries, respectively.
Since the MIPS ELF loader contains an actual (non-rewinding) seek,
patch 5 adds a pipe-aware wrapper around lseek to still do some
(forward) seeking despite the file descriptor being non-seekable.
Patch 6, 7 and 8 then use this to eventually rework the kernel image
loading for MIPS to be pipe-safe, too.
Patch 9 moves the ARM kernel loading from arm/fdt.c into arm/kvm.c,
to be in line with the other architectures.
Now that we may read from pipes or sockets, simply using the read(2)
syscall breaks occasionally.
Patch 10 introduces a safe wrapper for reading whole files (inspired
by the ARM implementation), whereas patches 11-14 move the kernel
loading in each architecture over to using the safe read wrappers.

These patches apply on top of the latest kvmtool master branch.
So far I could test arm, arm64 and x86, with MIPS and PowerPC
being at least compile-tested.

Cheers,
Andre.

Andre Przywara (14):
  Refactor kernel image loading
  arm/powerpc: remove unneeded seeks in kernel loading
  x86: allow pipes for bzImage kernel images
  x86: support loading flat binary kernel images from a pipe
  kvmtool: introduce pseek
  MIPS: use pseek() in ELF kernel image loading
  MIPS: move ELF headers loading outside of load_elf_binary()
  MIPS: remove seeks from load_flat_binary()
  arm: move kernel loading into arm/kvm.c
  provide generic read_file() implementation
  arm/arm64: use read_file() in kernel and initrd loading
  powerpc: use read_file() in kernel and initrd loading
  MIPS: use read wrappers in kernel loading
  x86: use read wrappers in kernel loading

 arm/fdt.c|  99 +---
 arm/kvm.c|  87 
 include/kvm/kvm.h|   5 +--
 include/kvm/read-write.h |   4 ++
 kvm.c|  42 ++---
 mips/kvm.c   | 114 ++-
 powerpc/kvm.c|  42 -
 util/read-write.c|  61 +
 x86/kvm.c| 102 ++
 9 files changed, 297 insertions(+), 259 deletions(-)

-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/14] MIPS: remove seeks from load_flat_binary()

[Andre Przywara]

Remove the need to rewind the kernel image file if loading it as a
flat binary by re-using the already read portion of the file passed
in as a buffer.
This allows the MIPS flat binary kernel image to be read from a pipe.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 mips/kvm.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mips/kvm.c b/mips/kvm.c
index ed81a02..d970ee0 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -164,24 +164,26 @@ static void kvm__mips_install_cmdline(struct kvm *kvm)
 /* Load at the 1M point. */
 #define KERNEL_LOAD_ADDR 0x100
 
-static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
+static bool load_flat_binary(struct kvm *kvm, int fd_kernel, const void *buf,
+int buflen)
 {
void *p;
void *k_start;
int nr;
 
-   if (lseek(fd_kernel, 0, SEEK_SET)  0)
-   die_perror(lseek);
-
p = k_start = guest_flat_to_host(kvm, KERNEL_LOAD_ADDR);
 
+   memcpy(p, buf, buflen);
+   p += buflen;
+
while ((nr = read(fd_kernel, p, 65536))  0)
p += nr;
 
kvm-arch.is64bit = true;
kvm-arch.entry_point = 0x8100ull;
 
-   pr_info(Loaded kernel to 0x%x (%ld bytes), KERNEL_LOAD_ADDR, (long 
int)(p - k_start));
+   pr_info(Loaded kernel to 0x%x (%ld bytes), KERNEL_LOAD_ADDR,
+   (long int)(p - k_start));
 
return true;
 }
@@ -357,7 +359,7 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int 
fd_kernel, int fd_initrd,
return true;
}
 
-   return load_flat_binary(kvm, fd_kernel);
+   return load_flat_binary(kvm, fd_kernel, eh, sizeof(eh));
 }
 
 void ioport__map_irq(u8 *irq)
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/14] kvmtool: introduce pseek

[Andre Przywara]

pseek is a wrapper around lseek(2), which can cope with non-seekable
file descriptors. The limitation is of course that we only can seek
forward and only relative to the current position (hence the whence
parameter is missing). In this case pseek will do dummy reads to skip
over the requested amount of data.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 include/kvm/read-write.h |  2 ++
 util/read-write.c| 40 
 2 files changed, 42 insertions(+)

diff --git a/include/kvm/read-write.h b/include/kvm/read-write.h
index 67571f9..87eb167 100644
--- a/include/kvm/read-write.h
+++ b/include/kvm/read-write.h
@@ -40,4 +40,6 @@ int aio_pwritev(io_context_t ctx, struct iocb *iocb, int fd, 
const struct iovec
off_t offset, int ev, void *param);
 #endif
 
+int pseek(int fd, off_t offset);
+
 #endif /* KVM_READ_WRITE_H */
diff --git a/util/read-write.c b/util/read-write.c
index 44709df..401afd3 100644
--- a/util/read-write.c
+++ b/util/read-write.c
@@ -352,3 +352,43 @@ restart:
return ret;
 }
 #endif
+
+/*
+ * A wrapper around lseek(fd, offset, SEEK_CUR), which can cope with being
+ * used on non-seekable file descriptors like pipes or sockets. If offset
+ * is positive, it will skip over the requested number of bytes by reading
+ * them into a dummy buffer.
+ */
+#define BUF_SIZE 256
+int pseek(int fd, off_t offset)
+{
+   off_t ret;
+   int to_read;
+   char skip_buf[BUF_SIZE];
+
+   ret = lseek(fd, offset, SEEK_CUR);
+   if (ret != (off_t)-1)
+   return 0;
+
+   if (errno != ESPIPE)
+   return -1;
+
+   if (offset  0) {
+   errno = EINVAL;
+   return -1;
+   }
+
+   while (offset  0) {
+   to_read = offset  BUF_SIZE ? BUF_SIZE : offset;
+   ret = read(fd, skip_buf, to_read);
+   if (ret == -1  errno != EINTR)
+   return -1;
+   if (ret == 0) {
+   errno = EINVAL;
+   return -1;
+   }
+   offset -= ret;
+   }
+
+   return 0;
+}
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/14] 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.przyw...@arm.com
---
 mips/kvm.c | 35 ---
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/mips/kvm.c b/mips/kvm.c
index d970ee0..2f0d61b 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -169,21 +169,27 @@ static bool load_flat_binary(struct kvm *kvm, int 
fd_kernel, const void *buf,
 {
void *p;
void *k_start;
-   int nr;
+   ssize_t kernel_size;
 
p = k_start = guest_flat_to_host(kvm, KERNEL_LOAD_ADDR);
 
memcpy(p, buf, buflen);
p += buflen;
 
-   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 = 0x8100ull;
 
-   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;
 }
@@ -199,7 +205,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)) {
@@ -214,8 +219,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;
}
@@ -245,7 +249,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)) {
@@ -260,8 +263,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;
}
@@ -292,7 +294,6 @@ union ElfHeaders {
 static bool load_elf_binary(struct kvm *kvm, int fd_kernel,
union ElfHeaders *eh)
 {
-   size_t nr;
char *p;
struct kvm__arch_elf_info ei;
 
@@ -331,13 +332,9 @@ static bool load_elf_binary(struct kvm *kvm, int fd_kernel,
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);
+
+   if (read_in_full(fd_kernel, p, ei.len) != (ssize_t)ei.len)
+   die_perror(read);
 
return true;
 }
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/14] arm/powerpc: remove unneeded seeks in kernel loading

[Andre Przywara]

With the introduction of kvm__arch_load_kernel_image() we are sure
that nobody peeks at the kernel image files before, so rewinding
the file to its beginning is unnecessary.
Remove those seeks in the arm and powerpc implementations.
This allows to use a pipe instead of a regular file for the kernel
image, enabling on-the-fly uncompression or downloading via the
command line.
$ lkvm run -k (zcat zImage.gz) ...
$ lkvm run -k (wget -O - http://foo.com/guest.zImage) ...

This is limited to the kernel images for the arm/arm64 and powerpc
architectures for now, other architectures and initrds need more work.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 arm/fdt.c | 3 ---
 powerpc/kvm.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index ec7453f..cb4f00d 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -245,9 +245,6 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int 
fd_kernel, int fd_initrd,
void *pos, *kernel_end, *limit;
unsigned long guest_addr;
 
-   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 at the top of memory.
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 13bba30..87d0f9e 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -165,9 +165,6 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int 
fd_kernel, int fd_initrd,
void *i_start;
int nr;
 
-   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)
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/14] 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.przyw...@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 87eb167..658a653 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 401afd3..32691a9 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.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/14] MIPS: move ELF headers loading outside of load_elf_binary()

[Andre Przywara]

Refactor MIPS' load_elf_binary() implementation by not reading the
ELF header itself, but using a pointer to a memory buffer instead.
This prepares for removing the need to rewind the image file later.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 mips/kvm.c | 52 +---
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/mips/kvm.c b/mips/kvm.c
index 4d08b20..ed81a02 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -282,46 +282,39 @@ static bool kvm__arch_get_elf_32_info(Elf32_Ehdr *ehdr, 
int fd_kernel,
return true;
 }
 
-static bool load_elf_binary(struct kvm *kvm, int fd_kernel)
-{
-   union {
-   Elf64_Ehdr ehdr;
-   Elf32_Ehdr ehdr32;
-   } eh;
+union ElfHeaders {
+   Elf64_Ehdr ehdr;
+   Elf32_Ehdr ehdr32;
+};
 
+static bool load_elf_binary(struct kvm *kvm, int fd_kernel,
+   union ElfHeaders *eh)
+{
size_t nr;
char *p;
struct kvm__arch_elf_info ei;
 
-   if (lseek(fd_kernel, 0, SEEK_SET)  0)
-   die_perror(lseek);
-
-   nr = read(fd_kernel, eh, sizeof(eh));
-   if (nr != sizeof(eh)) {
-   pr_info(Couldn't read %d bytes for ELF header., 
(int)sizeof(eh));
-   return false;
-   }
-
-   if (eh.ehdr.e_ident[EI_MAG0] != ELFMAG0 ||
-   eh.ehdr.e_ident[EI_MAG1] != ELFMAG1 ||
-   eh.ehdr.e_ident[EI_MAG2] != ELFMAG2 ||
-   eh.ehdr.e_ident[EI_MAG3] != ELFMAG3 ||
-   (eh.ehdr.e_ident[EI_CLASS] != ELFCLASS64  
eh.ehdr.e_ident[EI_CLASS] != ELFCLASS32) ||
-   eh.ehdr.e_ident[EI_VERSION] != EV_CURRENT) {
+   if (eh-ehdr.e_ident[EI_MAG0] != ELFMAG0 ||
+   eh-ehdr.e_ident[EI_MAG1] != ELFMAG1 ||
+   eh-ehdr.e_ident[EI_MAG2] != ELFMAG2 ||
+   eh-ehdr.e_ident[EI_MAG3] != ELFMAG3 ||
+   (eh-ehdr.e_ident[EI_CLASS] != ELFCLASS64 
+   eh-ehdr.e_ident[EI_CLASS] != ELFCLASS32) ||
+   eh-ehdr.e_ident[EI_VERSION] != EV_CURRENT) {
pr_info(Incompatible ELF header.);
return false;
}
-   if (eh.ehdr.e_type != ET_EXEC || eh.ehdr.e_machine != EM_MIPS) {
+   if (eh-ehdr.e_type != ET_EXEC || eh-ehdr.e_machine != EM_MIPS) {
pr_info(Incompatible ELF not MIPS EXEC.);
return false;
}
 
-   if (eh.ehdr.e_ident[EI_CLASS] == ELFCLASS64) {
-   if (!kvm__arch_get_elf_64_info(eh.ehdr, fd_kernel, ei))
+   if (eh-ehdr.e_ident[EI_CLASS] == ELFCLASS64) {
+   if (!kvm__arch_get_elf_64_info(eh-ehdr, fd_kernel, ei))
return false;
kvm-arch.is64bit = true;
} else {
-   if (!kvm__arch_get_elf_32_info(eh.ehdr32, fd_kernel, ei))
+   if (!kvm__arch_get_elf_32_info(eh-ehdr32, fd_kernel, ei))
return false;
kvm-arch.is64bit = false;
}
@@ -334,7 +327,8 @@ 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);
+   (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)
@@ -349,12 +343,16 @@ static bool load_elf_binary(struct kvm *kvm, int 
fd_kernel)
 bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 const char *kernel_cmdline)
 {
+   union ElfHeaders eh;
+
if (fd_initrd != -1) {
pr_err(Initrd not supported on MIPS.);
return false;
}
 
-   if (load_elf_binary(kvm, fd_kernel)) {
+   read_in_full(fd_kernel, eh, sizeof(eh));
+
+   if (load_elf_binary(kvm, fd_kernel, eh)) {
kvm__mips_install_cmdline(kvm);
return true;
}
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/14] 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.przyw...@arm.com
---
 arm/fdt.c | 96 +--
 arm/kvm.c | 89 ++
 2 files changed, 90 insertions(+), 95 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index cb4f00d..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,94 +220,3 @@ static int setup_fdt(struct kvm *kvm)
return 0;
 }
 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,
-const char *kernel_cmdline)
-{
-   void *pos, *kernel_end, *limit;
-   unsigned long guest_addr;
-
-   /*
-* Linux requires the initrd and dtb to be mapped inside lowmem,
-* so we can't just place them at 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);
-   if (read_image(fd_kernel, pos, limit) == -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);
-
-   /*
-* 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;
-   if (read_image(fd_initrd, pos, limit) == -ENOMEM)
-   die(initrd too big to contain in guest memory.);
-
-   kvm-arch.initrd_guest_start = initrd_start;
-   kvm-arch.initrd_size = host_to_guest_flat(kvm, pos) - 
initrd_start;
-   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..6e3f80e 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,91

[PATCH 01/14] 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 and rename the existing flat binary
loader functions for the other architectures to the new name.

Signed-off-by: Andre Przywara andre.przyw...@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 0x100
-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

[PATCH 14/14] 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.przyw...@arm.com
---
 x86/kvm.c | 40 +++-
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/x86/kvm.c b/x86/kvm.c
index 9817953..8cf4ec6 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 *buf, int 
len)
 {
void *p;
-   int nr;
 
p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
 
memcpy(p, buf, len);
p += len;
 
-   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,11 +232,9 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
 const char *kernel_cmdline, struct boot_params *boot)
 {
struct boot_params *kern_boot;
-   unsigned long setup_sects;
size_t cmdline_size;
-   ssize_t setup_size;
+   ssize_t file_size;
void *p;
-   int nr;
u16 vidmode;
 
/*
@@ -250,25 +248,26 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
if (boot-hdr.version  BOOT_PROTOCOL_REQUIRED)
die(Too old kernel);
 
+   /* read real-mode setup.bin to boot loader address */
+   p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
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;
-   p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
+   file_size = (boot-hdr.setup_sects + 1)  9;
 
-   /* copy setup.bin to mem */
+   /* copy in the part already read earlier from the file */
memcpy(p, boot, sizeof(struct boot_params));
p += sizeof(struct boot_params);
-   setup_size -= sizeof(struct boot_params);
-   if (read(fd_kernel, p, setup_size) != setup_size)
-   die_perror(read);
+   file_size -= sizeof(struct boot_params);
 
-   /* copy vmlinux.bin to BZ_KERNEL_START*/
-   p = guest_flat_to_host(kvm, BZ_KERNEL_START);
+   if (read_in_full(fd_kernel, p, file_size) != file_size)
+   die_perror(kernel setup read);
 
-   while ((nr = read(fd_kernel, p, 65536))  0)
-   p += nr;
+   /* read actual kernel image (vmlinux.bin) to BZ_KERNEL_START */
+   p = guest_flat_to_host(kvm, BZ_KERNEL_START);
+   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) {
@@ -319,8 +318,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;
@@ -343,7 +341,7 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int 
fd_kernel, int fd_initrd,
 {
struct boot_params boot;
 
-   if (read(fd_kernel, boot, sizeof(boot)) != sizeof(boot))
+   if (read_in_full(fd_kernel, boot, sizeof(boot)) != sizeof(boot))
return false;
 
if (load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline, boot))
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/14] 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.przyw...@arm.com
---
 arm/kvm.c | 42 --
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/arm/kvm.c b/arm/kvm.c
index 6e3f80e..277d9e6 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -87,19 +87,6 @@ void kvm__arch_init(struct kvm *kvm, const char 
*hugetlbfs_path, u64 ram_size)
die(Failed to create virtual GIC);
 }
 
-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,
@@ -107,6 +94,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;
 
/*
 * Linux requires the initrd and dtb to be mapped inside lowmem,
@@ -116,13 +104,17 @@ 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.);
+
+   die_perror(kernel read);
+   }
+   kernel_end = pos + file_size;
 
-   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);
+   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
@@ -160,11 +152,17 @@ 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.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/14] x86: support loading flat binary kernel images from a pipe

[Andre Przywara]

With the latest patches we allow loading bzImage kernels from a pipe,
but we still fail on flat binary images.
Rework the loading routines to take memory buffers for the beginning
of the file, so we don't need to rewind the image.
This allows to fall back to flat binary loading if bzImage fails
without using a seek, so kvmtool will happily accept any file
descriptor (including pipes) for the image file.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 x86/kvm.c | 48 +---
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/x86/kvm.c b/x86/kvm.c
index 8fe5585..9817953 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -206,16 +206,16 @@ static inline void *guest_real_to_host(struct kvm *kvm, 
u16 selector, u16 offset
return guest_flat_to_host(kvm, flat);
 }
 
-static bool load_flat_binary(struct kvm *kvm, int fd_kernel)
+static bool load_flat_binary(struct kvm *kvm, int fd_kernel, void *buf, int 
len)
 {
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);
 
+   memcpy(p, buf, len);
+   p += len;
+
while ((nr = read(fd_kernel, p, 65536))  0)
p += nr;
 
@@ -229,11 +229,10 @@ static bool load_flat_binary(struct kvm *kvm, int 
fd_kernel)
 static const char *BZIMAGE_MAGIC = HdrS;
 
 static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
-const char *kernel_cmdline)
+const char *kernel_cmdline, struct boot_params *boot)
 {
struct boot_params *kern_boot;
unsigned long setup_sects;
-   struct boot_params boot;
size_t cmdline_size;
ssize_t setup_size;
void *p;
@@ -245,26 +244,23 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
 * memory layout.
 */
 
-   if (read(fd_kernel, boot, sizeof(boot)) != sizeof(boot))
-   return false;
-
-   if (memcmp(boot.hdr.header, BZIMAGE_MAGIC, strlen(BZIMAGE_MAGIC)))
+   if (memcmp(boot-hdr.header, BZIMAGE_MAGIC, strlen(BZIMAGE_MAGIC)))
return false;
 
-   if (boot.hdr.version  BOOT_PROTOCOL_REQUIRED)
+   if (boot-hdr.version  BOOT_PROTOCOL_REQUIRED)
die(Too old kernel);
 
-   if (!boot.hdr.setup_sects)
-   boot.hdr.setup_sects = BZ_DEFAULT_SETUP_SECTS;
-   setup_sects = boot.hdr.setup_sects + 1;
+   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;
p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
 
/* copy setup.bin to mem */
-   memcpy(p, boot, sizeof(boot));
-   p += sizeof(boot);
-   setup_size -= sizeof(boot);
+   memcpy(p, boot, sizeof(struct boot_params));
+   p += sizeof(struct boot_params);
+   setup_size -= sizeof(struct boot_params);
if (read(fd_kernel, p, setup_size) != setup_size)
die_perror(read);
 
@@ -277,10 +273,10 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
p = guest_flat_to_host(kvm, BOOT_CMDLINE_OFFSET);
if (kernel_cmdline) {
cmdline_size = strlen(kernel_cmdline) + 1;
-   if (cmdline_size  boot.hdr.cmdline_size)
-   cmdline_size = boot.hdr.cmdline_size;
+   if (cmdline_size  boot-hdr.cmdline_size)
+   cmdline_size = boot-hdr.cmdline_size;
 
-   memset(p, 0, boot.hdr.cmdline_size);
+   memset(p, 0, boot-hdr.cmdline_size);
memcpy(p, kernel_cmdline, cmdline_size - 1);
}
 
@@ -313,7 +309,7 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
if (fstat(fd_initrd, initrd_stat))
die_perror(fstat);
 
-   addr = boot.hdr.initrd_addr_max  ~0xf;
+   addr = boot-hdr.initrd_addr_max  ~0xf;
for (;;) {
if (addr  BZ_KERNEL_START)
die(Not enough memory for initrd);
@@ -345,15 +341,21 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
 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))
+   struct boot_params boot;
+
+   if (read(fd_kernel, boot, sizeof(boot)) != sizeof(boot))
+   return false;
+
+   if (load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline, boot))
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

[PATCH 03/14] x86: allow pipes for bzImage kernel images

[Andre Przywara]

Streamline the x86 kernel loading implementation by removing unneeded
seeks and thus allow an x86 bzImage to be loaded from a pipe.
Flat binaries are taken care of in a separate patch.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 x86/kvm.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/x86/kvm.c b/x86/kvm.c
index a0204b8..8fe5585 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -245,9 +245,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
 * memory layout.
 */
 
-   if (lseek(fd_kernel, 0, SEEK_SET)  0)
-   die_perror(lseek);
-
if (read(fd_kernel, boot, sizeof(boot)) != sizeof(boot))
return false;
 
@@ -257,9 +254,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
if (boot.hdr.version  BOOT_PROTOCOL_REQUIRED)
die(Too old kernel);
 
-   if (lseek(fd_kernel, 0, SEEK_SET)  0)
-   die_perror(lseek);
-
if (!boot.hdr.setup_sects)
boot.hdr.setup_sects = BZ_DEFAULT_SETUP_SECTS;
setup_sects = boot.hdr.setup_sects + 1;
@@ -267,7 +261,10 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, 
int fd_initrd,
setup_size = setup_sects  9;
p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
 
-   /* copy setup.bin to mem*/
+   /* copy setup.bin to mem */
+   memcpy(p, boot, sizeof(boot));
+   p += sizeof(boot);
+   setup_size -= sizeof(boot);
if (read(fd_kernel, p, setup_size) != setup_size)
die_perror(read);
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmtool: Makefile: allow overriding CC and LD

[Andre Przywara]

Hi Michael,

On 19/06/15 02:14, Michael Ellerman wrote:
 On Thu, 2015-06-18 at 16:50 +0100, Andre Przywara wrote:
 Currently we set CC unconditionally to ${CROSS_COMPILE}gcc, the same
 for LD.
 Allow people to override the compiler name by specifying it explicitly
 on the command line or via the environment.
 Beside calling a certain compiler binary this allows to pass in
 options to the compiler, which lets us get rid of the PowerPC
 overrides in the Makefile. Possible uses:
 $ make CC=gcc -m64 LD=ld -melf64ppc
 (build kvmtool on a PowerPC toolchain defaulting to 32-bit)
 $ make CC=gcc -m32 LD=ld -melf_i386
 (build a 32-bit binary on a multilib-enabled x86-64 compiler)
 
 
 I'm not a big fan of that.
 
 Your examples are all about overriding CFLAGS and LDFLAGS, not CC and LD. So
 if anything you should be allowing that. Adding flags to CC and LD is asking
 for trouble.

Will just disabled overriding CFLAGS and LDFLAGS, I think because
kvmtool inherited some C nastiness from the kernel, which does not
compile with random flags set (CFLAGS=-std=gnu99 was the one the broke it).
Maybe we should revisit that, either fix the code to be more robust to
comply with various standards or document that you should not have
CFLAGS set. Then allow overriding CFLAGS again.

But I thought that overriding CC is common practise - if you want to
select a different compiler, that is. Using a different bitness seems a
lot like a different compiler to me, same with different endianness.
I think I saw quite some examples on the web about using CC=gcc -m32.

I agree that abusing CC to pass optimization options to the compiler is
not good, but for kvmtool's Makefile I don't see how adding flags to CC
would hurt.

Cheers,
Andre.
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] powerpc: use default endianness for converting guest/init

[Andre Przywara]

Hi Michael,

On 19/06/15 02:08, Michael Ellerman wrote:
 On Thu, 2015-06-18 at 15:52 +0100, Andre Przywara wrote:
 Hi,

 On 06/17/2015 10:43 AM, Andre Przywara wrote:
 For converting the guest/init binary into an object file, we call
 the linker binary, setting the endianness to big endian explicitly
 when compiling kvmtool for powerpc.
 This breaks if the compiler is actually targetting little endian
 (which is true for the Debian port, for instance).
 Remove the explicit big endianness switch from the linker call to
 allow linking on little endian PowerPC builds again.

 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
 Hi,

 this fixed the powerpc64le build for me, while still compiling fine
 for big endian. Admittedly this whole init-guest_init.o conversion
 has its issues (with MIPS, for instance), which deserve proper fixing,
 but lets just fix that build for now.

 Will was concerned about breaking toolchains where the linker does not
 default to 64-bit. Is that an issue we care about?
 
 Yeah, that would be Debian  Ubuntu BE at least, and maybe Fedora too? I'm not
 sure how you compiled it big endian?

I have my own cross-compiler built from scratch. This is
powerpc64-linux-gnu, which is big endian. I don't have any distribution
behind it, it's just a cross-compiler with glibc.

 AFAICT LDFLAGS is only used in this dodgy binary-to-object-file
 conversion of guest/init. For this we rely on the resulting .o file to
 have the same ELF target as the other object files to be finally linked
 into the lkvm binary. As we don't compile guest/init with CFLAGS, there
 is a possible mismatch.

 I am looking into a proper fix for this now (compiling guest/init with
 CFLAGS, calling $CC with linker options instead of $LD and allowing CC
 and LD override). Still struggling with MIPS, though :-(
 
 Yeah that's obviously a better solution medium term.
 
 Can you do something like this? Sorry untested:
 
 diff --git a/Makefile b/Makefile
 index 6110b8e..8663d67 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -149,7 +149,11 @@ ifeq ($(ARCH), powerpc)
 OBJS+= powerpc/xics.o
 ARCH_INCLUDE := powerpc/include
 CFLAGS  += -m64
 -   LDFLAGS += -m elf64ppc
 +   ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),-m elf64ppc),y)
 +   LDFLAGS += -m elf64ppc
 +   else
 +   LDFLAGS += -m elf64leppc
 +   endif
  
 ARCH_WANT_LIBFDT := y
  endif

Nah, actually I want to get rid of those LDFLAGS at all. For some
reasons using ld to convert a random binary file into a C object is
causing trouble on MIPS, because ld uses a slightly different ELF target
than CC there.
I think this conversion should be more a job for objcopy than for ld,
but that does not fix the problem in a generic way (though I was able to
hack it with some magic objcopy options).

What works though is using xxd to convert the binary guest/init into a C
array:
$ xxd -i guest/init | $(CC) -x c -c - -o guest/guest_init.o
This has the nice property of using the same compiler that generates the
other object files and thus automatically matches them (which is a
problem under MIPS atm, as ld seems to default to some different ELF type).
The only issue is that xxd is part of the vim package, which would annoy
Emacs users. Not sure we are in a position to mandate vim for compiling
kvmtool ;-)

Cheers,
Andre.
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in

Re: [PATCH 2/3] powerpc: use default endianness for converting guest/init

[Andre Przywara]

Hi,

On 06/17/2015 10:43 AM, Andre Przywara wrote:
 For converting the guest/init binary into an object file, we call
 the linker binary, setting the endianness to big endian explicitly
 when compiling kvmtool for powerpc.
 This breaks if the compiler is actually targetting little endian
 (which is true for the Debian port, for instance).
 Remove the explicit big endianness switch from the linker call to
 allow linking on little endian PowerPC builds again.
 
 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
 Hi,
 
 this fixed the powerpc64le build for me, while still compiling fine
 for big endian. Admittedly this whole init-guest_init.o conversion
 has its issues (with MIPS, for instance), which deserve proper fixing,
 but lets just fix that build for now.
 

Will was concerned about breaking toolchains where the linker does not
default to 64-bit. Is that an issue we care about?
AFAICT LDFLAGS is only used in this dodgy binary-to-object-file
conversion of guest/init. For this we rely on the resulting .o file to
have the same ELF target as the other object files to be finally linked
into the lkvm binary. As we don't compile guest/init with CFLAGS, there
is a possible mismatch.

I am looking into a proper fix for this now (compiling guest/init with
CFLAGS, calling $CC with linker options instead of $LD and allowing CC
and LD override). Still struggling with MIPS, though :-(

If someone is eager to fix compilation on PowerPC meanwhile, feel free
to use this fix for the time being.

Cheers,
Andre.

 
  Makefile | 1 -
  1 file changed, 1 deletion(-)
 
 diff --git a/Makefile b/Makefile
 index 6110b8e..c118e1a 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -149,7 +149,6 @@ ifeq ($(ARCH), powerpc)
   OBJS+= powerpc/xics.o
   ARCH_INCLUDE := powerpc/include
   CFLAGS  += -m64
 - LDFLAGS += -m elf64ppc
  
   ARCH_WANT_LIBFDT := y
  endif
 
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] kvmtool: Makefile: allow overriding CC and LD

[Andre Przywara]

Currently we set CC unconditionally to ${CROSS_COMPILE}gcc, the same
for LD.
Allow people to override the compiler name by specifying it explicitly
on the command line or via the environment.
Beside calling a certain compiler binary this allows to pass in
options to the compiler, which lets us get rid of the PowerPC
overrides in the Makefile. Possible uses:
$ make CC=gcc -m64 LD=ld -melf64ppc
(build kvmtool on a PowerPC toolchain defaulting to 32-bit)
$ make CC=gcc -m32 LD=ld -melf_i386
(build a 32-bit binary on a multilib-enabled x86-64 compiler)

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 Makefile | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 6110b8e..888bee5 100644
--- a/Makefile
+++ b/Makefile
@@ -14,9 +14,13 @@ export E Q
 include config/utilities.mak
 include config/feature-tests.mak
 
-CC := $(CROSS_COMPILE)gcc
+ifeq ($(origin CC), default)
+   CC  := $(CROSS_COMPILE)gcc
+endif
 CFLAGS :=
-LD := $(CROSS_COMPILE)ld
+ifeq ($(origin LD), default)
+   LD  := $(CROSS_COMPILE)ld
+endif
 LDFLAGS:=
 
 FIND   := find
@@ -148,8 +152,6 @@ ifeq ($(ARCH), powerpc)
OBJS+= powerpc/spapr_pci.o
OBJS+= powerpc/xics.o
ARCH_INCLUDE := powerpc/include
-   CFLAGS  += -m64
-   LDFLAGS += -m elf64ppc
 
ARCH_WANT_LIBFDT := y
 endif
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] powerpc: add hvcall.h header from Linux

[Andre Przywara]

The powerpc code uses some PAPR hypercalls, of which we need the
hypercall number. Copy just the needed macro definitions from the
kernel's (private) hvcall.h file and remove the extra tricks formerly
used to be able to include this header file directly.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
Hi,

this version of the header file just contains the definitions we
need, while still being easily diff-able against the original file.
Please consider applying this one.

Cheers,
Andre.

 powerpc/include/asm/hvcall.h | 33 +
 powerpc/spapr.h  |  3 ---
 2 files changed, 33 insertions(+), 3 deletions(-)
 create mode 100644 powerpc/include/asm/hvcall.h

diff --git a/powerpc/include/asm/hvcall.h b/powerpc/include/asm/hvcall.h
new file mode 100644
index 000..9d58f9b
--- /dev/null
+++ b/powerpc/include/asm/hvcall.h
@@ -0,0 +1,33 @@
+#ifndef _ASM_POWERPC_HVCALL_H
+#define _ASM_POWERPC_HVCALL_H
+
+/* This file is a trimmed-down version of arch/powerpc/include/asm/hvcall.h. */
+
+#define H_SUCCESS  0
+
+#define H_HARDWARE -1  /* Hardware error */
+#define H_FUNCTION -2  /* Function not supported */
+#define H_PRIVILEGE-3  /* Caller not privileged */
+#define H_PARAMETER-4  /* Parameter invalid, out-of-range or 
conflicting */
+
+#define H_SET_DABR 0x28
+#define H_LOGICAL_CI_LOAD  0x3c
+#define H_LOGICAL_CI_STORE 0x40
+#define H_LOGICAL_CACHE_LOAD   0x44
+#define H_LOGICAL_CACHE_STORE  0x48
+#define H_LOGICAL_ICBI 0x4c
+#define H_LOGICAL_DCBF 0x50
+
+#define H_GET_TERM_CHAR0x54
+#define H_PUT_TERM_CHAR0x58
+
+#define H_EOI  0x64
+#define H_CPPR 0x68
+#define H_IPI  0x6c
+#define H_IPOLL0x70
+#define H_XIRR 0x74
+
+#define H_SET_MODE 0x31C
+#define MAX_HCALL_OPCODE   H_SET_MODE
+
+#endif /* _ASM_POWERPC_HVCALL_H */
diff --git a/powerpc/spapr.h b/powerpc/spapr.h
index 0537f88..4c6e349 100644
--- a/powerpc/spapr.h
+++ b/powerpc/spapr.h
@@ -16,10 +16,7 @@
 
 #include inttypes.h
 
-/* We need some of the H_ hcall defs, but they're __KERNEL__ only. */
-#define __KERNEL__
 #include asm/hvcall.h
-#undef __KERNEL__
 
 #include kvm/kvm.h
 #include kvm/kvm-cpu.h
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] kvmtool: fixes for PowerPC

[Andre Przywara]

Hello,

some patches to fix at least the build of the new kvmtool for
PowerPC. I could only compile test it so far, so I'd be grateful
if people more familiar with that architecture can have a look
and maybe even test it on actual machines.

Cheers,
Andre.

Andre Przywara (3):
  powerpc: implement barrier primitives
  powerpc: use default endianness for converting guest/init
  powerpc: add hvcall.h header from Linux

 Makefile  |   1 -
 powerpc/include/asm/hvcall.h  | 287 ++
 powerpc/include/kvm/barrier.h |   4 +-
 powerpc/spapr.h   |   3 -
 4 files changed, 290 insertions(+), 5 deletions(-)
 create mode 100644 powerpc/include/asm/hvcall.h

-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] powerpc: use default endianness for converting guest/init

[Andre Przywara]

For converting the guest/init binary into an object file, we call
the linker binary, setting the endianness to big endian explicitly
when compiling kvmtool for powerpc.
This breaks if the compiler is actually targetting little endian
(which is true for the Debian port, for instance).
Remove the explicit big endianness switch from the linker call to
allow linking on little endian PowerPC builds again.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
Hi,

this fixed the powerpc64le build for me, while still compiling fine
for big endian. Admittedly this whole init-guest_init.o conversion
has its issues (with MIPS, for instance), which deserve proper fixing,
but lets just fix that build for now.

Andre.

 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 6110b8e..c118e1a 100644
--- a/Makefile
+++ b/Makefile
@@ -149,7 +149,6 @@ ifeq ($(ARCH), powerpc)
OBJS+= powerpc/xics.o
ARCH_INCLUDE := powerpc/include
CFLAGS  += -m64
-   LDFLAGS += -m elf64ppc
 
ARCH_WANT_LIBFDT := y
 endif
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] powerpc: add hvcall.h header from Linux

[Andre Przywara]

The powerpc code uses some PAPR hypercalls, of which we need the
hypercall number. Copy the macro definition parts from the kernel's
(private) hvcall.h file and remove the extra tricks formerly used
to be able to include this header file directly.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
Hi,

I copied most of the Linux header, without removing
definitions that kvmtool doesn't use. That should make updates
easier. If people would prefer a bespoke header, let me know.

Andre.

 powerpc/include/asm/hvcall.h | 287 +++
 powerpc/spapr.h  |   3 -
 2 files changed, 287 insertions(+), 3 deletions(-)
 create mode 100644 powerpc/include/asm/hvcall.h

diff --git a/powerpc/include/asm/hvcall.h b/powerpc/include/asm/hvcall.h
new file mode 100644
index 000..b6dc250
--- /dev/null
+++ b/powerpc/include/asm/hvcall.h
@@ -0,0 +1,287 @@
+#ifndef _ASM_POWERPC_HVCALL_H
+#define _ASM_POWERPC_HVCALL_H
+
+#define HVSC   .long 0x4422
+
+#define H_SUCCESS  0
+#define H_BUSY 1   /* Hardware busy -- retry later */
+#define H_CLOSED   2   /* Resource closed */
+#define H_NOT_AVAILABLE 3
+#define H_CONSTRAINED  4   /* Resource request constrained to max allowed 
*/
+#define H_PARTIAL   5
+#define H_IN_PROGRESS  14  /* Kind of like busy */
+#define H_PAGE_REGISTERED 15
+#define H_PARTIAL_STORE   16
+#define H_PENDING  17  /* returned from H_POLL_PENDING */
+#define H_CONTINUE 18  /* Returned from H_Join on success */
+#define H_LONG_BUSY_START_RANGE9900  /* Start of long busy 
range */
+#define H_LONG_BUSY_ORDER_1_MSEC   9900  /* Long busy, hint that 1msec \
+is a good time to retry */
+#define H_LONG_BUSY_ORDER_10_MSEC  9901  /* Long busy, hint that 10msec \
+is a good time to retry */
+#define H_LONG_BUSY_ORDER_100_MSEC 9902  /* Long busy, hint that 100msec \
+is a good time to retry */
+#define H_LONG_BUSY_ORDER_1_SEC9903  /* Long busy, hint that 
1sec \
+is a good time to retry */
+#define H_LONG_BUSY_ORDER_10_SEC   9904  /* Long busy, hint that 10sec \
+is a good time to retry */
+#define H_LONG_BUSY_ORDER_100_SEC  9905  /* Long busy, hint that 100sec \
+is a good time to retry */
+#define H_LONG_BUSY_END_RANGE  9905  /* End of long busy range */
+
+/* Internal value used in book3s_hv kvm support; not returned to guests */
+#define H_TOO_HARD 
+
+#define H_HARDWARE -1  /* Hardware error */
+#define H_FUNCTION -2  /* Function not supported */
+#define H_PRIVILEGE-3  /* Caller not privileged */
+#define H_PARAMETER-4  /* Parameter invalid, out-of-range or 
conflicting */
+#define H_BAD_MODE -5  /* Illegal msr value */
+#define H_PTEG_FULL-6  /* PTEG is full */
+#define H_NOT_FOUND-7  /* PTE was not found */
+#define H_RESERVED_DABR-8  /* DABR address is reserved by the 
hypervisor on this processor */
+#define H_NO_MEM   -9
+#define H_AUTHORITY-10
+#define H_PERMISSION   -11
+#define H_DROPPED  -12
+#define H_SOURCE_PARM  -13
+#define H_DEST_PARM-14
+#define H_REMOTE_PARM  -15
+#define H_RESOURCE -16
+#define H_ADAPTER_PARM  -17
+#define H_RH_PARM   -18
+#define H_RCQ_PARM  -19
+#define H_SCQ_PARM  -20
+#define H_EQ_PARM   -21
+#define H_RT_PARM   -22
+#define H_ST_PARM   -23
+#define H_SIGT_PARM -24
+#define H_TOKEN_PARM-25
+#define H_MLENGTH_PARM  -27
+#define H_MEM_PARM  -28
+#define H_MEM_ACCESS_PARM -29
+#define H_ATTR_PARM -30
+#define H_PORT_PARM -31
+#define H_MCG_PARM  -32
+#define H_VL_PARM   -33
+#define H_TSIZE_PARM-34
+#define H_TRACE_PARM-35
+
+#define H_MASK_PARM -37
+#define H_MCG_FULL  -38
+#define H_ALIAS_EXIST   -39
+#define H_P_COUNTER -40
+#define H_TABLE_FULL-41
+#define H_ALT_TABLE -42
+#define H_MR_CONDITION  -43
+#define H_NOT_ENOUGH_RESOURCES -44
+#define H_R_STATE   -45
+#define H_RESCINDED -46
+#define H_P2   -55
+#define H_P3   -56
+#define H_P4   -57
+#define H_P5   -58
+#define H_P6   -59
+#define H_P7   -60
+#define H_P8   -61
+#define H_P9   -62
+#define H_TOO_BIG  -64
+#define H_OVERLAP  -68
+#define H_INTERRUPT-69
+#define H_BAD_DATA -70
+#define H_NOT_ACTIVE   -71
+#define H_SG_LIST  -72
+#define H_OP_MODE  -73
+#define H_COP_HW   -74
+#define H_UNSUPPORTED_FLAG_START   -256
+#define H_UNSUPPORTED_FLAG_END -511
+#define H_MULTI_THREADS_ACTIVE -9005
+#define H_OUTSTANDING_COP_OPS  -9006
+
+
+/* Long Busy is a condition that can