[Qemu-devel] [RFC PATCH 0/3] Make kvm-unit-tests more friendly to upstream QEMU

[Qemu-devel] [RFC PATCH 0/3] Make kvm-unit-tests more friendly to upstream QEMU

[Anthony Liguori]

This series makes an attempt to make kvm-unit-tests more friendly to upstream
QEMU.  I've been writing unit tests for all of the QMP commands and many of
them, like ballooning, require guest cooperation to be tested in a meaningful
way.

I'm leaning towards building simple guests using libcflat in order to do this
using some very simple PCI device drivers.  To get started, I made a few changes
to kvm-unit-tests to make them more friendly to QEMU upstream basically by
using platform devices instead of relying on special test devices.

I'm not sure this is the right path to go yet but I thought I'd share anyway.

[Qemu-devel] [PATCH 1/3] kvm-unit-tests: add x86 port io accessors

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/lib/x86/io.h b/lib/x86/io.h
new file mode 100644
index 0000000..bd6341c
--- /dev/null
+++ b/lib/x86/io.h
@@ -0,0 +1,40 @@
+#ifndef IO_H
+#define IO_H
+
+static inline unsigned char inb(unsigned short port)
+{
+    unsigned char value;
+    asm volatile("inb %w1, %0" : "=a" (value) : "Nd" (port));
+    return value;
+}
+
+static inline unsigned short inw(unsigned short port)
+{
+    unsigned short value;
+    asm volatile("inw %w1, %0" : "=a" (value) : "Nd" (port));
+    return value;
+}
+
+static inline unsigned int inl(unsigned short port)
+{
+    unsigned int value;
+    asm volatile("inl %w1, %0" : "=a" (value) : "Nd" (port));
+    return value;
+}
+
+static inline void outb(unsigned char value, unsigned short port)
+{
+    asm volatile("outb %b0, %w1" : : "a"(value), "Nd"(port));
+}
+
+static inline void outw(unsigned short value, unsigned short port)
+{
+    asm volatile("outw %w0, %w1" : : "a"(value), "Nd"(port));
+}
+
+static inline void outl(unsigned int value, unsigned short port)
+{
+    asm volatile("outl %0, %w1" : : "a"(value), "Nd"(port));
+}
+
+#endif
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 1/3] kvm-unit-tests: add x86 port io accessors

[Avi Kivity]

On 02/24/2011 11:48 PM, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> diff --git a/lib/x86/io.h b/lib/x86/io.h
> new file mode 100644
> index 0000000..bd6341c
> --- /dev/null
> +++ b/lib/x86/io.h
> @@ -0,0 +1,40 @@
> +#ifndef IO_H
> +#define IO_H
> +
> +static inline unsigned char inb(unsigned short port)
> +{
> +    unsigned char value;
> +    asm volatile("inb %w1, %0" : "=a" (value) : "Nd" (port));
> +    return value;
> +}

Are those %[wb] really needed?  gcc should do the right thing based on 
the argument type.

Might as well put all of that into processor.h.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 1/3] kvm-unit-tests: add x86 port io accessors

[Anthony Liguori]

On 02/27/2011 06:44 AM, Avi Kivity wrote:
> On 02/24/2011 11:48 PM, Anthony Liguori wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/lib/x86/io.h b/lib/x86/io.h
>> new file mode 100644
>> index 0000000..bd6341c
>> --- /dev/null
>> +++ b/lib/x86/io.h
>> @@ -0,0 +1,40 @@
>> +#ifndef IO_H
>> +#define IO_H
>> +
>> +static inline unsigned char inb(unsigned short port)
>> +{
>> +    unsigned char value;
>> +    asm volatile("inb %w1, %0" : "=a" (value) : "Nd" (port));
>> +    return value;
>> +}
>
> Are those %[wb] really needed?  gcc should do the right thing based on 
> the argument type.

It's just a little extra type safety.

>
> Might as well put all of that into processor.h.

Seems reasonable.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 1/3] kvm-unit-tests: add x86 port io accessors

[Avi Kivity]

On 02/27/2011 04:01 PM, Anthony Liguori wrote:
> On 02/27/2011 06:44 AM, Avi Kivity wrote:
>> On 02/24/2011 11:48 PM, Anthony Liguori wrote:
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> diff --git a/lib/x86/io.h b/lib/x86/io.h
>>> new file mode 100644
>>> index 0000000..bd6341c
>>> --- /dev/null
>>> +++ b/lib/x86/io.h
>>> @@ -0,0 +1,40 @@
>>> +#ifndef IO_H
>>> +#define IO_H
>>> +
>>> +static inline unsigned char inb(unsigned short port)
>>> +{
>>> +    unsigned char value;
>>> +    asm volatile("inb %w1, %0" : "=a" (value) : "Nd" (port));
>>> +    return value;
>>> +}
>>
>> Are those %[wb] really needed?  gcc should do the right thing based 
>> on the argument type.
>
> It's just a little extra type safety.

Yeah, but those constraints aren't really documented.  Linux does use 
them in a handful of places so they're unlikely to go away though.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 2/3] kvm-unit-tests: do not set level sensitive irq when initializing the PIC

[Anthony Liguori]

I'm not sure if this was intentional but the QEMU i8259 does not support this
flag.  I haven't observed any issues with this but I'll happily admit that
I'm not very aware of what I'm doing here.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/lib/x86/apic.c b/lib/x86/apic.c
index 7bb98ed..2c2afb7 100644
--- a/lib/x86/apic.c
+++ b/lib/x86/apic.c
@@ -1,5 +1,6 @@
 #include "libcflat.h"
 #include "apic.h"
+#include "io.h"
 
 static void *g_apic = (void *)0xfee00000;
 static void *g_ioapic = (void *)0xfec00000;
@@ -11,11 +12,6 @@ struct apic_ops {
     u32 (*id)(void);
 };
 
-static void outb(unsigned char data, unsigned short port)
-{
-    asm volatile ("out %0, %1" : : "a"(data), "d"(port));
-}
-
 static u32 xapic_read(unsigned reg)
 {
     return *(volatile u32 *)(g_apic + reg);
@@ -133,7 +129,7 @@ void ioapic_write_redir(unsigned line, ioapic_redir_entry_t e)
 void enable_apic(void)
 {
     printf("enabling apic\n");
-    xapic_write(0xf0, 0x1ff); /* spurious vector register */
+    xapic_write(0xf0, 0x1f7); /* spurious vector register */
 }
 
 void mask_pic_interrupts(void)
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 2/3] kvm-unit-tests: do not set level sensitive irq when initializing the PIC

[Avi Kivity]

On 02/24/2011 11:48 PM, Anthony Liguori wrote:
> I'm not sure if this was intentional but the QEMU i8259 does not support this
> flag.  I haven't observed any issues with this but I'll happily admit that
> I'm not very aware of what I'm doing here.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
>   static u32 xapic_read(unsigned reg)
>   {
>       return *(volatile u32 *)(g_apic + reg);
> @@ -133,7 +129,7 @@ void ioapic_write_redir(unsigned line, ioapic_redir_entry_t e)
>   void enable_apic(void)
>   {
>       printf("enabling apic\n");
> -    xapic_write(0xf0, 0x1ff); /* spurious vector register */
> +    xapic_write(0xf0, 0x1f7); /* spurious vector register */
>   }

Not sure what you're doing here.  You're changing the APIC Spurious 
Vector from 0xff to 0xf7?  This has nothing to do with the i8259 or 
level triggeredness as far as I can tell - it just enables the APIC (bit 
8) and selects a vector for reporting spurious interrupts (0xff happens 
to be the reset value).

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 3/3] kvm-unit-tests: make I/O more friendly to existing QEMU hardware

[Anthony Liguori]

Use the serial port for printf() and use the Bochs bios exit port if the
testdev port isn't available.

This unconditionally switches to use the serial port but tries to use the
testdev exit port since that lets you pass an exit status.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/lib/x86/io.c b/lib/x86/io.c
index 894f398..edc9437 100644
--- a/lib/x86/io.c
+++ b/lib/x86/io.c
@@ -1,13 +1,53 @@
 #include "libcflat.h"
 #include "smp.h"
+#include "io.h"
 
 static struct spinlock lock;
+static int serial_iobase = 0x3f8;
+static int serial_inited = 0;
+
+static void serial_outb(char ch)
+{
+        u8 lsr;
+
+        do {
+                lsr = inb(serial_iobase + 0x05);
+        } while (!(lsr & 0x20));
+
+        outb(ch, serial_iobase + 0x00);
+}
+
+static void serial_init(void)
+{
+        u8 lcr;
+
+        /* set DLAB */
+        lcr = inb(serial_iobase + 0x03);
+        lcr |= 0x80;
+        outb(lcr, serial_iobase + 0x03);
+
+        /* set baud rate to 115200 */
+        outb(0x01, serial_iobase + 0x00);
+        outb(0x00, serial_iobase + 0x01);
+
+        /* clear DLAB */
+        lcr = inb(serial_iobase + 0x03);
+        lcr &= ~0x80;
+        outb(lcr, serial_iobase + 0x03);
+}
 
 static void print_serial(const char *buf)
 {
 	unsigned long len = strlen(buf);
+        unsigned long i;
 
-	asm volatile ("rep/outsb" : "+S"(buf), "+c"(len) : "d"(0xf1));
+        if (!serial_inited) {
+            serial_init();
+        }
+
+        for (i = 0; i < len; i++) {
+            serial_outb(buf[i]);
+        }
 }
 
 void puts(const char *s)
@@ -19,5 +59,14 @@ void puts(const char *s)
 
 void exit(int code)
 {
-        asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
+        static const char shutdown_str[8] = "Shutdown";
+        int i;
+
+        /* test device exit (with status) */
+        outl(code, 0xf4);
+
+        /* if that failed, try the Bochs poweroff port */
+        for (i = 0; i < 8; i++) {
+                outb(shutdown_str[i], 0x8900);
+        }
 }
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 3/3] kvm-unit-tests: make I/O more friendly to existing QEMU hardware

[Avi Kivity]

On 02/24/2011 11:48 PM, Anthony Liguori wrote:
> Use the serial port for printf() and use the Bochs bios exit port if the
> testdev port isn't available.
>
> This unconditionally switches to use the serial port but tries to use the
> testdev exit port since that lets you pass an exit status.

The only issue I can see is that the serial port is much slower than the 
testdev port (since we can use rep outsb there).  That only matters for 
access.flat in full output mode (not default).  So please keep the old 
code under #if 0 so we can easily switch back to it.

-- 
error compiling committee.c: too many arguments to function