[PATCH 0/4] kvmtool: ARM: fixing initrd, serial IRQs and bzImage message

[PATCH 0/4] kvmtool: ARM: fixing initrd, serial IRQs and bzImage message

[Andre Przywara]

Hi,

dumping a part of my kvmtool patch queue here.

Patch 1 (fixing initrd for ARM) was already on the list, but hasn't
been merged yet.
Patch 2 and 3 let kvmtool describe the 8250 serial IRQs as level
triggered and fix a stability problem.
Patch 4 is an easy fix for a message that annoys me every time ;-)

Cheers,
Andre.

Andre Przywara (4):
  kvmtool: ARM: fix initrd functionality
  kvmtool: ARM: allow level interrupts in device tree
  kvmtool: ARM: advertise 8250 IRQs as level-triggered
  kvmtool: remove warning about bzImage on non-x86 architectures

 tools/kvm/arm/fdt.c            |   10 +++++-----
 tools/kvm/hw/serial.c          |    5 +++--
 tools/kvm/include/kvm/ioport.h |    4 +++-
 tools/kvm/ioport.c             |    6 ++++--
 tools/kvm/kvm.c                |    5 ++++-
 tools/kvm/virtio/mmio.c        |    5 +++--
 6 files changed, 22 insertions(+), 13 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] kvmtool: ARM: fix initrd functionality

[Andre Przywara]

lkvm -i is currently broken on ARM/ARM64.
We should not try to convert smaller-than-4GB addresses into 64-bit
big endian and then stuff them into u32 variables if we expect to read
anything other than 0 out of it.
Adjust the type to u64 to write the proper address in BE format into
the /chosen node (and also match the address size we formely posted)
and let Linux thus read the right values.
This fixes initrd functionality for ARM and ARM64 guests.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 tools/kvm/arm/fdt.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c
index 5626931..4a33846 100644
--- a/tools/kvm/arm/fdt.c
+++ b/tools/kvm/arm/fdt.c
@@ -149,8 +149,8 @@ static int setup_fdt(struct kvm *kvm)
 
 	/* Initrd */
 	if (kvm->arch.initrd_size != 0) {
-		u32 ird_st_prop = cpu_to_fdt64(kvm->arch.initrd_guest_start);
-		u32 ird_end_prop = cpu_to_fdt64(kvm->arch.initrd_guest_start +
+		u64 ird_st_prop = cpu_to_fdt64(kvm->arch.initrd_guest_start);
+		u64 ird_end_prop = cpu_to_fdt64(kvm->arch.initrd_guest_start +
 					       kvm->arch.initrd_size);
 
 		_FDT(fdt_property(fdt, "linux,initrd-start",
-- 
1.7.9.5

[PATCH 2/4] kvmtool: ARM: allow level interrupts in device tree

[Andre Przywara]

Currently we describe every interrupt for each device in the FDT
as being edge triggered.
Add a parameter to the irq property generation to allow devices to
specify their interrupts as level triggered if needed.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/kvm/arm/fdt.c            |    6 +++---
 tools/kvm/hw/serial.c          |    5 +++--
 tools/kvm/include/kvm/ioport.h |    4 +++-
 tools/kvm/ioport.c             |    6 ++++--
 tools/kvm/virtio/mmio.c        |    5 +++--
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c
index 4a33846..918d89f 100644
--- a/tools/kvm/arm/fdt.c
+++ b/tools/kvm/arm/fdt.c
@@ -74,12 +74,12 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
 	_FDT(fdt_end_node(fdt));
 }
 
-static void generate_irq_prop(void *fdt, u8 irq)
+static void generate_irq_prop(void *fdt, u8 irq, u32 irq_type)
 {
 	u32 irq_prop[] = {
 		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_SPI),
 		cpu_to_fdt32(irq - GIC_SPI_IRQ_BASE),
-		cpu_to_fdt32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI),
+		cpu_to_fdt32(irq_type)
 	};
 
 	_FDT(fdt_property(fdt, "interrupts", irq_prop, sizeof(irq_prop)));
@@ -127,7 +127,7 @@ static int setup_fdt(struct kvm *kvm)
 	void *fdt_dest		= guest_flat_to_host(kvm,
 						     kvm->arch.dtb_guest_start);
 	void (*generate_mmio_fdt_nodes)(void *, struct device_header *,
-					void (*)(void *, u8));
+					void (*)(void *, u8, u32));
 	void (*generate_cpu_peripheral_fdt_nodes)(void *, struct kvm *, u32)
 					= kvm->cpus[0]->generate_fdt_nodes;
 
diff --git a/tools/kvm/hw/serial.c b/tools/kvm/hw/serial.c
index 60147de..266863b 100644
--- a/tools/kvm/hw/serial.c
+++ b/tools/kvm/hw/serial.c
@@ -369,7 +369,8 @@ static bool serial8250_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port,
 #define DEVICE_NAME_MAX_LEN 32
 static void serial8250_generate_fdt_node(struct ioport *ioport, void *fdt,
 					 void (*generate_irq_prop)(void *fdt,
-								   u8 irq))
+								   u8 irq,
+								   u32 type))
 {
 	char dev_name[DEVICE_NAME_MAX_LEN];
 	struct serial8250_device *dev = ioport->priv;
@@ -384,7 +385,7 @@ static void serial8250_generate_fdt_node(struct ioport *ioport, void *fdt,
 	_FDT(fdt_begin_node(fdt, dev_name));
 	_FDT(fdt_property_string(fdt, "compatible", "ns16550a"));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
-	generate_irq_prop(fdt, dev->irq);
+	generate_irq_prop(fdt, dev->irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
 	_FDT(fdt_property_cell(fdt, "clock-frequency", 1843200));
 	_FDT(fdt_end_node(fdt));
 }
diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h
index 18da47c..ffd938d 100644
--- a/tools/kvm/include/kvm/ioport.h
+++ b/tools/kvm/include/kvm/ioport.h
@@ -31,7 +31,9 @@ struct ioport_operations {
 	bool (*io_in)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size);
 	bool (*io_out)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size);
 	void (*generate_fdt_node)(struct ioport *ioport, void *fdt,
-				  void (*generate_irq_prop)(void *fdt, u8 irq));
+				  void (*generate_irq_prop)(void *fdt,
+							    u8 irq,
+							    u32 irq_type));
 };
 
 void ioport__setup_arch(struct kvm *kvm);
diff --git a/tools/kvm/ioport.c b/tools/kvm/ioport.c
index 5bfb7e2..b507cab 100644
--- a/tools/kvm/ioport.c
+++ b/tools/kvm/ioport.c
@@ -59,7 +59,8 @@ static void ioport_remove(struct rb_root *root, struct ioport *data)
 static void generate_ioport_fdt_node(void *fdt,
 				     struct device_header *dev_hdr,
 				     void (*generate_irq_prop)(void *fdt,
-							       u8 irq))
+							       u8 irq,
+							       u32 irq_type))
 {
 	struct ioport *ioport = container_of(dev_hdr, struct ioport, dev_hdr);
 	struct ioport_operations *ops = ioport->ops;
@@ -71,7 +72,8 @@ static void generate_ioport_fdt_node(void *fdt,
 static void generate_ioport_fdt_node(void *fdt,
 				     struct device_header *dev_hdr,
 				     void (*generate_irq_prop)(void *fdt,
-							       u8 irq))
+							       u8 irq,
+							       u32 irq_type))
 {
 	die("Unable to generate device tree nodes without libfdt\n");
 }
diff --git a/tools/kvm/virtio/mmio.c b/tools/kvm/virtio/mmio.c
index 3a2bd62..28b0651 100644
--- a/tools/kvm/virtio/mmio.c
+++ b/tools/kvm/virtio/mmio.c
@@ -233,7 +233,8 @@ static void virtio_mmio_mmio_callback(struct kvm_cpu *vcpu,
 static void generate_virtio_mmio_fdt_node(void *fdt,
 					  struct device_header *dev_hdr,
 					  void (*generate_irq_prop)(void *fdt,
-								    u8 irq))
+								    u8 irq,
+								    u32 type))
 {
 	char dev_name[DEVICE_NAME_MAX_LEN];
 	struct virtio_mmio *vmmio = container_of(dev_hdr,
@@ -250,7 +251,7 @@ static void generate_virtio_mmio_fdt_node(void *fdt,
 	_FDT(fdt_begin_node(fdt, dev_name));
 	_FDT(fdt_property_string(fdt, "compatible", "virtio,mmio"));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
-	generate_irq_prop(fdt, vmmio->irq);
+	generate_irq_prop(fdt, vmmio->irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
 	_FDT(fdt_end_node(fdt));
 }
 #else
-- 
1.7.9.5

[PATCH 3/4] kvmtool: ARM: advertise 8250 IRQs as level-triggered

[Andre Przywara]

Both the 16550/8250 UART emulation in kvmtool as well as all the
drivers and DTBs for real hardware use level triggered interrupts.
But the device tree currently describes them as being edge triggered,
which can lead to hangs in guests.
Use the new IRQ type parameter to properly describe the interrupts.
This goes along the lines of a similar QEMU patch:
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=0be969a2d974971628fc4ed95834d22ecf0fd497

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/kvm/hw/serial.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kvm/hw/serial.c b/tools/kvm/hw/serial.c
index 266863b..f223802 100644
--- a/tools/kvm/hw/serial.c
+++ b/tools/kvm/hw/serial.c
@@ -385,7 +385,7 @@ static void serial8250_generate_fdt_node(struct ioport *ioport, void *fdt,
 	_FDT(fdt_begin_node(fdt, dev_name));
 	_FDT(fdt_property_string(fdt, "compatible", "ns16550a"));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
-	generate_irq_prop(fdt, dev->irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+	generate_irq_prop(fdt, dev->irq, GIC_FDT_IRQ_FLAGS_LEVEL_HI);
 	_FDT(fdt_property_cell(fdt, "clock-frequency", 1843200));
 	_FDT(fdt_end_node(fdt));
 }
-- 
1.7.9.5

[PATCH 4/4] kvmtool: remove warning about bzImage on non-x86 architectures

[Andre Przywara]

Among the architectures supported by kvmtool, only x86 defines a
bzImage format. So we shouldn't bother users of other architectures
with a message about something that cannot work.
Make the bzImage check dependent on compiling for x86.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/kvm/kvm.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
index e1b9f6c..21817c3 100644
--- a/tools/kvm/kvm.c
+++ b/tools/kvm/kvm.c
@@ -380,12 +380,15 @@ 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);
+	pr_warning("%s is not a bzImage. Trying to load it as an ELF or flat binary...",
+		   kernel_filename);
+#endif
 
 	ret = load_elf_binary(kvm, fd_kernel, fd_initrd, kernel_cmdline);
 
-- 
1.7.9.5

Re: [PATCH 2/4] kvmtool: ARM: allow level interrupts in device tree

[Will Deacon]

On Thu, Dec 11, 2014 at 04:30:33PM +0000, Andre Przywara wrote:
> Currently we describe every interrupt for each device in the FDT
> as being edge triggered.
> Add a parameter to the irq property generation to allow devices to
> specify their interrupts as level triggered if needed.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  tools/kvm/arm/fdt.c            |    6 +++---
>  tools/kvm/hw/serial.c          |    5 +++--
>  tools/kvm/include/kvm/ioport.h |    4 +++-
>  tools/kvm/ioport.c             |    6 ++++--
>  tools/kvm/virtio/mmio.c        |    5 +++--
>  5 files changed, 16 insertions(+), 10 deletions(-)

[...]

> @@ -71,7 +72,8 @@ static void generate_ioport_fdt_node(void *fdt,
>  static void generate_ioport_fdt_node(void *fdt,
>  				     struct device_header *dev_hdr,
>  				     void (*generate_irq_prop)(void *fdt,
> -							       u8 irq))
> +							       u8 irq,
> +							       u32 irq_type))
>  {
>  	die("Unable to generate device tree nodes without libfdt\n");
>  }
> diff --git a/tools/kvm/virtio/mmio.c b/tools/kvm/virtio/mmio.c
> index 3a2bd62..28b0651 100644
> --- a/tools/kvm/virtio/mmio.c
> +++ b/tools/kvm/virtio/mmio.c
> @@ -233,7 +233,8 @@ static void virtio_mmio_mmio_callback(struct kvm_cpu *vcpu,
>  static void generate_virtio_mmio_fdt_node(void *fdt,
>  					  struct device_header *dev_hdr,
>  					  void (*generate_irq_prop)(void *fdt,
> -								    u8 irq))
> +								    u8 irq,
> +								    u32 type))
>  {
>  	char dev_name[DEVICE_NAME_MAX_LEN];
>  	struct virtio_mmio *vmmio = container_of(dev_hdr,
> @@ -250,7 +251,7 @@ static void generate_virtio_mmio_fdt_node(void *fdt,
>  	_FDT(fdt_begin_node(fdt, dev_name));
>  	_FDT(fdt_property_string(fdt, "compatible", "virtio,mmio"));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> -	generate_irq_prop(fdt, vmmio->irq);
> +	generate_irq_prop(fdt, vmmio->irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);

This is a GIC-specific #define in arch-agnostic code. I think we should have
a new enum type for describing edge and level interrupts, then just use
that instead.

Will

Re: [PATCH 2/4] kvmtool: ARM: allow level interrupts in device tree

[Andre Przywara]

Hi,

On 11/12/14 17:15, Will Deacon wrote:
> On Thu, Dec 11, 2014 at 04:30:33PM +0000, Andre Przywara wrote:
>> Currently we describe every interrupt for each device in the FDT
>> as being edge triggered.
>> Add a parameter to the irq property generation to allow devices to
>> specify their interrupts as level triggered if needed.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  tools/kvm/arm/fdt.c            |    6 +++---
>>  tools/kvm/hw/serial.c          |    5 +++--
>>  tools/kvm/include/kvm/ioport.h |    4 +++-
>>  tools/kvm/ioport.c             |    6 ++++--
>>  tools/kvm/virtio/mmio.c        |    5 +++--
>>  5 files changed, 16 insertions(+), 10 deletions(-)
> 
> [...]
> 
>> @@ -71,7 +72,8 @@ static void generate_ioport_fdt_node(void *fdt,
>>  static void generate_ioport_fdt_node(void *fdt,
>>  				     struct device_header *dev_hdr,
>>  				     void (*generate_irq_prop)(void *fdt,
>> -							       u8 irq))
>> +							       u8 irq,
>> +							       u32 irq_type))
>>  {
>>  	die("Unable to generate device tree nodes without libfdt\n");
>>  }
>> diff --git a/tools/kvm/virtio/mmio.c b/tools/kvm/virtio/mmio.c
>> index 3a2bd62..28b0651 100644
>> --- a/tools/kvm/virtio/mmio.c
>> +++ b/tools/kvm/virtio/mmio.c
>> @@ -233,7 +233,8 @@ static void virtio_mmio_mmio_callback(struct kvm_cpu *vcpu,
>>  static void generate_virtio_mmio_fdt_node(void *fdt,
>>  					  struct device_header *dev_hdr,
>>  					  void (*generate_irq_prop)(void *fdt,
>> -								    u8 irq))
>> +								    u8 irq,
>> +								    u32 type))
>>  {
>>  	char dev_name[DEVICE_NAME_MAX_LEN];
>>  	struct virtio_mmio *vmmio = container_of(dev_hdr,
>> @@ -250,7 +251,7 @@ static void generate_virtio_mmio_fdt_node(void *fdt,
>>  	_FDT(fdt_begin_node(fdt, dev_name));
>>  	_FDT(fdt_property_string(fdt, "compatible", "virtio,mmio"));
>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>> -	generate_irq_prop(fdt, vmmio->irq);
>> +	generate_irq_prop(fdt, vmmio->irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> 
> This is a GIC-specific #define in arch-agnostic code. I think we should have
> a new enum type for describing edge and level interrupts, then just use
> that instead.

Right, good point.
I dug a bit in the kernel source, and actually those values behind the
defines are not GIC specific, but are generic IRQ FDT values (used by
other architectures as well). Even the GIC driver source uses these
defines, it's just the GICv3 binding doc that speaks of those specific
numbers.
So if you don't mind, I will remove the GIC_FDT_IRQ_FLAGS_{LEVEL,EDGE}
defines from kvmtool's headers and use the generic defines from the
kernel in one of the fdt headers.

Cheers,
Andre.