Re: [Qemu-devel] [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread Stefan Weil
Jamie Lokier schrieb:
> Stefan Weil wrote:
>   
>> Make using mingw32 on windows does not preserve $@ in macros
>> when they are modified using this pattern:
>> target: macro += something
>> 
>
> Is it not using GNU Make, or is it some ancient version?
> ("make --version" shows the version, if it's GNU Make).
>
> -- Jamie
>
>   

GNU make 3.79.1 (2004)

-- Stefan




Re: [Qemu-devel] [PATCH 09/10] qdev: Use QError for 'device not found' error

2009-11-19 Thread Amit Shah
On (Wed) Nov 18 2009 [16:17:02], Markus Armbruster wrote:
> Luiz Capitulino  writes:
> 
> > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >  /* find driver */
> >  info = qdev_find_info(NULL, driver);
> >  if (!info) {
> > -qemu_error("Device \"%s\" not found.  Try -device '?' for a 
> > list.\n",
> > -   driver);
> > +qemu_error_new(QERR_DEVICE_NOT_FOUND, driver);
> >  return NULL;
> >  }
> >  if (info->no_user) {
> 
> Not obvious from this patch, but we lose the "Try -device '?' for a
> list" hint here.  In PATCH 7/10:

BTW that hint isn't always appropriate as it's printed on the monitor
when doing 'device_add' as well.

Amit




[Qemu-devel] Re: [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread Stefan Weil
malc schrieb:
> On Thu, 19 Nov 2009, Stefan Weil wrote:
>
>> Make using mingw32 on windows does not preserve $@ in macros
>> when they are modified using this pattern:
>> target: macro += something
>>
>> This behaviour results in an error when QEMU_CFLAGS containing
>> "-MMD -MP -MT $@" is modified for compilation of source files
>> which use SDL: $@ will expand to nothing, -MT no longer has
>> the correct argument (it will take the next one from the command
>> line) and the build will fail or run with a wrong command line.
>>
>> The problem is fixed by using a new macro QEMU_DGFLAGS
>> which is not modified by a target rule.
>
> Why not just stuff `-MMD -MP -MT $@' into the rules?
>
> [..snip..]

As in most cases, there are many ways to do something...

During my test, I had the dependency flags in the rules.

I decided to use a macro to allow people running make
without dependency generation (make QEMU_DGFLAGS=).

Stefan





Re: [Qemu-devel] [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread Jamie Lokier
Stefan Weil wrote:
> Make using mingw32 on windows does not preserve $@ in macros
> when they are modified using this pattern:
> target: macro += something

Is it not using GNU Make, or is it some ancient version?
("make --version" shows the version, if it's GNU Make).

-- Jamie




Re: [Qemu-devel] Re: virtio: Report new guest memory statistics pertinent to memory ballooning (V4)

2009-11-19 Thread Jamie Lokier
Adam Litke wrote:
> The precision for most of these stats (except major and minor faults)
> is 4kb (at least for Linux guests on the platforms I can think of).

Sparc and Alpha have 8kb pages (minimum).

The stats could be counted in 4kb pages, but what's the point in that?

-- Jamie




[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Rusty Russell
On Fri, 20 Nov 2009 01:49:05 am Adam Litke wrote:
> Rusty and Anthony,
> If I've addressed all outstanding issues, please consider this patch for
> inclusion.  Thanks.
> 
> Changes since V2:
>  - Increase stat field size to 64 bits
>  - Report all sizes in kb (not pages)

Hi Adam,

   Looks like we're very close.  A few minor things:

Why k?  Why not just do the simplest possible thing, and just report all
stats as straight numbers, now we have 64 bits.

>  - Drop anon_pages stat and fix endianness conversion

Please drop endianness conversion.

> +struct virtio_balloon_stat
> +{
> + __le16 tag;
> + __le64 val;
> +};

Let's not introduce padding as well; __attribute__((packed)) here please.

Thanks,
Rusty.




[Qemu-devel] Networking hangs

2009-11-19 Thread Christoffer Dall

Hi.

I am experiencing problems with bridged networking to ARM guests. I have 
experimented with various kernel versions, distributions and host 
machines and I experience the problem in all cases.


When I copy files into the guest using SCP, after an undeterministic 
number of megabytes have been copied, the network crashes. At this point 
not even pings to localhost works inside the guest.


I am using v0.11.0, emulating a versatilepb board with an arm1136-r2 cpu 
and using the following parameter for networking when running the system 
emulator:


-net nic,vlan=0 -net 
tap,vlan=0,ifname=tap0,script=$(QEMUIFUP),downscript=$(QEMUIFDOWN)


QEMUIFUP:
ifconfig tap0 192.168.7.1

QEMUIFDOWN:
ifconfig tap0 down

Any help is greatly appreciated.

Best,
Christoffer Dall




[Qemu-devel] Re: [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread malc
On Thu, 19 Nov 2009, Stefan Weil wrote:

> Make using mingw32 on windows does not preserve $@ in macros
> when they are modified using this pattern:
> target: macro += something
> 
> This behaviour results in an error when QEMU_CFLAGS containing
> "-MMD -MP -MT $@" is modified for compilation of source files
> which use SDL: $@ will expand to nothing, -MT no longer has
> the correct argument (it will take the next one from the command
> line) and the build will fail or run with a wrong command line.
> 
> The problem is fixed by using a new macro QEMU_DGFLAGS
> which is not modified by a target rule.

Why not just stuff `-MMD -MP -MT $@' into the rules?

[..snip..]

-- 
mailto:av1...@comtv.ru




[Qemu-devel] POST failure (loop) with isapc and seabios

2009-11-19 Thread Sebastian Herbszt

i386-softmmu/qemu -M isapc -bios pc-bios/bios.bin

(qemu) info cpus
* CPU #0: pc=0x000f3852
(qemu) x/20i $pc
0x000f3852:  mov0xfd408,%eax
0x000f3857:  cmp%eax,%edx
0x000f3859:  jne0xf3852
0x000f385b:  mov%ecx,0x1
0x000f3861:  mov%ebx,0x10004
0x000f3867:  xor%eax,%eax
0x000f3869:  cmpl   $0x0,0xf78ec
0x000f3870:  je 0xf3897
0x000f3872:  mov$0x510,%edx
0x000f3877:  mov$0xf,%eax
0x000f387c:  out%ax,(%dx)
0x000f387e:  mov$0x11,%dl
0x000f3880:  in (%dx),%al
0x000f3881:  mov%al,0xe4(%esp)
0x000f3888:  in (%dx),%al
0x000f3889:  mov%al,0xe5(%esp)
0x000f3890:  mov0xe4(%esp),%eax
0x000f3897:  movzwl %ax,%eax
0x000f389a:  mov%eax,0xfd40c
0x000f389f:  test   %eax,%eax

Could be smp_probe():

   // Wait for other CPUs to process the SIPI.
   if (CONFIG_COREBOOT) {
   msleep(10);
   } else {
   u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
   while (cmos_smp_count + 1 != readl(&CountCPUs))
;
   }

Works with pcbios.bin.

- Sebastian





[Qemu-devel] [PATCH] eepro100: Restructure code (new function tx_command)

2009-11-19 Thread Stefan Weil
Handling of transmit commands is rather complex,
so about 80 lines of code were moved from function
action_command to the new function tx_command.

The two new values "tx" and "cb_address" in the
eepro100 status structure made this possible without
passing too many parameters.

In addition, the moved code was cleaned a little bit:
old comments marked with //~ were removed, C++ style
comments were replaced by C style comments, C++ like
variable declarations after code were reordered.

Simplified mode is still broken. Nor did I fix
endianess issues. Both problems will be fixed in
additional patches (which need this one).

Signed-off-by: Stefan Weil 
---
 hw/eepro100.c |  192 +
 1 files changed, 98 insertions(+), 94 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 4210d8a..7093af8 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -213,6 +213,10 @@ typedef struct {
 uint32_t ru_offset; /* RU address offset */
 uint32_t statsaddr; /* pointer to eepro100_stats_t */
 
+/* Temporary data. */
+eepro100_tx_t tx;
+uint32_t cb_address;
+
 /* Statistical counters. Also used for wake-up packet (i82559). */
 eepro100_stats_t statistics;
 
@@ -748,17 +752,100 @@ static void dump_statistics(EEPRO100State * s)
 //~ missing("CU dump statistical counters");
 }
 
+static void tx_command(EEPRO100State *s)
+{
+uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr);
+uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
+/* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
+uint8_t buf[2600];
+uint16_t size = 0;
+uint32_t tbd_address = s->cb_address + 0x10;
+TRACE(RXTX, logout
+("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count 
%u\n",
+ tbd_array, tcb_bytes, s->tx.tbd_count));
+
+if (tcb_bytes > 2600) {
+logout("TCB byte count too large, using 2600\n");
+tcb_bytes = 2600;
+}
+if (!((tcb_bytes > 0) || (tbd_array != 0x))) {
+logout
+("illegal values of TBD array address and TCB byte count!\n");
+}
+assert(tcb_bytes <= sizeof(buf));
+while (size < tcb_bytes) {
+uint32_t tx_buffer_address = ldl_phys(tbd_address);
+uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+//~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+tbd_address += 8;
+TRACE(RXTX, logout
+("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
+ tx_buffer_address, tx_buffer_size));
+tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+cpu_physical_memory_read(tx_buffer_address, &buf[size],
+ tx_buffer_size);
+size += tx_buffer_size;
+}
+if (tbd_array == 0x) {
+/* Simplified mode. Was already handled by code above. */
+} else {
+/* Flexible mode. */
+uint8_t tbd_count = 0;
+if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
+/* Extended Flexible TCB. */
+for (; tbd_count < 2; tbd_count++) {
+uint32_t tx_buffer_address = ldl_phys(tbd_address);
+uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+tbd_address += 8;
+TRACE(RXTX, logout
+("TBD (extended flexible mode): buffer address 0x%08x, 
size 0x%04x\n",
+ tx_buffer_address, tx_buffer_size));
+tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+cpu_physical_memory_read(tx_buffer_address, &buf[size],
+ tx_buffer_size);
+size += tx_buffer_size;
+if (tx_buffer_el & 1) {
+break;
+}
+}
+}
+tbd_address = tbd_array;
+for (; tbd_count < s->tx.tbd_count; tbd_count++) {
+uint32_t tx_buffer_address = ldl_phys(tbd_address);
+uint16_t tx_buffer_size = lduw_phys(tbd_address + 4);
+uint16_t tx_buffer_el = lduw_phys(tbd_address + 6);
+tbd_address += 8;
+TRACE(RXTX, logout
+("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
+ tx_buffer_address, tx_buffer_size));
+tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
+cpu_physical_memory_read(tx_buffer_address, &buf[size],
+ tx_buffer_size);
+size += tx_buffer_size;
+if (tx_buffer_el & 1) {
+break;
+}
+}
+}
+TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, 
size)));
+qemu_send_packet(s->vc, buf, size);
+s->statistics.tx_good_frames++;
+/* Transmit with bad status

Re: [Qemu-devel] Build failure on mingw / make: *** [config-all-devices.mak] Error 2

2009-11-19 Thread Stefan Weil
Sebastian Herbszt schrieb:
> Stefan Weil wrote:
>> Sebastian Herbszt schrieb:
>>> v0.11.0-rc0-1677-gf165b53
>>>
>>> $ ./configure --target-list=i386-softmmu
>>> $ make
>>> Makefile:427: no file name for `-include'
>>> /bin/sh.exe: -c: line 1: unexpected EOF while looking for matching `"'
>>> /bin/sh.exe: -c: line 2: syntax error: unexpected end of file
>>> make: *** [config-all-devices.mak] Error 2
>>>
>>> - Sebastian
>>
>> I just sent a patch which should fix this problem. See
>> [PATCH] Fix build for mingw32 on windows ($$ expansion)
>
> It's a different one. This one seems to be related to "Makefile"
> config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
>$(call quiet-command,cat $(SUBDIR_DEVICES_MAK) | grep "=y$$" | sort
> -u > $@,"  GEN   $@")
>
> - Sebastian

Yes. I sent two patches. They fix two different problems :-)

Stefan






Re: [Qemu-devel] Build failure on mingw / make: *** [config-all-devices.mak] Error 2

2009-11-19 Thread Sebastian Herbszt

Sebastian Herbszt wrote:

Stefan Weil wrote:

Sebastian Herbszt schrieb:

v0.11.0-rc0-1677-gf165b53

$ ./configure --target-list=i386-softmmu
$ make
Makefile:427: no file name for `-include'
/bin/sh.exe: -c: line 1: unexpected EOF while looking for matching `"'
/bin/sh.exe: -c: line 2: syntax error: unexpected end of file
make: *** [config-all-devices.mak] Error 2

- Sebastian


I just sent a patch which should fix this problem. See
[PATCH] Fix build for mingw32 on windows ($$ expansion)


It's a different one. This one seems to be related to "Makefile"
config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
   $(call quiet-command,cat $(SUBDIR_DEVICES_MAK) | grep "=y$$" | sort -u > $@,"  
GEN   $@")


Just ignore this. Thought you meant "[PATCH] Fix build for mingw32 on windows ($@ in 
macro)".
Sorry!

- Sebastian





Re: [Qemu-devel] Build failure on mingw / make: *** [config-all-devices.mak] Error 2

2009-11-19 Thread Sebastian Herbszt

Stefan Weil wrote:

Sebastian Herbszt schrieb:

v0.11.0-rc0-1677-gf165b53

$ ./configure --target-list=i386-softmmu
$ make
Makefile:427: no file name for `-include'
/bin/sh.exe: -c: line 1: unexpected EOF while looking for matching `"'
/bin/sh.exe: -c: line 2: syntax error: unexpected end of file
make: *** [config-all-devices.mak] Error 2

- Sebastian


I just sent a patch which should fix this problem. See
[PATCH] Fix build for mingw32 on windows ($$ expansion)


It's a different one. This one seems to be related to "Makefile"
config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
   $(call quiet-command,cat $(SUBDIR_DEVICES_MAK) | grep "=y$$" | sort -u > $@,"  
GEN   $@")

- Sebastian





Re: [Qemu-devel] Build failure on mingw / make: *** [config-all-devices.mak] Error 2

2009-11-19 Thread Stefan Weil
Sebastian Herbszt schrieb:
> v0.11.0-rc0-1677-gf165b53
>
> $ ./configure --target-list=i386-softmmu
> $ make
> Makefile:427: no file name for `-include'
> /bin/sh.exe: -c: line 1: unexpected EOF while looking for matching `"'
> /bin/sh.exe: -c: line 2: syntax error: unexpected end of file
> make: *** [config-all-devices.mak] Error 2
>
> - Sebastian

I just sent a patch which should fix this problem. See
[PATCH] Fix build for mingw32 on windows ($$ expansion)

Regards,
Stefan





[Qemu-devel] Build failure on mingw / make: *** [config-all-devices.mak] Error 2

2009-11-19 Thread Sebastian Herbszt

v0.11.0-rc0-1677-gf165b53

$ ./configure --target-list=i386-softmmu
$ make
Makefile:427: no file name for `-include'
/bin/sh.exe: -c: line 1: unexpected EOF while looking for matching `"'
/bin/sh.exe: -c: line 2: syntax error: unexpected end of file
make: *** [config-all-devices.mak] Error 2

- Sebastian





Re: [Qemu-devel] [PATCH] e1000: Fix warning from code review

2009-11-19 Thread Ian Molton
Stefan Weil wrote:
> A code review run by Steve Grubb complained about code in e1000.c:
> 
> In hw/e1000.c at line 89, vlan is declared to be 4 bytes.
> At line 382 is an attempt to do a memmove over it with a size of 12.

> +/* Fields vlan and data must not be reordered or separated. */
>  unsigned char vlan[4];
>  unsigned char data[0x1];

Wouldnt it be better to stuff both into a struct or something? I guess
from the '12' that the data size can vary, but thats less important if
they are packed in a way that the compiler (and coders!) know not to
seperate them.

-Ian




[Qemu-devel] Re: [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread Sebastian Herbszt

Stefan Weil wrote:

Make using mingw32 on windows does not preserve $@ in macros
when they are modified using this pattern:
target: macro += something

This behaviour results in an error when QEMU_CFLAGS containing
"-MMD -MP -MT $@" is modified for compilation of source files
which use SDL: $@ will expand to nothing, -MT no longer has
the correct argument (it will take the next one from the command
line) and the build will fail or run with a wrong command line.

The problem is fixed by using a new macro QEMU_DGFLAGS
which is not modified by a target rule.

Signed-off-by: Stefan Weil 


Tested-by: Sebastian Herbszt 





[Qemu-devel] [PATCH] Fix build for mingw32 on windows ($$ expansion)

2009-11-19 Thread Stefan Weil
Make using mingw32 on windows fails when running grep "=y$$".
The command is expanded to grep "=y$ and the missing "
results in an error.

I don't expect a file config-devices.mak with =y somewhere in
the middle of a line (they are always at the end of the line),
so simplifying the regular expression to =y seems to be permitted.

This avoids problems with wrong expansion.

Signed-off-by: Stefan Weil 
---
 Makefile |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index f8a5036..16ed995 100644
--- a/Makefile
+++ b/Makefile
@@ -35,7 +35,7 @@ SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory)
 SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
 
 config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
-   $(call quiet-command,cat $(SUBDIR_DEVICES_MAK) | grep "=y$$" | sort -u 
> $@,"  GEN   $@")
+   $(call quiet-command,cat $(SUBDIR_DEVICES_MAK) | grep =y | sort -u > 
$@,"  GEN   $@")
 
 -include config-all-devices.mak
 
-- 
1.5.6.5





[Qemu-devel] [PATCH] Fix build for mingw32 on windows ($@ in macro)

2009-11-19 Thread Stefan Weil
Make using mingw32 on windows does not preserve $@ in macros
when they are modified using this pattern:
target: macro += something

This behaviour results in an error when QEMU_CFLAGS containing
"-MMD -MP -MT $@" is modified for compilation of source files
which use SDL: $@ will expand to nothing, -MT no longer has
the correct argument (it will take the next one from the command
line) and the build will fail or run with a wrong command line.

The problem is fixed by using a new macro QEMU_DGFLAGS
which is not modified by a target rule.

Signed-off-by: Stefan Weil 
---
 rules.mak |9 +
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/rules.mak b/rules.mak
index 77a801b..16713ba 100644
--- a/rules.mak
+++ b/rules.mak
@@ -11,16 +11,17 @@ MAKEFLAGS += -rR
 %.m:
 %.mak:
 
-QEMU_CFLAGS += -MMD -MP -MT $@
+# Flags for dependency generation
+QEMU_DGFLAGS += -MMD -MP -MT $@
 
 %.o: %.c $(GENERATED_HEADERS)
-   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) -c -o $@ $<,"  CC   
 $(TARGET_DIR)$@")
+   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c 
-o $@ $<,"  CC$(TARGET_DIR)$@")
 
 %.o: %.S
-   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) -c -o $@ $<,"  AS   
 $(TARGET_DIR)$@")
+   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c 
-o $@ $<,"  AS$(TARGET_DIR)$@")
 
 %.o: %.m
-   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) -c -o $@ $<,"  OBJC 
 $(TARGET_DIR)$@")
+   $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c 
-o $@ $<,"  OBJC  $(TARGET_DIR)$@")
 
 LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ 
$(1) $(ARLIBS_BEGIN) $(ARLIBS) $(ARLIBS_END) $(LIBS),"  LINK  $(TARGET_DIR)$@")
 
-- 
1.5.6.5





[Qemu-devel] [PATCH] e1000: Fix warning from code review

2009-11-19 Thread Stefan Weil
A code review run by Steve Grubb complained about code in e1000.c:

In hw/e1000.c at line 89, vlan is declared to be 4 bytes.
At line 382 is an attempt to do a memmove over it with a size of 12.

This was fixed by splitting the memmove in two calls and
adding a comment to the declaration of vlan and data.

Signed-off-by: Stefan Weil 
---
 hw/e1000.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 00f6a57..6f9adb5 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -89,6 +89,7 @@ typedef struct E1000State_st {
 struct e1000_tx {
 unsigned char header[256];
 unsigned char vlan_header[4];
+/* Fields vlan and data must not be reordered or separated. */
 unsigned char vlan[4];
 unsigned char data[0x1];
 uint16_t size;
@@ -383,7 +384,8 @@ xmit_seg(E1000State *s)
 if (tp->sum_needed & E1000_TXD_POPTS_IXSM)
 putsum(tp->data, tp->size, tp->ipcso, tp->ipcss, tp->ipcse);
 if (tp->vlan_needed) {
-memmove(tp->vlan, tp->data, 12);
+memmove(tp->vlan, tp->data, 4);
+memmove(tp->data, tp->data + 4, 8);
 memcpy(tp->data + 8, tp->vlan_header, 4);
 qemu_send_packet(s->vc, tp->vlan, tp->size + 4);
 } else
-- 
1.5.6.5





Re: [Qemu-devel] [RFC v0 00/15] QEMU Monitor Protocol

2009-11-19 Thread Luiz Capitulino
On Thu, 19 Nov 2009 13:13:28 -0200
Luiz Capitulino  wrote:

> o Not using the stream parser to read the input

 Just to clarify: this only means that a '\n' is required at the end of
input. This series is already capable on reading json input.




[Qemu-devel] Re: [RFC v0 00/15] QEMU Monitor Protocol

2009-11-19 Thread Luiz Capitulino
On Thu, 19 Nov 2009 17:20:30 +0200
Avi Kivity  wrote:

> On 11/19/2009 05:13 PM, Luiz Capitulino wrote:
> >   Hi,
> >
> >   This is not stable yet, it has a few bugs and a number of things to
> > be done, but I'm sending it now so that it can get an initial review
> > while I'm working on it.
> >
> >   At the end of the series there are two simple Python scripts which are
> > able to talk to QEMU by using QMP.
> >
> >   Main issues are:
> >
> > o Not all errors are being detected/handled correctly
> > o Not using the stream parser to read the input
> >
> >   If you want to try this, you need at least the latest version of QError,
> > and the conversions series to make this really useful.
> >
> 
> Can you post a capture of a few monitor commands through the new protocol?

 Here goes, it's a telnet session:

"""
{"QMP": {"capabilities": []}}

{ "execute": "info", "arguments": { "item": "balloon" } }
{"return": 1024}

{ "execute": "balloon", "arguments": { "value": 512 } }
{"return": "OK"}

{ "execute": "info", "arguments": { "item": "balloon" } }
{"return": 512}

{ "execute": "info", "arguments": { "item": "network" } }
{"return": [{"devices": [{"name": "user.0", "info": "net=10.0.2.0, 
restricted=n"}, {"name": "e1000.0", "info": 
"model=e1000,macaddr=52:54:00:12:34:56"}], "id": 0}]}

{ "execute": "pci_add", "arguments": { "pci_addr": "auto", "type": "nic" } }
{"return": {"bus": 0, "slot": 5, "domain": 0, "function": 0}}

{ "execute": "info", "arguments": { "item": "network" } }
{"return": [{"devices": [{"name": "user.0", "info": "net=10.0.2.0, 
restricted=n"}, {"name": "e1000.0", "info": 
"model=e1000,macaddr=52:54:00:12:34:56"}, {"name": "rtl8139.0", "info": 
"model=rtl8139,macaddr=52:54:00:12:34:57"}], "id": 0}]}

{ "execute": "migrate", "arguments": { "detach": "-d", "uri": 
"tcp:localhost:4445" } }
{"error": {"class": "InvalidParameter", "data": {"parameter": "detach", 
"reason": "must be an integer"}}}
"""




[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Adam Litke
On Thu, 2009-11-19 at 18:13 +0200, Avi Kivity wrote:
> On 11/19/2009 05:58 PM, Adam Litke wrote:
> > On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
> >
> >> On 11/19/2009 05:19 PM, Adam Litke wrote:
> >>  
> >>> Rusty and Anthony,
> >>> If I've addressed all outstanding issues, please consider this patch for
> >>> inclusion.  Thanks.
> >>>
> >>> +struct virtio_balloon_stat
> >>> +{
> >>> + __le16 tag;
> >>> + __le64 val;
> >>> +};
> >>> +
> >>>
> >>>
> >> You're not doing endian conversion in the host?
> >>  
> > No.  I was following by example.  For the virtio_balloon, the existing
> > code is careful so that the guest always writes data in little endian.
> >
> 
> I don't follow.  If the guest is careful to write little-endian, surely 
> the host must be equally careful to read little-endian?

That is true and, by my reading of the existing qemu virtio-balloon
device code, isn't virtio_balloon_set_config() on a big endian host
already broken?


-- 
Thanks,
Adam





[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:58 PM, Adam Litke wrote:

On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
   

On 11/19/2009 05:19 PM, Adam Litke wrote:
 

Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion.  Thanks.

+struct virtio_balloon_stat
+{
+   __le16 tag;
+   __le64 val;
+};
+

   

You're not doing endian conversion in the host?
 

No.  I was following by example.  For the virtio_balloon, the existing
code is careful so that the guest always writes data in little endian.
   


I don't follow.  If the guest is careful to write little-endian, surely 
the host must be equally careful to read little-endian?


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





[Qemu-devel] Re: virtio: Report new guest memory statistics pertinent to memory ballooning (V4)

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:56 PM, Adam Litke wrote:

On Thu, 2009-11-19 at 17:19 +0200, Avi Kivity wrote:
   

On 11/19/2009 05:06 PM, Adam Litke wrote:
 

Avi and Anthony,
If you agree that I've addressed all outstanding issues, please consider this
patch for inclusion.  Thanks.


   

I'd like to see this (and all other virtio-ABI-modifying patches) first
go into the virtio pci spec, then propagated to guest and host.
 

Where can I find information on the procedure for updating the virtio
pci spec?

   


Send a patch to Rusty (same copy list).

http://ozlabs.org/~rusty/virtio-spec/


Changes since V3:
   - Increase stat field size to 64 bits
   - Report all sizes in kb (not pages)

   

Why not bytes?  It's the most natural unit.
 

The precision for most of these stats (except major and minor faults)
is 4kb (at least for Linux guests on the platforms I can think of).  I
chose kb units to avoid wasting bits.  I suppose the 64bit fields are
"Bigger than we could ever possibly need (tm)".  Others have suggested
bytes as well so I will be happy to make the change.
   


It's my engineering backgrounds.  I'd actually prefer them in a kg/m/s 
combo but it doesn't really work out.



-static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
+static void request_stats(VirtIOBalloon *vb)
+{
+vb->stats_requested = 1;
+reset_stats(vb);
+monitor_suspend(cur_mon);

   

You allow the guest to kill a monitor here.
 

Is there a better way to handle asynchronous communication with the
guest?
   


With the current monitor, the only option I can think of it a command to 
request stats and a command to report stats (with a version number so we 
can see if it actually worked).


The new monitor will support async command completion but even then I 
don't think we should allow a guest to stop command execution 
indefinitely (it would tie up resources at the client).



+typedef struct VirtIOBalloonStat {
+uint16_t tag;
+uint64_t val;
+} VirtIOBalloonStat;

   

Alignment here depends on word size.  This needs to be padded to be
aligned the same way on 32 and 64 bit hosts and guests.
 

ok.  I assume that means the structure size must be a multiple of 64
bits in size


Yes, and all values must be naturally aligned.

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





[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Adam Litke
On Thu, 2009-11-19 at 17:22 +0200, Avi Kivity wrote:
> On 11/19/2009 05:19 PM, Adam Litke wrote:
> > Rusty and Anthony,
> > If I've addressed all outstanding issues, please consider this patch for
> > inclusion.  Thanks.
> >
> > +struct virtio_balloon_stat
> > +{
> > +   __le16 tag;
> > +   __le64 val;
> > +};
> > +
> >
> 
> You're not doing endian conversion in the host?

No.  I was following by example.  For the virtio_balloon, the existing
code is careful so that the guest always writes data in little endian.

-- 
Thanks,
Adam





[Qemu-devel] Re: virtio: Report new guest memory statistics pertinent to memory ballooning (V4)

2009-11-19 Thread Adam Litke
On Thu, 2009-11-19 at 17:19 +0200, Avi Kivity wrote:
> On 11/19/2009 05:06 PM, Adam Litke wrote:
> > Avi and Anthony,
> > If you agree that I've addressed all outstanding issues, please consider 
> > this
> > patch for inclusion.  Thanks.
> >
> >
> 
> I'd like to see this (and all other virtio-ABI-modifying patches) first 
> go into the virtio pci spec, then propagated to guest and host.

Where can I find information on the procedure for updating the virtio
pci spec?

> > Changes since V3:
> >   - Increase stat field size to 64 bits
> >   - Report all sizes in kb (not pages)
> >
> 
> Why not bytes?  It's the most natural unit.

The precision for most of these stats (except major and minor faults)
is 4kb (at least for Linux guests on the platforms I can think of).  I
chose kb units to avoid wasting bits.  I suppose the 64bit fields are
"Bigger than we could ever possibly need (tm)".  Others have suggested
bytes as well so I will be happy to make the change.

> > -static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
> > +static void request_stats(VirtIOBalloon *vb)
> > +{
> > +vb->stats_requested = 1;
> > +reset_stats(vb);
> > +monitor_suspend(cur_mon);
> >
> 
> You allow the guest to kill a monitor here.

Is there a better way to handle asynchronous communication with the
guest?

> > +virtqueue_push(vb->svq,&vb->stats_vq_elem, vb->stats_vq_offset);
> > +virtio_notify(&vb->vdev, vb->svq);
> > +}
> > +
> >
> 
> 
> 
> > +typedef struct VirtIOBalloonStat {
> > +uint16_t tag;
> > +uint64_t val;
> > +} VirtIOBalloonStat;
> >
> 
> Alignment here depends on word size.  This needs to be padded to be 
> aligned the same way on 32 and 64 bit hosts and guests.

ok.  I assume that means the structure size must be a multiple of 64
bits in size?

-- 
Thanks,
Adam





[Qemu-devel] Re: SeaBIOS cdrom regression with Vista

2009-11-19 Thread Alexander Graf
Avi Kivity wrote:
> On 11/17/2009 03:21 PM, Avi Kivity wrote:
>> qemu-kvm's switch to seabios uncovered a regression with cdrom
>> handling.  Vista x64 no longer recognizes the cdrom, while pc-bios
>> still works.  Installing works, but that uses int 13, not the native
>> driver.  Haven't investigated further yet.
>>
>> Command line:
>>
>> qemu -drive
>> file=/root/kvm-autotest/client/tests/kvm/images/winvista-64.qcow2,if=ide,cache=writeback
>> -m 512 -smp 2 -cdrom
>> /root/kvm-autotest/client/tests/kvm/isos/windows/winutils.iso -snapshot
>>
>> b496fe34317ead61cf5ae019506fadc8f9ad6556 in qemu-kvm.git.
>>
>
> I've temporarily reverted to bochs bios until this issue is resolved.


Make sure my second linuxboot patch (fix linuxboot with bochsbios) is
applied then, as otherwise -kernel will break.


Alex




[Qemu-devel] Re: SeaBIOS cdrom regression with Vista

2009-11-19 Thread Avi Kivity

On 11/17/2009 03:21 PM, Avi Kivity wrote:
qemu-kvm's switch to seabios uncovered a regression with cdrom 
handling.  Vista x64 no longer recognizes the cdrom, while pc-bios 
still works.  Installing works, but that uses int 13, not the native 
driver.  Haven't investigated further yet.


Command line:

qemu -drive 
file=/root/kvm-autotest/client/tests/kvm/images/winvista-64.qcow2,if=ide,cache=writeback 
-m 512 -smp 2 -cdrom 
/root/kvm-autotest/client/tests/kvm/isos/windows/winutils.iso -snapshot


b496fe34317ead61cf5ae019506fadc8f9ad6556 in qemu-kvm.git.



I've temporarily reverted to bochs bios until this issue is resolved.

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





[Qemu-devel] Re: virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:19 PM, Adam Litke wrote:

Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion.  Thanks.

+struct virtio_balloon_stat
+{
+   __le16 tag;
+   __le64 val;
+};
+
   


You're not doing endian conversion in the host?

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





[Qemu-devel] Re: [RFC v0 00/15] QEMU Monitor Protocol

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:13 PM, Luiz Capitulino wrote:

  Hi,

  This is not stable yet, it has a few bugs and a number of things to
be done, but I'm sending it now so that it can get an initial review
while I'm working on it.

  At the end of the series there are two simple Python scripts which are
able to talk to QEMU by using QMP.

  Main issues are:

o Not all errors are being detected/handled correctly
o Not using the stream parser to read the input

  If you want to try this, you need at least the latest version of QError,
and the conversions series to make this really useful.
   


Can you post a capture of a few monitor commands through the new protocol?

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





[Qemu-devel] Re: virtio: Report new guest memory statistics pertinent to memory ballooning (V4)

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:06 PM, Adam Litke wrote:

Avi and Anthony,
If you agree that I've addressed all outstanding issues, please consider this
patch for inclusion.  Thanks.

   


I'd like to see this (and all other virtio-ABI-modifying patches) first 
go into the virtio pci spec, then propagated to guest and host.



Changes since V3:
  - Increase stat field size to 64 bits
  - Report all sizes in kb (not pages)
   


Why not bytes?  It's the most natural unit.


-static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
+static void request_stats(VirtIOBalloon *vb)
+{
+vb->stats_requested = 1;
+reset_stats(vb);
+monitor_suspend(cur_mon);
   


You allow the guest to kill a monitor here.


+virtqueue_push(vb->svq,&vb->stats_vq_elem, vb->stats_vq_offset);
+virtio_notify(&vb->vdev, vb->svq);
+}
+
   





+typedef struct VirtIOBalloonStat {
+uint16_t tag;
+uint64_t val;
+} VirtIOBalloonStat;
   


Alignment here depends on word size.  This needs to be padded to be 
aligned the same way on 32 and 64 bit hosts and guests.



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





[Qemu-devel] virtio: Add memory statistics reporting to the balloon driver (V3)

2009-11-19 Thread Adam Litke
Rusty and Anthony,
If I've addressed all outstanding issues, please consider this patch for
inclusion.  Thanks.

Changes since V2:
 - Increase stat field size to 64 bits
 - Report all sizes in kb (not pages)
 - Drop anon_pages stat and fix endianness conversion

Changes since V1:
 - Use a virtqueue instead of the device config space

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them directly to the hypervisor.

This patch enables the guest-side support by adding stats collection and
reporting to the virtio balloon driver.

Signed-off-by: Adam Litke 
Cc: Rusty Russell 
Cc: Anthony Liguori 
Cc: virtualizat...@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 200c22f..ebc9d39 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,7 +29,7 @@
 struct virtio_balloon
 {
struct virtio_device *vdev;
-   struct virtqueue *inflate_vq, *deflate_vq;
+   struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
 
/* Where the ballooning thread waits for config to change. */
wait_queue_head_t config_change;
@@ -50,6 +50,9 @@ struct virtio_balloon
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
u32 pfns[256];
+
+   /* Memory statistics */
+   struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
 };
 
 static struct virtio_device_id id_table[] = {
@@ -155,6 +158,57 @@ static void leak_balloon(struct virtio_balloon *vb, size_t 
num)
}
 }
 
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+   __le16 tag, __le64 val)
+{
+   BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
+   vb->stats[idx].tag = cpu_to_le16(tag);
+   vb->stats[idx].val = cpu_to_le64(val);
+}
+
+#define K(x) ((x) << (PAGE_SHIFT - 10))
+static void update_balloon_stats(struct virtio_balloon *vb)
+{
+   unsigned long events[NR_VM_EVENT_ITEMS];
+   struct sysinfo i;
+   int idx = 0;
+
+   all_vm_events(events);
+   si_meminfo(&i);
+
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN, K(events[PSWPIN]));
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT, K(events[PSWPOUT]));
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE, K(i.freeram));
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT, K(i.totalram));
+}
+
+/*
+ * While most virtqueues communicate guest-initiated requests to the 
hypervisor,
+ * the stats queue operates in reverse.  The driver initializes the virtqueue
+ * with a single buffer.  From that point forward, all conversations consist of
+ * a hypervisor request (a call to this function) which directs us to refill
+ * the virtqueue with a fresh stats buffer.
+ */
+static void stats_ack(struct virtqueue *vq)
+{
+   struct virtio_balloon *vb;
+   unsigned int len;
+   struct scatterlist sg;
+
+   vb = vq->vq_ops->get_buf(vq, &len);
+   if (!vb)
+   return;
+
+   update_balloon_stats(vb);
+
+   sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+   if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0)
+   BUG();
+   vq->vq_ops->kick(vq);
+}
+
 static void virtballoon_changed(struct virtio_device *vdev)
 {
struct virtio_balloon *vb = vdev->priv;
@@ -205,10 +259,10 @@ static int balloon(void *_vballoon)
 static int virtballoon_probe(struct virtio_device *vdev)
 {
struct virtio_balloon *vb;
-   struct virtqueue *vqs[2];
-   vq_callback_t *callbacks[] = { balloon_ack, balloon_ack };
-   const char *names[] = { "inflate", "deflate" };
-   int err;
+   struct virtqueue *vqs[3];
+   vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack };
+   const char *names[] = { "inflate", "deflate", "stats" };
+   int err, nvqs;
 
vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
if (!vb) {
@@ -221,13 +275,28 @@ static int virtballoon_probe(struct virtio_device *vdev)
init_waitqueue_head(&vb->config_change);
vb->vdev = vdev;
 
-   /* We expect two virtqueues. */
-   err = vdev->config->find_vqs(

[Qemu-devel] [PATCH 15/15] QMP: Introduce vm-info

2009-11-19 Thread Luiz Capitulino
A Python script which uses qmp.py to print some simple VM info.

Signed-off-by: Luiz Capitulino 
---
 QMP/vm-info |   32 
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100755 QMP/vm-info

diff --git a/QMP/vm-info b/QMP/vm-info
new file mode 100755
index 000..76a119e
--- /dev/null
+++ b/QMP/vm-info
@@ -0,0 +1,32 @@
+#!/usr/bin/python
+#
+# Print Virtual Machine information
+#
+# Usage:
+#
+# Start QEMU with:
+#
+# $ qemu [...] -monitor control,unix:./qmp,server
+#
+# Run vm-info:
+#
+# $ vm-info ./qmp
+#
+# Luiz Capitulino 
+
+import qmp
+from sys import argv,exit
+
+def main():
+if len(argv) != 2:
+print 'vm-info '
+exit(1)
+
+qemu = qmp.QEMUMonitorProtocol(argv[1])
+qemu.connect()
+
+for cmd in [ 'version', 'hpet', 'kvm', 'status', 'uuid', 'balloon' ]:
+print cmd + ': ' + str(qemu.send('info item=' + cmd))
+
+if __name__ == '__main__':
+main()
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 14/15] QMP: Introduce qmp-shell

2009-11-19 Thread Luiz Capitulino
This is a very simple shell written in Python which works
on top of QMP.

Unfortunately it's a bit awkward right now, as the user has
to specify the arguments names, for example:

(QEMU) info item=version
0.11.50
(QEMU)

Also, if the output is not a string or integer, the user is
is going to get a low-level dictionary or list.

It's worth to note that the shell is broken into two files.
One is the shell itself, the other is the QMP class which
handles the communication with QEMU.

Signed-off-by: Luiz Capitulino 
---
 QMP/qmp-shell |   66 +
 QMP/qmp.py|   54 ++
 2 files changed, 120 insertions(+), 0 deletions(-)
 create mode 100755 QMP/qmp-shell
 create mode 100644 QMP/qmp.py

diff --git a/QMP/qmp-shell b/QMP/qmp-shell
new file mode 100755
index 000..2faf79b
--- /dev/null
+++ b/QMP/qmp-shell
@@ -0,0 +1,66 @@
+#!/usr/bin/python
+#
+# Simple QEMU shell on top of QMP
+#
+# Usage:
+#
+# Start QEMU with:
+#
+# $ qemu [...] -monitor control,unix:./qmp,server
+#
+# Run the shell:
+#
+# $ qmp-shell ./qmp
+#
+# Commands have the following format:
+#
+# < command-name > [ arg-name1=arg1 ] ... [ arg-nameN=argN ]
+#
+# For example:
+#
+# (QEMU) info item=network
+#
+# Luiz Capitulino 
+
+import qmp
+import readline
+from sys import argv,exit
+
+def shell_help():
+print 'bye  exit from the shell'
+
+def main():
+if len(argv) != 2:
+print 'qemu-shell '
+exit(1)
+
+qemu = qmp.QEMUMonitorProtocol(argv[1])
+qemu.connect()
+
+print 'Connected!'
+
+while True:
+try:
+cmd = raw_input('(QEMU) ')
+except EOFError:
+print
+break
+if cmd == '':
+continue
+elif cmd == 'bye':
+break
+elif cmd == 'help':
+shell_help()
+else:
+try:
+resp = qemu.send(cmd)
+if resp == None:
+print 'Disconnected'
+break
+print resp
+except IndexError:
+print '-> command format:  ',
+print '[arg-name1=arg1] ... [arg-nameN=argN]'
+
+if __name__ == '__main__':
+main()
diff --git a/QMP/qmp.py b/QMP/qmp.py
new file mode 100644
index 000..c56f143
--- /dev/null
+++ b/QMP/qmp.py
@@ -0,0 +1,54 @@
+import socket, json
+
+class QMPError(Exception):
+pass
+
+class QMPConnectError(QMPError):
+pass
+
+class QEMUMonitorProtocol:
+def connect(self):
+self.sock.connect(self.filename)
+data = self.__json_read()
+if data == None:
+raise QMPConnectError
+if not data.has_key('QMP'):
+raise QMPConnectError
+return data['QMP']['capabilities']
+
+def send(self, cmdline):
+cmd = self.__build_cmd(cmdline)
+self.__json_send(cmd)
+resp = self.__json_read()
+if resp == None:
+return
+elif resp.has_key('error'):
+return resp['error']
+else:
+return resp['return']
+
+def __build_cmd(self, cmdline):
+cmdargs = cmdline.split()
+qmpcmd = { 'execute': cmdargs[0], 'arguments': {} }
+for arg in cmdargs[1:]:
+opt = arg.split('=')
+try:
+value = int(opt[1])
+except ValueError:
+value = opt[1]
+qmpcmd['arguments'][opt[0]] = value
+return qmpcmd
+
+def __json_send(self, cmd):
+self.sock.send(json.dumps(cmd) + '\n')
+
+def __json_read(self):
+try:
+return json.loads(self.sock.recv(1024))
+except ValueError:
+return
+
+def __init__(self, filename):
+self.filename = filename
+self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 13/15] QMP: Introduce qmp-events.txt

2009-11-19 Thread Luiz Capitulino
Signed-off-by: Luiz Capitulino 
---
 QMP/qmp-events.txt |   26 ++
 1 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100644 QMP/qmp-events.txt

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
new file mode 100644
index 000..682a5e5
--- /dev/null
+++ b/QMP/qmp-events.txt
@@ -0,0 +1,26 @@
+   QEMU Monitor Protocol: Events
+   =
+
+1 SHUTDOWN
+---
+
+Description: Issued when the Virtual Machine is powered down.
+Data: None.
+
+2 RESET
+---
+
+Description: Issued when the Virtual Machine is reseted.
+Data: None.
+
+3 STOP
+--
+
+Description: Issued when the Virtual Machine is stopped.
+Data: None.
+
+4 DEBUG
+---
+
+Description: Issued when the Virtual Machine enters debug mode.
+Data: None.
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 12/15] QMP: Introduce specification

2009-11-19 Thread Luiz Capitulino
Signed-off-by: Luiz Capitulino 
---
 QMP/qmp-spec.txt |  193 ++
 1 files changed, 193 insertions(+), 0 deletions(-)
 create mode 100644 QMP/qmp-spec.txt

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
new file mode 100644
index 000..e155f92
--- /dev/null
+++ b/QMP/qmp-spec.txt
@@ -0,0 +1,193 @@
+   QEMU Monitor Protocol Draft Specification - Version 0.1
+
+1. Introduction
+===
+
+This document specifies the QEMU Monitor Protocol (QMP), a JSON-based protocol
+which is available for applications to control QEMU at the machine-level.
+
+To enable QMP support, QEMU has to be run in "control mode". This is done by
+starting QEMU with the appropriate command-line options. Please, refer to the
+QEMU manual page for more information.
+
+2. Protocol Specification
+=
+
+This section details the protocol format. For the purpose of this document
+"Client" is any application which is communicating with QEMU in control mode,
+and "Server" is QEMU itself.
+
+JSON data structures, when mentioned in this document, are always in the
+following format:
+
+json-DATA-STRUCTURE-NAME
+
+Where DATA-STRUCTURE-NAME is any valid JSON data structure, as defined by
+the JSON standard:
+
+http://www.ietf.org/rfc/rfc4627.txt
+
+For convenience, json-objects mentioned in this document will have its members
+in a certain order. However, in real protocol usage json-objects members can
+be in ANY order, thus no particular order should be assumed.
+
+2.1 General Definitions
+---
+
+2.1.1 All interactions transmitted by the Server are json-objects, always
+  terminating with CRLF
+
+2.1.2 All json-objects members are mandatory when not specified otherwise
+
+2.2 Server Greeting
+---
+
+Right when connected the Server will issue a greeting message, which signals
+that the connection has been successfully established and that the Server is
+waiting for commands.
+
+The format is:
+
+{ "QMP": { "capabilities": json-array } }
+
+ Where,
+
+- The "capabilities" member specify the availability of features beyond the
+  baseline specification
+
+2.3 Issuing Commands
+
+
+The format for command execution is:
+
+{ "execute": json-string, "arguments": json-object, "id": json-value }
+
+ Where,
+
+- The "execute" member identifies the command to be executed by the Server
+- The "arguments" member is used to pass any arguments required for the
+  execution of the command, it is optional when no arguments are required
+- The "id" member is a transaction identification associated with the
+  command execution, it is optional and will be part of the response if
+  provided
+
+2.4 Commands Responses
+--
+
+There are two possible responses which the Server will issue as the result
+of a command execution: success or error.
+
+2.4.1 success
+-
+
+The success response is issued when the command execution has finished
+without errors.
+
+The format is:
+
+{ "return": json-value, "id": json-value }
+
+ Where,
+
+- The "return" member contains the command returned data, which is defined
+  in a per-command basis or "OK" if the command does not return data
+- The "id" member contains the transaction identification associated
+  with the command execution (if issued by the Client)
+
+2.4.2 error
+---
+
+The error response is issued when the command execution could not be
+completed because of an error condition.
+
+The format is:
+
+{ "error": { "class": json-string, "data": json-value }, "id": json-value }
+
+ Where,
+
+- The "class" member contains the error class (eg. "ServiceUnavailable")
+- The "data" member contains specific error data and is defined in a
+  per-command basis, it will be an empty json-object if the error has no data
+- The "id" member contains the transaction identification associated with
+  the command execution (if issued by the Client)
+
+Some errors can occur before the Server is able to read the "id" member,
+in these cases the "id" member will not be part of the error response, even
+if provided by the client.
+
+2.5 Asynchronous events
+---
+
+As a result of state changes, the Server may send messages unilaterally
+to the Client at any time. They are called 'asynchronous events'.
+
+The format is:
+
+{ "event": json-string, "data": json-value,
+  "timestamp": { "seconds": json-number, "microseconds": json-number } }
+
+ Where,
+
+- The "event" member contains the event's name
+- The "data" member contains event specific data, which is defined in a
+  per-event basis, it is optional
+- The "timestamp" member contains the exact time of when the event ocurred
+  in the Server. It is a fixed json-object with time in seconds and
+  microseconds
+
+For a listing of supported asynchronous events, please, refer to the
+qmp-events.txt file.
+
+3. QMP Examples
+===
+
+This section provides some examples of real 

[Qemu-devel] [PATCH 11/15] QMP: Introduce README file

2009-11-19 Thread Luiz Capitulino
Signed-off-by: Luiz Capitulino 
---
 QMP/README |   47 +++
 1 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100644 QMP/README

diff --git a/QMP/README b/QMP/README
new file mode 100644
index 000..5ac1742
--- /dev/null
+++ b/QMP/README
@@ -0,0 +1,47 @@
+  QEMU Monitor Protocol
+  =
+
+Introduction
+-
+
+The QEMU Monitor Protocol (QMP) is a JSON[1] based protocol for QEMU.
+
+By using it applications can control QEMU in reliable and "parseable" way,
+QMP also provides asynchronous events support.
+
+For more information, please, refer to the following files:
+
+o qmp-spec.txtQEMU Monitor Protocol current draft specification
+o qmp-events.txt  List of available asynchronous events
+o vm-info A simple QMP usage example in Python
+
+[1] http://www.json.org
+
+Usage
+-
+
+To enable QMP, QEMU has to be started in "control mode". This is done
+by passing the flag "control" to the "-monitor" command-line option.
+
+For example:
+
+$ qemu [...] -monitor control,tcp:localhost:,server
+
+Will start QEMU in control mode, waiting for a client TCP connection
+on localhost port .
+
+To manually test it you can connect with telnet and issue commands:
+
+$ telnet localhost 
+Trying ::1...
+Connected to localhost.
+Escape character is '^]'.
+{"QMP": {"capabilities": []}}
+{ "execute": "info", "arguments": { "item": "version" } }
+{"return": "0.11.50"}
+
+Contact
+---
+
+http://www.linux-kvm.org/page/MonitorProtocol
+Luiz Fernando N. Capitulino 
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 10/15] QMP: Disable monitor print functions

2009-11-19 Thread Luiz Capitulino
We still have handlers which will call monitor print functions
in several places.

If they do this when we are in control mode, we will be emitting
garbage to our clients.

To avoid this situation, this commit adds a way to disable
those functions. If any of them is called when in control mode,
we will emit a generic error.

Although this is far from the perfect solution, it guarantees
that only JSON is printed.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   14 +++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index dc4583f..e85f990 100644
--- a/monitor.c
+++ b/monitor.c
@@ -97,6 +97,7 @@ typedef struct MonitorControl {
 uint8_t buf[1024];
 int size;
 QObject *id;
+int print_enabled;
 } MonitorControl;
 
 struct Monitor {
@@ -184,9 +185,13 @@ static void monitor_puts(Monitor *mon, const char *str)
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
-char buf[4096];
-vsnprintf(buf, sizeof(buf), fmt, ap);
-monitor_puts(mon, buf);
+if (mon->mc && !mon->mc->print_enabled) {
+qemu_error_new(QERR_UNDEFINED_ERROR, "monitor print");
+} else {
+char buf[4096];
+vsnprintf(buf, sizeof(buf), fmt, ap);
+monitor_puts(mon, buf);
+}
 }
 
 void monitor_printf(Monitor *mon, const char *fmt, ...)
@@ -270,7 +275,10 @@ static void monitor_json_emitter(Monitor *mon, const 
QObject *data)
 json = qobject_to_json(data);
 assert(json != NULL);
 
+mon->mc->print_enabled = 1;
 monitor_printf(mon, "%s\n", qstring_get_str(json));
+mon->mc->print_enabled = 0;
+
 QDECREF(json);
 }
 
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 09/15] QMP: Introduce basic asynchronous events

2009-11-19 Thread Luiz Capitulino
Debug, shutdown, reset, powerdown and stop are all basic events,
as they are very simple they can be added in the same commit.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   15 +++
 monitor.h |5 +
 vl.c  |   11 +--
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 616f712..dc4583f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -341,6 +341,21 @@ void monitor_protocol_event(MonitorEvent event, QObject 
*data)
 return;
 
 switch (event) {
+case EVENT_DEBUG:
+event_name = "DEBUG";
+break;
+case EVENT_SHUTDOWN:
+event_name = "SHUTDOWN";
+break;
+case EVENT_RESET:
+event_name = "RESET";
+break;
+case EVENT_POWERDOWN:
+event_name = "POWERDOWN";
+break;
+case EVENT_STOP:
+event_name = "STOP";
+break;
 default:
 abort();
 break;
diff --git a/monitor.h b/monitor.h
index a1d8b7a..851fd33 100644
--- a/monitor.h
+++ b/monitor.h
@@ -15,6 +15,11 @@ extern Monitor *cur_mon;
 
 /* QMP events */
 typedef enum MonitorEvent {
+EVENT_DEBUG,
+EVENT_SHUTDOWN,
+EVENT_RESET,
+EVENT_POWERDOWN,
+EVENT_STOP,
 EVENT_MAX,
 } MonitorEvent;
 
diff --git a/vl.c b/vl.c
index 6b1a77a..5da50e7 100644
--- a/vl.c
+++ b/vl.c
@@ -4110,9 +4110,12 @@ static void main_loop(void)
 #endif
 } while (vm_can_run());
 
-if (qemu_debug_requested())
+if (qemu_debug_requested()) {
+monitor_protocol_event(EVENT_DEBUG, NULL);
 vm_stop(EXCP_DEBUG);
+}
 if (qemu_shutdown_requested()) {
+monitor_protocol_event(EVENT_SHUTDOWN, NULL);
 if (no_shutdown) {
 vm_stop(0);
 no_shutdown = 0;
@@ -4120,15 +4123,19 @@ static void main_loop(void)
 break;
 }
 if (qemu_reset_requested()) {
+monitor_protocol_event(EVENT_RESET, NULL);
 pause_all_vcpus();
 qemu_system_reset();
 resume_all_vcpus();
 }
 if (qemu_powerdown_requested()) {
+monitor_protocol_event(EVENT_POWERDOWN, NULL);
 qemu_irq_raise(qemu_system_powerdown);
 }
-if ((r = qemu_vmstop_requested()))
+if ((r = qemu_vmstop_requested())) {
+monitor_protocol_event(EVENT_STOP, NULL);
 vm_stop(r);
+}
 }
 pause_all_vcpus();
 }
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 08/15] QMP: Asynchronous events infrastructure

2009-11-19 Thread Luiz Capitulino
Asynchronous events are generated with a call to
monitor_protocol_event().

This function builds the right data-type and emit the event
right away. The emitted data is always a JSON object and its
format is as follows:

{ "event": json-string,
  "timestamp": { "seconds": json-number, "microseconds": json-number },
  "data": json-value }

This design is based on ideas by Amit Shah .

Signed-off-by: Luiz Capitulino 
---
 monitor.c   |   50 ++
 monitor.h   |6 ++
 qemu-tool.c |4 
 3 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5c5ae97..616f712 100644
--- a/monitor.c
+++ b/monitor.c
@@ -306,6 +306,56 @@ static void monitor_protocol_emitter(Monitor *mon, QObject 
*data)
 QDECREF(qmp);
 }
 
+static void timestamp_put(QDict *qdict)
+{
+int err;
+QObject *obj;
+struct timeval tv;
+
+err = gettimeofday(&tv, NULL);
+if (err < 0)
+return;
+
+obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
+"'microseconds': %" PRId64 " }",
+(int64_t) tv.tv_sec, (int64_t) tv.tv_usec);
+assert(obj != NULL);
+
+qdict_put_obj(qdict, "timestamp", obj);
+}
+
+/**
+ * monitor_protocol_event(): Generate a Monitor event
+ *
+ * Event-specific data can be emitted through the (optional) 'data' parameter.
+ */
+void monitor_protocol_event(MonitorEvent event, QObject *data)
+{
+QDict *qmp;
+const char *event_name;
+Monitor *mon = cur_mon;
+
+assert(event < EVENT_MAX);
+
+if (!monitor_ctrl_mode(mon))
+return;
+
+switch (event) {
+default:
+abort();
+break;
+}
+
+qmp = qdict_new();
+timestamp_put(qmp);
+qdict_put(qmp, "event", qstring_from_str(event_name));
+if (data)
+qdict_put_obj(qmp, "data", data);
+
+monitor_json_emitter(mon, QOBJECT(qmp));
+QDECREF(qmp);
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
 const char *p, *pstart;
diff --git a/monitor.h b/monitor.h
index 556507c..a1d8b7a 100644
--- a/monitor.h
+++ b/monitor.h
@@ -13,6 +13,12 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 
+/* QMP events */
+typedef enum MonitorEvent {
+EVENT_MAX,
+} MonitorEvent;
+
+void monitor_protocol_event(MonitorEvent event, QObject *data);
 const char *monitor_cmdline_parse(const char *cmdline, int *flags);
 void monitor_init(CharDriverState *chr, int flags);
 
diff --git a/qemu-tool.c b/qemu-tool.c
index b35ea8e..18b48af 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -56,6 +56,10 @@ int get_async_context_id(void)
 return 0;
 }
 
+void monitor_protocol_event(MonitorEvent event, QObject *data)
+{
+}
+
 QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque)
 {
 QEMUBH *bh;
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 07/15] QMP: Input support

2009-11-19 Thread Luiz Capitulino
QMP input is handled by monitor_handle_qmp_command().

This function's job is to check if the input is correct and
if so call the appropriate handler. In other words, it does
for QMP what monitor_parse_command() does for the user
protocol.

This means that monitor_handle_qmp_command() also has to
parse the (ugly) "args_type" format to able to get the
arguments names and types expected by the handler.

The format to input commands to QMP is as follows:

{ "execute": json-string,
  "id": json-value, "arguments": json-object }

Please, note that this commit also adds "id" support.

TODO: Use QJSON to read from the user
TODO: Errors need to be reviewed

Signed-off-by: Luiz Capitulino 
---
 monitor.c |  251 +++--
 1 files changed, 244 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index e81f9e6..5c5ae97 100644
--- a/monitor.c
+++ b/monitor.c
@@ -96,6 +96,7 @@ struct mon_fd_t {
 typedef struct MonitorControl {
 uint8_t buf[1024];
 int size;
+QObject *id;
 } MonitorControl;
 
 struct Monitor {
@@ -296,6 +297,11 @@ static void monitor_protocol_emitter(Monitor *mon, QObject 
*data)
 mon->error = NULL;
 }
 
+if (mon->mc->id) {
+qdict_put_obj(qmp, "id", mon->mc->id);
+mon->mc->id = NULL;
+}
+
 monitor_json_emitter(mon, QOBJECT(qmp));
 QDECREF(qmp);
 }
@@ -374,16 +380,27 @@ static void do_info(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 const mon_cmd_t *cmd;
 const char *item = qdict_get_try_str(qdict, "item");
 
-if (!item)
+if (!item) {
+if (monitor_ctrl_mode(mon)) {
+/* 'item' is mandatory in ctrl mode */
+qemu_error_new(QERR_COMMAND_NOT_ISSUED);
+return;
+}
 goto help;
+}
 
 for (cmd = info_cmds; cmd->name != NULL; cmd++) {
 if (compare_cmd(item, cmd->name))
 break;
 }
 
-if (cmd->name == NULL)
+if (cmd->name == NULL) {
+if (monitor_ctrl_mode(mon)) {
+qemu_error_new(QERR_COMMAND_NOT_FOUND, item);
+return;
+}
 goto help;
+}
 
 if (monitor_handler_ported(cmd)) {
 cmd->mhandler.info_new(mon, ret_data);
@@ -397,7 +414,12 @@ static void do_info(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 cmd->user_print(mon, *ret_data);
 }
 } else {
-cmd->mhandler.info(mon);
+if (monitor_ctrl_mode(mon)) {
+/* handler not converted yet */
+qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd->name);
+} else {
+cmd->mhandler.info(mon);
+}
 }
 
 return;
@@ -3355,10 +3377,15 @@ static void monitor_handle_command(Monitor *mon, const 
char *cmdline)
 if (monitor_handler_ported(cmd)) {
 monitor_call_handler(mon, cmd, qdict);
 } else {
-cmd->mhandler.cmd(mon, qdict);
+if (monitor_ctrl_mode(mon)) {
+/* handler not converted yet */
+qemu_error_new(QERR_COMMAND_NOT_FOUND, cmd->name);
+} else {
+cmd->mhandler.cmd(mon, qdict);
 
-if (monitor_has_error(mon)) {
-monitor_print_error(mon);
+if (monitor_has_error(mon)) {
+monitor_print_error(mon);
+}
 }
 }
 
@@ -3590,6 +3617,216 @@ static int monitor_can_read(void *opaque)
 return (mon->suspend_cnt == 0) ? 128 : 0;
 }
 
+typedef struct CmdArgs {
+QString *name;
+int type;
+int flag;
+int optional;
+} CmdArgs;
+
+static int check_opt(const CmdArgs *cmd_args, const char *name)
+{
+if (!cmd_args->optional) {
+qemu_error_new(QERR_INVALID_PARAMETER, name, "parameter required");
+return -1;
+}
+
+return 0;
+}
+
+static int check_arg(const CmdArgs *cmd_args, const QDict *args)
+{
+QObject *value;
+const char *name;
+
+name = qstring_get_str(cmd_args->name);
+
+if (!args) {
+return check_opt(cmd_args, name);
+}
+
+value = qdict_get(args, name);
+if (!value) {
+return check_opt(cmd_args, name);
+}
+
+switch (cmd_args->type) {
+case 'F':
+case 'B':
+case 's':
+if (qobject_type(value) != QTYPE_QSTRING) {
+qemu_error_new(QERR_INVALID_PARAMETER, name,"must be a 
string");
+return -1;
+}
+break;
+case '/': {
+int i;
+const char *keys[] = { "count", "format", "size", NULL };
+
+for (i = 0; keys[i]; i++) {
+QObject *obj = qdict_get(args, keys[i]);
+if (!obj) {
+qemu_error_new(QERR_INVALID_PARAMETER, name,
+   "missing 'count', 'format' or 'size'");
+return -1;
+}
+if (qobject_type(obj) != QTYPE_QINT) {
+qemu_error_new(QERR_INVALID_PARAMETER, name,
+

[Qemu-devel] [PATCH 06/15] QMP: Output support

2009-11-19 Thread Luiz Capitulino
In the new Monitor, output is always done by only two
functions: do_info() and monitor_call_handler().

To support QMP output, we modify those functions to test if we
are in control mode. If so, we call monitor_protocol_emitter()
to emit QMP output, otherwise we do regular output.

QMP has two types of responses to issued commands: success and
error. The outputed data is always a JSON object.

Success responses have the following format:

{ "return": json-value, "id": json-value }

Error responses have the following format:

{ "error": { "class": json-string,
 "desc": json-string,
 "data": json-value } "id": json-value }

Please, note that the "id" key is part of the input code, and
thus is not added in this commit.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   56 +---
 1 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index be68dd5..e81f9e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -273,6 +273,33 @@ static void monitor_json_emitter(Monitor *mon, const 
QObject *data)
 QDECREF(json);
 }
 
+static void monitor_protocol_emitter(Monitor *mon, QObject *data)
+{
+QDict *qmp;
+
+qmp = qdict_new();
+
+if (!monitor_has_error(mon)) {
+/* success response */
+if (data) {
+qobject_incref(data);
+qdict_put_obj(qmp, "return", data);
+} else {
+// FIXME: should we return json-null here?
+qdict_put(qmp, "return", qstring_from_str("OK"));
+}
+} else {
+/* error response */
+QINCREF(mon->error->error);
+qdict_put(qmp, "error", mon->error->error);
+QDECREF(mon->error);
+mon->error = NULL;
+}
+
+monitor_json_emitter(mon, QOBJECT(qmp));
+QDECREF(qmp);
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
 const char *p, *pstart;
@@ -360,8 +387,15 @@ static void do_info(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 
 if (monitor_handler_ported(cmd)) {
 cmd->mhandler.info_new(mon, ret_data);
-if (*ret_data)
-cmd->user_print(mon, *ret_data);
+
+if (!monitor_ctrl_mode(mon)) {
+/*
+ * User Protocol function is called here, Monitor Protocol is
+ * handled by monitor_handle_command()
+ */
+if (*ret_data)
+cmd->user_print(mon, *ret_data);
+}
 } else {
 cmd->mhandler.info(mon);
 }
@@ -3292,8 +3326,15 @@ static void monitor_call_handler(Monitor *mon, const 
mon_cmd_t *cmd,
 QObject *data = NULL;
 
 cmd->mhandler.cmd_new(mon, params, &data);
-if (data)
-cmd->user_print(mon, data);
+
+if (monitor_ctrl_mode(mon)) {
+/* Monitor Protocol */
+monitor_protocol_emitter(mon, data);
+} else {
+/* User Protocol */
+ if (data)
+cmd->user_print(mon, data);
+}
 
 qobject_decref(data);
 }
@@ -3315,10 +3356,11 @@ static void monitor_handle_command(Monitor *mon, const 
char *cmdline)
 monitor_call_handler(mon, cmd, qdict);
 } else {
 cmd->mhandler.cmd(mon, qdict);
-}
 
-if (monitor_has_error(mon))
-monitor_print_error(mon);
+if (monitor_has_error(mon)) {
+monitor_print_error(mon);
+}
+}
 
 qemu_errors_to_previous();
 
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 05/15] QMP: chardev handling

2009-11-19 Thread Luiz Capitulino
This patch adds initial QMP support in QEMU. It's important
to notice that most QMP code will be part of the Monitor.

Input will be handled by monitor_control_read(). Currently it
reads the input and discards it, next patches add proper input
support.

The function monitor_json_emitter(), as its name implies, is
used by the Monitor to emit JSON output. In this commit it's
used by monitor_control_event() to print our greeting message.

Finally, control mode support is also added to monitor_init(),
allowing QMP to be really enabled.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   78 +++-
 1 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 07bd21c..be68dd5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -50,6 +50,7 @@
 #include "qdict.h"
 #include "qstring.h"
 #include "qerror.h"
+#include "qjson.h"
 
 //#define DEBUG
 //#define DEBUG_COMPLETION
@@ -92,6 +93,11 @@ struct mon_fd_t {
 QLIST_ENTRY(mon_fd_t) next;
 };
 
+typedef struct MonitorControl {
+uint8_t buf[1024];
+int size;
+} MonitorControl;
+
 struct Monitor {
 CharDriverState *chr;
 int mux_out;
@@ -101,6 +107,7 @@ struct Monitor {
 uint8_t outbuf[1024];
 int outbuf_index;
 ReadLineState *rs;
+MonitorControl *mc;
 CPUState *mon_cpu;
 BlockDriverCompletionFunc *password_completion_cb;
 void *password_opaque;
@@ -255,6 +262,17 @@ static void monitor_print_qobject(Monitor *mon, const 
QObject *data)
 monitor_puts(mon, "\n");
 }
 
+static void monitor_json_emitter(Monitor *mon, const QObject *data)
+{
+QString *json;
+
+json = qobject_to_json(data);
+assert(json != NULL);
+
+monitor_printf(mon, "%s\n", qstring_get_str(json));
+QDECREF(json);
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
 const char *p, *pstart;
@@ -3530,6 +3548,37 @@ static int monitor_can_read(void *opaque)
 return (mon->suspend_cnt == 0) ? 128 : 0;
 }
 
+/**
+ * monitor_control_read(): Read and handle QMP input
+ */
+static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
+{
+Monitor *old_mon = cur_mon;
+int i;
+
+cur_mon = opaque;
+
+for (i = 0; i < size; i++) {
+if (buf[i] == '\r' || buf[i] == '\n') {
+cur_mon->mc->buf[cur_mon->mc->size] = '\0';
+cur_mon->mc->size = 0;
+
+/* TODO: parse QMP input */
+break;
+} else {
+cur_mon->mc->buf[cur_mon->mc->size++] = buf[i];
+if (cur_mon->mc->size == sizeof(cur_mon->mc->buf)) {
+/* FIXME: qemu_error_new() */
+monitor_printf(cur_mon, "Command too long\n");
+cur_mon->mc->size = 0;
+break;
+}
+}
+}
+
+cur_mon = old_mon;
+}
+
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
 {
 Monitor *old_mon = cur_mon;
@@ -3573,6 +3622,24 @@ void monitor_resume(Monitor *mon)
 readline_show_prompt(mon->rs);
 }
 
+/**
+ * monitor_control_event(): Print QMP gretting on RESET
+ */
+static void monitor_control_event(void *opaque, int event)
+{
+if (event == CHR_EVENT_OPENED) {
+QObject *data;
+Monitor *mon = opaque;
+
+data = qobject_from_jsonf("{ 'QMP': { 'capabilities': [] } }");
+assert(data != NULL);
+monitor_json_emitter(mon, data);
+qobject_decref(data);
+
+mon->mc->size = 0;
+}
+}
+
 static void monitor_event(void *opaque, int event)
 {
 Monitor *mon = opaque;
@@ -3659,8 +3726,15 @@ void monitor_init(CharDriverState *chr, int flags)
 monitor_read_command(mon, 0);
 }
 
-qemu_chr_add_handlers(chr, monitor_can_read, monitor_read, monitor_event,
-  mon);
+if (monitor_ctrl_mode(mon)) {
+mon->mc = qemu_mallocz(sizeof(MonitorControl));
+/* Control mode requires special handlers */
+qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
+  monitor_control_event, mon);
+} else {
+qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
+  monitor_event, mon);
+}
 
 QLIST_INSERT_HEAD(&mon_list, mon, entry);
 if (!cur_mon || (flags & MONITOR_IS_DEFAULT))
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 04/15] QError: Add errors used by QMP

2009-11-19 Thread Luiz Capitulino
NOTE: These errors where added quickly to satisfy immediate needs,
they need to be reviewed.

Signed-off-by: Luiz Capitulino 
---
 qerror.c |   20 
 qerror.h |   15 +++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index d8b125e..4349417 100644
--- a/qerror.c
+++ b/qerror.c
@@ -52,6 +52,26 @@ const QErrorStringTable qerror_table[] = {
 .error_fmt = QERR_KVM_MISSING_CAP,
 .desc  = "Using KVM without %(capability), %(feature) unavailable",
 },
+{
+.error_fmt   = QERR_BAD_SYNTAX,
+.desc= "wrong syntax: '%(reason)",
+},
+{
+.error_fmt   = QERR_COMMAND_NOT_FOUND,
+.desc= "command: %(name)",
+},
+{
+.error_fmt   = QERR_COMMAND_NOT_ISSUED,
+.desc= "a command should be issued",
+},
+{
+.error_fmt   = QERR_INVALID_PARAMETER,
+.desc= "invalid parameter '%(parameter)': %(reason)",
+},
+{
+.error_fmt   = QERR_UNDEFINED_ERROR,
+.desc= "an error has ocurred: %(hint)",
+},
 {}
 };
 
diff --git a/qerror.h b/qerror.h
index 6c100af..1b704e4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -47,4 +47,19 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
 "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s 
} }"
 
+#define QERR_BAD_SYNTAX \
+"{ 'class': 'BadSyntax', 'data': { 'reason': %s } }"
+
+#define QERR_COMMAND_NOT_FOUND \
+"{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
+
+#define QERR_COMMAND_NOT_ISSUED \
+"{ 'class': 'CommandNotIssued', 'data': { } }"
+
+#define QERR_INVALID_PARAMETER \
+"{ 'class': 'InvalidParameter', 'data': { 'parameter': %s, 'reason': 
%s } }"
+
+#define QERR_UNDEFINED_ERROR \
+"{ 'class': 'UndefinedError', 'data': { 'hint': %s } }"
+
 #endif /* QERROR_H */
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 03/15] monitor: Move handler calling code to its own function

2009-11-19 Thread Luiz Capitulino
It's going to be used by QMP code as well.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   20 +---
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index a98dc42..07bd21c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3268,6 +3268,18 @@ static void monitor_print_error(Monitor *mon)
 mon->error = NULL;
 }
 
+static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
+ const QDict *params)
+{
+QObject *data = NULL;
+
+cmd->mhandler.cmd_new(mon, params, &data);
+if (data)
+cmd->user_print(mon, data);
+
+qobject_decref(data);
+}
+
 static void monitor_handle_command(Monitor *mon, const char *cmdline)
 {
 QDict *qdict;
@@ -3282,13 +3294,7 @@ static void monitor_handle_command(Monitor *mon, const 
char *cmdline)
 qemu_errors_to_mon(mon);
 
 if (monitor_handler_ported(cmd)) {
-QObject *data = NULL;
-
-cmd->mhandler.cmd_new(mon, qdict, &data);
-if (data)
-cmd->user_print(mon, data);
-
-qobject_decref(data);
+monitor_call_handler(mon, cmd, qdict);
 } else {
 cmd->mhandler.cmd(mon, qdict);
 }
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 02/15] monitor: Command-line flag to enable control mode

2009-11-19 Thread Luiz Capitulino
This commit adds a flag called 'control' to the '-monitor'
command-line option. This flag enables control mode.

The syntax is:

qemu [...] -monitor control,

Where  is a chardev (excluding 'vc', for obvious reasons).

For example:

$ qemu [...] -monitor control,tcp:localhost:,server

Will run QEMU in control mode, waiting for a client TCP connection
on localhost port .

TODO: Update manpage.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |   18 ++
 monitor.h |1 +
 vl.c  |   11 +++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index e7c6451..a98dc42 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3616,6 +3616,24 @@ static void monitor_event(void *opaque, int event)
  * End:
  */
 
+const char *monitor_cmdline_parse(const char *cmdline, int *flags)
+{
+const char *dev;
+
+if (strstart(cmdline, "control,", &dev)) {
+if (strstart(dev, "vc", NULL)) {
+fprintf(stderr, "qemu: control mode is for low-level interaction 
");
+fprintf(stderr, "cannot be used with device 'vc'\n");
+exit(1);
+}
+*flags &= ~MONITOR_USE_READLINE;
+*flags |= MONITOR_USE_CONTROL;
+return dev;
+}
+
+return cmdline;
+}
+
 void monitor_init(CharDriverState *chr, int flags)
 {
 static int is_first_init = 1;
diff --git a/monitor.h b/monitor.h
index 6cb1d4b..556507c 100644
--- a/monitor.h
+++ b/monitor.h
@@ -13,6 +13,7 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 
+const char *monitor_cmdline_parse(const char *cmdline, int *flags);
 void monitor_init(CharDriverState *chr, int flags);
 
 int monitor_suspend(Monitor *mon);
diff --git a/vl.c b/vl.c
index 73fe68d..6b1a77a 100644
--- a/vl.c
+++ b/vl.c
@@ -4639,6 +4639,7 @@ int main(int argc, char **argv, char **envp)
 const char *r, *optarg;
 CharDriverState *monitor_hds[MAX_MONITOR_DEVICES];
 const char *monitor_devices[MAX_MONITOR_DEVICES];
+int monitor_flags[MAX_MONITOR_DEVICES];
 int monitor_device_index;
 const char *serial_devices[MAX_SERIAL_PORTS];
 int serial_device_index;
@@ -4726,8 +4727,10 @@ int main(int argc, char **argv, char **envp)
 virtio_console_index = 0;
 
 monitor_devices[0] = "vc:80Cx24C";
+monitor_flags[0] = MONITOR_IS_DEFAULT | MONITOR_USE_READLINE;
 for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
 monitor_devices[i] = NULL;
+monitor_flags[i] = MONITOR_USE_READLINE;
 }
 monitor_device_index = 0;
 
@@ -5150,7 +5153,9 @@ int main(int argc, char **argv, char **envp)
 fprintf(stderr, "qemu: too many monitor devices\n");
 exit(1);
 }
-monitor_devices[monitor_device_index] = optarg;
+monitor_devices[monitor_device_index] =
+monitor_cmdline_parse(optarg,
+&monitor_flags[monitor_device_index]);
 monitor_device_index++;
 break;
 case QEMU_OPTION_chardev:
@@ -5844,9 +5849,7 @@ int main(int argc, char **argv, char **envp)
 
 for (i = 0; i < MAX_MONITOR_DEVICES; i++) {
 if (monitor_devices[i] && monitor_hds[i]) {
-monitor_init(monitor_hds[i],
- MONITOR_USE_READLINE |
- ((i == 0) ? MONITOR_IS_DEFAULT : 0));
+monitor_init(monitor_hds[i], monitor_flags[i]);
 }
 }
 
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [PATCH 01/15] monitor: Introduce MONITOR_USE_CONTROL flag

2009-11-19 Thread Luiz Capitulino
This flag will be set when Monitor enters "control mode", in
which the output will be defined by the QEMU Monitor Protocol.

This also introduces a macro to check if the flag is set.

Signed-off-by: Luiz Capitulino 
---
 monitor.c |6 ++
 monitor.h |1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 45ab883..e7c6451 100644
--- a/monitor.c
+++ b/monitor.c
@@ -119,6 +119,12 @@ Monitor *cur_mon = NULL;
 static void monitor_command_cb(Monitor *mon, const char *cmdline,
void *opaque);
 
+/* Return true if in control mode, false otherwise */
+static inline int monitor_ctrl_mode(const Monitor *mon)
+{
+return (mon->flags & MONITOR_USE_CONTROL);
+}
+
 static void monitor_read_command(Monitor *mon, int show_prompt)
 {
 readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
diff --git a/monitor.h b/monitor.h
index c7d2d0b..6cb1d4b 100644
--- a/monitor.h
+++ b/monitor.h
@@ -11,6 +11,7 @@ extern Monitor *cur_mon;
 /* flags for monitor_init */
 #define MONITOR_IS_DEFAULT0x01
 #define MONITOR_USE_READLINE  0x02
+#define MONITOR_USE_CONTROL   0x04
 
 void monitor_init(CharDriverState *chr, int flags);
 
-- 
1.6.5.3.148.g785c5





[Qemu-devel] [RFC v0 00/15] QEMU Monitor Protocol

2009-11-19 Thread Luiz Capitulino
 Hi,

 This is not stable yet, it has a few bugs and a number of things to
be done, but I'm sending it now so that it can get an initial review
while I'm working on it.

 At the end of the series there are two simple Python scripts which are
able to talk to QEMU by using QMP.

 Main issues are:

o Not all errors are being detected/handled correctly
o Not using the stream parser to read the input

 If you want to try this, you need at least the latest version of QError,
and the conversions series to make this really useful.

 Thanks.




[Qemu-devel] virtio: Report new guest memory statistics pertinent to memory ballooning (V4)

2009-11-19 Thread Adam Litke
Avi and Anthony,
If you agree that I've addressed all outstanding issues, please consider this
patch for inclusion.  Thanks.

Changes since V3:
 - Increase stat field size to 64 bits
 - Report all sizes in kb (not pages)
 - Drop anon_pages stat

Changes since V2:
 - Use a virtqueue for communication instead of the device config space

Changes since V1:
 - In the monitor, print all stats on one line with less abbreviated names
 - Coding style changes

When using ballooning to manage overcommitted memory on a host, a system for
guests to communicate their memory usage to the host can provide information
that will minimize the impact of ballooning on the guests.  The current method
employs a daemon running in each guest that communicates memory statistics to a
host daemon at a specified time interval.  The host daemon aggregates this
information and inflates and/or deflates balloons according to the level of
host memory pressure.  This approach is effective but overly complex since a
daemon must be installed inside each guest and coordinated to communicate with
the host.  A simpler approach is to collect memory statistics in the virtio
balloon driver and communicate them directly to the hypervisor.

This patch implements the qemu side of the communication channel.  I will post
the kernel driver modifications in-reply to this message.

Signed-off-by: Adam Litke 
Cc: Anthony Liguori 
Cc: Avi Kivity 
Cc: qemu-devel@nongnu.org

diff --git a/balloon.h b/balloon.h
index 60b4a5d..def4c56 100644
--- a/balloon.h
+++ b/balloon.h
@@ -16,12 +16,12 @@
 
 #include "cpu-defs.h"
 
-typedef ram_addr_t (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef int (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
 
 void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
 
 void qemu_balloon(ram_addr_t target);
 
-ram_addr_t qemu_balloon_status(void);
+int qemu_balloon_status(void);
 
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index cfd3b41..b90e1f2 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -19,6 +19,7 @@
 #include "balloon.h"
 #include "virtio-balloon.h"
 #include "kvm.h"
+#include "monitor.h"
 
 #if defined(__linux__)
 #include 
@@ -27,9 +28,13 @@
 typedef struct VirtIOBalloon
 {
 VirtIODevice vdev;
-VirtQueue *ivq, *dvq;
+VirtQueue *ivq, *dvq, *svq;
 uint32_t num_pages;
 uint32_t actual;
+uint64_t stats[VIRTIO_BALLOON_S_NR];
+VirtQueueElement stats_vq_elem;
+size_t stats_vq_offset;
+uint8_t stats_requested;
 } VirtIOBalloon;
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
@@ -46,6 +51,33 @@ static void balloon_page(void *addr, int deflate)
 #endif
 }
 
+static inline void reset_stats(VirtIOBalloon *dev)
+{
+int i;
+for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
+}
+
+static inline void print_stat(uint64_t val, const char *label)
+{
+if (val != -1) {
+monitor_printf(cur_mon, ",%s=%lld", label, (unsigned long long)val);
+}
+}
+
+static void virtio_balloon_print_stats(VirtIOBalloon *dev)
+{
+uint32_t actual = ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
+
+monitor_printf(cur_mon, "balloon: actual=%d", (int)(actual >> 20));
+print_stat(dev->stats[VIRTIO_BALLOON_S_SWAP_IN], "mem_swapped_in");
+print_stat(dev->stats[VIRTIO_BALLOON_S_SWAP_OUT], "mem_swapped_out");
+print_stat(dev->stats[VIRTIO_BALLOON_S_MAJFLT], "major_page_faults");
+print_stat(dev->stats[VIRTIO_BALLOON_S_MINFLT], "minor_page_faults");
+print_stat(dev->stats[VIRTIO_BALLOON_S_MEMFREE], "free_mem");
+print_stat(dev->stats[VIRTIO_BALLOON_S_MEMTOT], "total_mem");
+monitor_printf(cur_mon, "\n");
+}
+
 /* FIXME: once we do a virtio refactoring, this will get subsumed into common
  * code */
 static size_t memcpy_from_iovector(void *data, size_t offset, size_t size,
@@ -104,6 +136,31 @@ static void virtio_balloon_handle_output(VirtIODevice 
*vdev, VirtQueue *vq)
 }
 }
 
+static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
+{
+VirtIOBalloon *s = to_virtio_balloon(vdev);
+VirtQueueElement *elem = &s->stats_vq_elem;
+VirtIOBalloonStat stat;
+size_t offset = 0;
+
+if (!virtqueue_pop(vq, elem))
+return;
+
+while (memcpy_from_iovector(&stat, offset, sizeof(stat), elem->out_sg,
+elem->out_num) == sizeof(stat)) {
+offset += sizeof(stat);
+if (stat.tag < VIRTIO_BALLOON_S_NR)
+s->stats[stat.tag] = stat.val;
+}
+s->stats_vq_offset = offset;
+
+if (s->stats_requested) {
+virtio_balloon_print_stats(s);
+monitor_resume(cur_mon);
+s->stats_requested = 0;
+}
+}
+
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
 VirtIOBalloon *dev = to_virtio_balloon(vdev);
@@ -126,10 +183,19 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev)

[Qemu-devel] Re: Endless loop in qcow2_alloc_cluster_offset

2009-11-19 Thread Jan Kiszka
Kevin Wolf wrote:
> Hi Jan,
> 
> Am 19.11.2009 13:19, schrieb Jan Kiszka:
>> (gdb) print ((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
>> $5 = (struct QCowL2Meta *) 0xcb3568
>> (gdb) print *((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
>> $6 = {offset = 7417176064, n_start = 0, nb_available = 16, nb_clusters = 0, 
>> depends_on = 0xcb3568, dependent_requests = {lh_first = 0x0}, next_in_flight 
>> = {le_next = 0xcb3568, le_prev = 0xc4ebd8}}
>>
>> So next == first.
> 
> Oops. Doesn't sound quite right...
> 
>> Is something fiddling with cluster_allocs concurrently, e.g. some signal
>> handler? Or what could cause this list corruption? Would it be enough to
>> move to QLIST_FOREACH_SAFE?
> 
> Are there any specific signals you're thinking of? Related to block code

No, was just blind guessing.

> I can only think of SIGUSR2 and this one shouldn't call any block driver
> functions directly. You're using aio=threads, I assume? (It's the default)

Yes, all on defaults.

> 
> QLIST_FOREACH_SAFE shouldn't make a difference in this place as the loop
> doesn't insert or remove any elements. If the list is corrupted now, I
> think it would be corrupted with QLIST_FOREACH_SAFE as well - at best,
> the endless loop would occur one call later.
> 
> The only way I see to get such a loop in a list is to re-insert an
> element that already is part of the list. The only insert is at
> qcow2-cluster.c:777. Remains the question how we came there twice
> without run_dependent_requests() removing the L2Meta from our list first
> - because this is definitely wrong...
> 
> Presumably, it's not reproducible?

Likely not. What I did was nothing special, and I did not noticed such a
crash in the last months.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux




[Qemu-devel] Re: Endless loop in qcow2_alloc_cluster_offset

2009-11-19 Thread Kevin Wolf
Hi Jan,

Am 19.11.2009 13:19, schrieb Jan Kiszka:
> (gdb) print ((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
> $5 = (struct QCowL2Meta *) 0xcb3568
> (gdb) print *((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
> $6 = {offset = 7417176064, n_start = 0, nb_available = 16, nb_clusters = 0, 
> depends_on = 0xcb3568, dependent_requests = {lh_first = 0x0}, next_in_flight 
> = {le_next = 0xcb3568, le_prev = 0xc4ebd8}}
> 
> So next == first.

Oops. Doesn't sound quite right...

> Is something fiddling with cluster_allocs concurrently, e.g. some signal
> handler? Or what could cause this list corruption? Would it be enough to
> move to QLIST_FOREACH_SAFE?

Are there any specific signals you're thinking of? Related to block code
I can only think of SIGUSR2 and this one shouldn't call any block driver
functions directly. You're using aio=threads, I assume? (It's the default)

QLIST_FOREACH_SAFE shouldn't make a difference in this place as the loop
doesn't insert or remove any elements. If the list is corrupted now, I
think it would be corrupted with QLIST_FOREACH_SAFE as well - at best,
the endless loop would occur one call later.

The only way I see to get such a loop in a list is to re-insert an
element that already is part of the list. The only insert is at
qcow2-cluster.c:777. Remains the question how we came there twice
without run_dependent_requests() removing the L2Meta from our list first
- because this is definitely wrong...

Presumably, it's not reproducible?

Kevin




Re: [Qemu-devel] Re: [PATCH 00/10]: QError v4

2009-11-19 Thread Markus Armbruster
Paolo Bonzini  writes:

> On 11/19/2009 11:25 AM, Markus Armbruster wrote:
>> But I can't see that need in your particular example.  The client asks
>> the server to create a snapshot.  The operation fails.  The client
>> already knows what the name of the snapshot is, doesn't it?  No need to
>> get it from the error.
>
> Yeah, I just took a random error from grep so your objection makes
> total sense.  But is that still true when we have asynchronous
> notifications?

I don't know.  We'll figure it out when we get them.

Asynchronous notifications aren't errors, are they?  I'd expect them to
be different kinds of objects.  And then what makes sense for one of
them may or may not transfer to the other.




[Qemu-devel] [PATCH] Add SIZE type to qdev properties

2009-11-19 Thread Ian Molton
This patch adds a 'SIZE' type property to those available to qdevs.
It is the analogue of the OPT_SIZE property for options.

Signed-off-by: Ian Molton 
---
 hw/qdev-properties.c |   34 ++
 hw/qdev.h|4 
 parse_common.h   |   40 
 qemu-option.c|   23 +++
 4 files changed, 81 insertions(+), 20 deletions(-)
 create mode 100644 parse_common.h

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index bda6699..42f0d5c 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1,6 +1,7 @@
 #include "sysemu.h"
 #include "net.h"
 #include "qdev.h"
+#include "parse_common.h"
 
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
@@ -194,6 +195,39 @@ PropertyInfo qdev_prop_hex64 = {
 .print = print_hex64,
 };
 
+/* --- 64bit unsigned int 'size' type --- */
+
+static int parse_size(DeviceState *dev, Property *prop, const char *str)
+{
+uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+if(str != NULL)
+return parse_common_size(str, ptr);
+
+return -1;
+}
+
+static int print_size(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
+char suffixes[] = {'T', 'G', 'M', 'K', 'B'};
+int i;
+uint64_t div;
+
+for(div = (long int)1 << 40 ;
+!(*ptr / div) ; div >>= 10, i++);
+
+return snprintf(dest, len, "%0.03f%c", (double)*ptr/div, suffixes[i]);
+}
+
+PropertyInfo qdev_prop_size = {
+.name  = "size",
+.type  = PROP_TYPE_SIZE,
+.size  = sizeof(uint64_t),
+.parse = parse_size,
+.print = print_size,
+};
+
 /* --- string --- */
 
 static int parse_string(DeviceState *dev, Property *prop, const char *str)
diff --git a/hw/qdev.h b/hw/qdev.h
index 41642ee..5521093 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -74,6 +74,7 @@ enum PropertyType {
 PROP_TYPE_UINT32,
 PROP_TYPE_INT32,
 PROP_TYPE_UINT64,
+PROP_TYPE_SIZE,
 PROP_TYPE_TADDR,
 PROP_TYPE_MACADDR,
 PROP_TYPE_DRIVE,
@@ -189,6 +190,7 @@ extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_hex64;
+extern PropertyInfo qdev_prop_size;
 extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
 extern PropertyInfo qdev_prop_ptr;
@@ -226,6 +228,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
 DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
 #define DEFINE_PROP_HEX64(_n, _s, _f, _d)   \
 DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
+#define DEFINE_PROP_SIZE(_n, _s, _f, _d)   \
+DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_size, uint64_t)
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)   \
 DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
 
diff --git a/parse_common.h b/parse_common.h
new file mode 100644
index 000..93b1fc2
--- /dev/null
+++ b/parse_common.h
@@ -0,0 +1,40 @@
+/*
+ * Common parsing routines
+ *
+ * Copyright Collabora 2009
+ *
+ * Author:
+ *  Ian Molton 
+ *
+ * Derived from the parser in qemu-option.c
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+static inline int parse_common_size(const char *value, uint64_t *ptr) {
+char *postfix;
+double sizef;
+
+sizef = strtod(value, &postfix);
+switch (*postfix) {
+case 'T':
+sizef *= 1024;
+case 'G':
+sizef *= 1024;
+case 'M':
+sizef *= 1024;
+case 'K':
+case 'k':
+sizef *= 1024;
+case 'B':
+case 'b':
+case '\0':
+*ptr = (uint64_t) sizef;
+return 0;
+}
+
+return -1;
+}
+
diff --git a/qemu-option.c b/qemu-option.c
index 49efd39..efb8e5a 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -28,6 +28,7 @@
 
 #include "qemu-common.h"
 #include "qemu-option.h"
+#include "parse_common.h"
 
 /*
  * Extracts the name of an option from the parameter string (p points at the
@@ -203,28 +204,10 @@ static int parse_option_number(const char *name, const 
char *value, uint64_t *re
 
 static int parse_option_size(const char *name, const char *value, uint64_t 
*ret)
 {
-char *postfix;
-double sizef;
-
 if (value != NULL) {
-sizef = strtod(value, &postfix);
-switch (*postfix) {
-case 'T':
-sizef *= 1024;
-case 'G':
-sizef *= 1024;
-case 'M':
-sizef *= 1024;
-case 'K':
-case 'k':
-sizef *= 1024;
-case 'b':
-case '\0':
-*ret = (uint64_t) sizef;
-break;
-default:
+   if(parse_common_size(value, ret) == -1) {
 fprintf(stderr, "Option '%s' needs size as parameter\n", name);
-fprintf(stderr, "You may use k, M, G or T suffixes for "
+

[Qemu-devel] Re: Issues building seabios

2009-11-19 Thread Avi Kivity

On 11/19/2009 03:39 PM, Kevin O'Connor wrote:

On Thu, Nov 19, 2009 at 03:10:20PM +0200, Avi Kivity wrote:
   

Trying to debug the cdrom issue, I see

   Compiling whole program out/ccode32.o
src/util.c: In function ‘__end_thread’:
src/util.c:183: internal compiler error: in simplify_subreg, at
simplify-rtx.c:5055

(with F12's gcc (GCC) 4.4.2 20091027 (Red Hat 4.4.2-7))
 

I think this should be fixed in gcc - see:

https://bugzilla.redhat.com/show_bug.cgi?id=531385

Long story short - this is the result of gcc's "-combine" being
fragile.  It's possible to avoid by compiling seabios with "make
COMPSTRAT=1".  We may need to change the default.

   


That works.  Still complains about cdemu_drive and trampoline_checkirqs.


Ughh - did FC12 go out without this fix?
   


Yes, and 4.4.2-8 is not on updates-testing or rawhide.

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





Re: [Qemu-devel] [PATCH 00/13]: QError v5

2009-11-19 Thread Markus Armbruster
As far as I can see, this series addresses the review comments regarding
the current QError implementation.  I'm still concerned about QError
design, and my concerns are being discussed.  I hope we can wrap that up
soon and move on.




[Qemu-devel] Re: Issues building seabios

2009-11-19 Thread Kevin O'Connor
On Thu, Nov 19, 2009 at 03:10:20PM +0200, Avi Kivity wrote:
> Trying to debug the cdrom issue, I see
>
>   Compiling whole program out/ccode32.o
> src/util.c: In function ‘__end_thread’:
> src/util.c:183: internal compiler error: in simplify_subreg, at  
> simplify-rtx.c:5055
>
> (with F12's gcc (GCC) 4.4.2 20091027 (Red Hat 4.4.2-7))

I think this should be fixed in gcc - see:

https://bugzilla.redhat.com/show_bug.cgi?id=531385

Long story short - this is the result of gcc's "-combine" being
fragile.  It's possible to avoid by compiling seabios with "make
COMPSTRAT=1".  We may need to change the default.

Ughh - did FC12 go out without this fix?

-Kevin




[Qemu-devel] Re: [PATCH 00/10]: QError v4

2009-11-19 Thread Paolo Bonzini

On 11/19/2009 11:25 AM, Markus Armbruster wrote:

But I can't see that need in your particular example.  The client asks
the server to create a snapshot.  The operation fails.  The client
already knows what the name of the snapshot is, doesn't it?  No need to
get it from the error.


Yeah, I just took a random error from grep so your objection makes total 
sense.  But is that still true when we have asynchronous notifications?


Paolo





[Qemu-devel] Interactive Unix 4

2009-11-19 Thread Gustavo Alves
I'm trying to install Interactive Unix 4 with qemu 0.10.6, but there is some
problems when accessing the IDE disk. Everything appears to hang when the
system try to format the fs and all I can see on gdb is some interrupts
being executed. The fdisk utility works correctly.

As I never had experience before with qemu, how this problem can the
debugged? Someone had previous experience with this issue? Anyone can
give-me some directions?

Thanks

Gustavo


[Qemu-devel] Issues building seabios

2009-11-19 Thread Avi Kivity

Trying to debug the cdrom issue, I see

  Compiling whole program out/ccode32.o
src/util.c: In function ‘__end_thread’:
src/util.c:183: internal compiler error: in simplify_subreg, at 
simplify-rtx.c:5055


(with F12's gcc (GCC) 4.4.2 20091027 (Red Hat 4.4.2-7))

The issue seems to be with the pos variable; removing everything except 
the declaration + initialization retains the error, converting the 
variable to static removes it.  Presumably you've seen many of these?


With that out of the way, I get:

  Linking (no relocs) out/rom32.o
  Linking (no relocs) out/rom16.o
  Linking out/rom.o
`cdemu_drive' referenced in section `.text32' of out/rom32.o: defined in 
discarded section `.discard.var16.src/cdrom.c.107' of out/rom32.o
`cdemu_drive' referenced in section `.text32' of out/rom32.o: defined in 
discarded section `.discard.var16.src/cdrom.c.107' of out/rom32.o
`cdemu_drive' referenced in section `.text32' of out/rom32.o: defined in 
discarded section `.discard.var16.src/cdrom.c.107' of out/rom32.o


Given we're chasing a cdom related problem, this is suspicious, even 
though we aren't using cdemu.  Presumably access to uninitialized memory 
can confuse disk.c.


out/rom32.o: In function `check_irqs32':
/build/home/tlv/akivity/qemu-kvm/roms/seabios/src/util.c:109: undefined 
reference to `trampoline_checkirqs'


No idea where that came from.

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





Re: [Qemu-devel] [BUG] Changing SDL full screen mode no longer works

2009-11-19 Thread Aurelien Jarno
Stefan Weil a écrit :
> Stefan Weil schrieb:
>> Stefano Stabellini schrieb:
>>> Hi all,
>>> currently vga always resizes the screen when vga_hw_invalidate is called
>>> while this is not required and all the other graphic emulators don't.
>>> This patch fixes it, making vga invalidate behaviour consistent with the
>>> other emulated devices.
>>>
>>> Signed-off-by: Stefano Stabellini 
>> This rather old patch breaks switching to and from full screen mode
>> using SDL and alt-ctrl-f key.
>>
>> Please revert or fix it. The patch is also part of stable-0.11, so I
>> expect the same problem there (but did not test it).
>>
>> Found by git bisect, tested on several linux installations.
>>
> 
> Debian qemu 0.11.0-5 is suffering from this bug, too.
> 
> So 0bd8246bfec1dfb2eb959f52db535572c0260f4c
> should be reverted or fixed not only for master but
> also for stable-0.11.
> 

Before reverting that into stable-0.11, we should take time to
understand the problem and have a correct fix (which might be a revert).


-- 
Aurelien Jarno  GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net




[Qemu-devel] Re: [PATCH 07/10] Introduce QError

2009-11-19 Thread Paolo Bonzini

On 11/19/2009 09:42 AM, Markus Armbruster wrote:

>
>It's probably convenient to have qjson emitting QError, I'm unsure
>  if we should do that for all kinds of QObjects though.


For a general purpose system, I'd recommend to cover all types.  But as
long as this has just one user (QEMU), it can use the special purpose
excuse not to.


You're not sending QErrors on the wire yet, are you?  Once you do, I 
suspect it will be easiest to handle to_json for QError too (or make it 
a virtual method as it was in my patches...).


Paolo





[Qemu-devel] Endless loop in qcow2_alloc_cluster_offset

2009-11-19 Thread Jan Kiszka
Hi,

I just managed to push a qemu-kvm process (git rev. b496fe3431) into an
endless loop in qcow2_alloc_cluster_offset, namely over
QLIST_FOREACH(old_alloc, &s->cluster_allocs, next_in_flight):

(gdb) bt
#0  0x0048614b in qcow2_alloc_cluster_offset (bs=0xc4e1d0, 
offset=7417184256, n_start=0, n_end=16, num=0xcb351c, m=0xcb3568) at 
/data/qemu-kvm/block/qcow2-cluster.c:750
#1  0x004828d0 in qcow_aio_write_cb (opaque=0xcb34d0, ret=0) at 
/data/qemu-kvm/block/qcow2.c:587
#2  0x00482a44 in qcow_aio_writev (bs=, 
sector_num=, qiov=, nb_sectors=, cb=, opaque=) at 
/data/qemu-kvm/block/qcow2.c:645
#3  0x00470e89 in bdrv_aio_writev (bs=0xc4e1d0, sector_num=2, 
qiov=0x7f48a9010ed0, nb_sectors=16, cb=0x470d20 , 
opaque=0x7f48a9010f0c) at /data/qemu-kvm/block.c:1362
#4  0x00472991 in bdrv_write_em (bs=0xc4e1d0, sector_num=14486688, 
buf=0xd67200 "H\a", nb_sectors=16) at /data/qemu-kvm/block.c:1736
#5  0x00435581 in ide_sector_write (s=0xc92650) at 
/data/qemu-kvm/hw/ide/core.c:622
#6  0x00425fc2 in kvm_handle_io (env=) at 
/data/qemu-kvm/kvm-all.c:553
#7  kvm_run (env=) at /data/qemu-kvm/qemu-kvm.c:964
#8  0x00426049 in kvm_cpu_exec (env=0x1000) at 
/data/qemu-kvm/qemu-kvm.c:1651
#9  0x0042627d in kvm_main_loop_cpu (_env=) at 
/data/qemu-kvm/qemu-kvm.c:1893
#10 ap_main_loop (_env=) at /data/qemu-kvm/qemu-kvm.c:1943
#11 0x7f48ae89d070 in start_thread () from /lib64/libpthread.so.0
#12 0x7f48abf0711d in clone () from /lib64/libc.so.6
#13 0x in ?? ()
(gdb) print ((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
$5 = (struct QCowL2Meta *) 0xcb3568
(gdb) print *((BDRVQcowState *)bs->opaque)->cluster_allocs.lh_first 
$6 = {offset = 7417176064, n_start = 0, nb_available = 16, nb_clusters = 0, 
depends_on = 0xcb3568, dependent_requests = {lh_first = 0x0}, next_in_flight = 
{le_next = 0xcb3568, le_prev = 0xc4ebd8}}

So next == first.

Is something fiddling with cluster_allocs concurrently, e.g. some signal
handler? Or what could cause this list corruption? Would it be enough to
move to QLIST_FOREACH_SAFE?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux




Re: [Qemu-devel] Re: [PATCH 00/10]: QError v4

2009-11-19 Thread Markus Armbruster
Paolo Bonzini  writes:

>> 2. It falls short of the requirement that clients can reasonably
>> classify errors they don't know.
>
> True.  (Though adding a classification mechanism can be done later
> once we have an idea of what errors are there at all).

Yes.

If we think there's a risk screwing up the classification, because we
don't yet understand the errors sufficiently, and we think a screwup
would be is awkward to fix later, then deferring to a later iteration
would make sense.

>> 3. It falls short of the requirement that clients can easily present a
>> human-readable error description to their human users, regardless of
>> whether they know the error or not.
>
> That's true.  I would definitely have the interpolated desc field go
> on the wire too, as part of the JSON form of QError.

That would be my preference, too.

>> If I understand Dan correctly, machine-readable error code +
>> human-readable description is just fine, as long as the error code is
>> reasonably specific and the description is precise and complete.  Have
>> we heard anything else from client developers?
>
> Then (except for the asynchronicity) the current qemu monitor protocol
> is also just fine, including the fact that we send migrate and get
> m\rmi\rmig\rmigr etc. back on the socket.
>
> "error while creating snapshot on '%s'" may be easy to parse, but
> looking at a dictionary field in { "filename" : "blah.img" } is
> easier, and at the same time solves issues with escaping weird
> characters in the file name.

As soon as the client needs to know more than a (reasonably specific)
error code, structured error data helps.

But I can't see that need in your particular example.  The client asks
the server to create a snapshot.  The operation fails.  The client
already knows what the name of the snapshot is, doesn't it?  No need to
get it from the error.

>> * The crucial question for the client isn't "what exactly went wrong".
>>It is "how should I handle this error".  Answering that question
>>should be easy (say, check the error code).  Figuring out what went
>>wrong should still be possible for a human operator of the client.
>
> The same error can be handled in a different way depending on the
> situation.  A missing image is in general a fatal error, but maybe
> another client could create images on the fly.

The latter client surely knows this error.

My question is how to help clients handle errors they don't know.  In
your example, the help would boil down to "this error is generally
fatal".

>> Based on what I've learned about client requirements so far, I figure
>> that solution is "easily classified error code + human-readable
>> description".
>
> How would you classify the error code?  By subsystem or by (for
> example) severity or anything else?  Does QEMU have subsystems that
> are well-identified enough to be carved in stone in QError?  (All
> genuine questions.  I have no idea).

My first cut (really just a shot from the hip) was[*]:

Something like client error (i.e. your command was stupid, and
trying again will be just as stupid), permanent server error (it
wasn't stupid, but I can't do it for you), transient server error (I
couldn't do it for you, but trying again later could help).


[*] Message-ID: <877huy6hzm@pike.pond.sub.org>




Re: [Qemu-devel] Re: SeaBIOS cdrom regression with Vista

2009-11-19 Thread Avi Kivity

On 11/19/2009 05:24 AM, Kevin O'Connor wrote:

On Wed, Nov 18, 2009 at 12:19:20AM -0500, Kevin O'Connor wrote:
   

On Tue, Nov 17, 2009 at 03:21:31PM +0200, Avi Kivity wrote:
 

qemu-kvm's switch to seabios uncovered a regression with cdrom handling.
Vista x64 no longer recognizes the cdrom, while pc-bios still works.
Installing works, but that uses int 13, not the native driver.  Haven't
investigated further yet.
   

Thanks for reporting Avi.  I will take a look as well.
 

I'm able to reproduce this problem on my machine with a 32bit vista
install.  I'll let you know what I find.
   


This reproduces with qemu upstream as well.  The Windows error log 
complains that the cdrom driver wouldn't load, but nothing else.


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





Re: [Qemu-devel] [PATCH 00/10]: QError v4

2009-11-19 Thread Markus Armbruster
Anthony Liguori  writes:

> Markus Armbruster wrote:
>> 1. QError feels overengineered, particularly for a first iteration of a
>>protocol.
>>
>>We go to great lengths to build highly structured error objects.
>>There is only one sane reason for doing that: a demonstrated client
>>need.  I can't see that need.
>>   
>
> A GUI wants to present the user a mechanism to reduce the memory
> consumption of a VM.  There are three reasons why this would not work.
> The first is that ballooning was not configured for this guest.  The
> second is that ballooning was configured but the version of the KVM
> kernel module does not have a sync mmu.  The third is that an
> appropriate guest driver has not been loaded for the device.
>
> If we don't specify a type of error, all the GUI can do is fail right
> before trying to balloon the guest.  That offers the user no insight
> why it failed other than popping up a message box with an error
> message (that may not even be in their locale).  For a non-english
> speaker, it's gibberish.
>
> If you use three types of errors, then a GUI can differentiate these
> cases and presents the user different feedback.  For the first case,
> it can simply gray out the menu item.  For the second can, it can also
> gray out the menu item but it can provide a one time indication (in
> say, a status bar), that ballooning is unavailable because the kernel
> version does not support it.  For the third error, you would want to
> prompt the user to indicate the driver is not loaded in the guest.

So far, this is a convincing argument for reasonably specific error
codes, and on that we're in violent agreement.  What you didn't
demonstrate here is a need for *structured* error reporting,
i.e. machine-readable data beyond a simple error code.

> It's highly likely that for this last case, you'd want generic code to
> generate this error.  Further more, in order to generate the error
> message for a user, you need to know what device does not have a
> functioning driver.  You may say it's obvious for something like info
> balloon but it's not so obvious if you assume we support things other
> than just virtio-pci-balloon.  For instance, with s390, it's a
> different balloon driver.  For xenner/xenpv, it's a different driver.
> It really suggests we should send the qdev device name for the current
> balloon driver.

This is your argument for structured error reporting.  It boils down to
client localization.

I challenge the idea that we should even try to get that covered now.
We need to get the basics right, and the only way to do that is to use
them in anger.  The more baggage we design into the thing before we use
it, the higher the risk that we screw it up.

With our JSON encoding we can easily extend the error object without
breaking existing users.  It's designed for growth.  One more reason not
to try to anticipate everything.

Iteration beats the committee.  Let's iterate.

>> 2. It falls short of the requirement that clients can reasonably
>>classify errors they don't know.
>>   
>
> I think this is wishful thinking.  I strongly suspect that you can't
> find a reasonably popular web browser that doesn't know all of the
> error codes that web servers generate.

I'd argue that that wasn't the case while HTTP was still evolving, and I
bet it won't be the case while QMP is evolving.

> We should document all of the errors that each QMP function can return.

Yes, we should.

> That said, if you thought there was 4-5 common classes of errors,
> adding an additional 'base_class' field could be a completely
> reasonable thing to do.  As long as you're adding information
> vs. taking it away, I'm quite happy.

Fair enough.

HTTP and FTP encode the error class into the error code.  Clever, but
since we decided to use textual error codes, a separate textual error
class probably makes the most sense.

>> 3. It falls short of the requirement that clients can easily present a
>>human-readable error description to their human users, regardless of
>>whether they know the error or not.
>>   
>
> That's just incorrect.  We provide an example conversion table that's
> akin to strerror() or a __repr__ for an exception in Python.

As Jamie already said, that won't work unless we transfer the table over
the wire, or require client to match server perfectly.  I doubt you want
the latter.  Did you mean to suggest the former?

>> In more detail[*]:
>>
>> There are more than one way to screw up here.  One is certainly to fall
>> short of client requirements in a way that is costly to fix (think
>> incompatible protocol revision).  Another is to overshoot them in a way
>> that is costly to maintain.  A third one is to spend too much time on
>> figuring out the perfect solution.
>>
>> I believe our true problem is that we're still confused and/or
>> disagreeing on client requirements, and this has led to a design that
>> tries to cover all the conceivable bases, and feels overengineered to

[Qemu-devel] [PATCH 5/7] lsi53c895a: Fix SDID in SELECT ID command

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

See SCRIPTS Programming Guide, 3.2.17 SELECT.

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 2fb928d..4c638be 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1028,7 +1028,7 @@ again:
 if (insn & (1 << 25)) {
 id = read_dword(s, s->dsa + sxt24(insn));
 } else {
-id = addr;
+id = insn;
 }
 id = (id >> 16) & 0xf;
 if (insn & (1 << 26)) {





[Qemu-devel] [PATCH 7/7] lsi53c895a: Implement IRQ on reselection

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

The critical part of this change is how to deal with simultaneaous
generation of interrupts. The only (normal) case when this happens in
the emulation is near simultaneous reselection + selection. If selection
comes first, there is no problem, since the target attempting
reselection loses the arbitration (in the emulation it only means that
the reselect function will not be started). In the worst case the host
adapter is reselected, but the device driver already started a
selection, so we jump to the alternative address to handle the
situation.

The SCRIPTS code can trigger another interrupt to notify the driver that
the new task has to be postponed. I suppose that on real hardware there
is enough time after the reselection interrupt to set the SIP bit before
the next interrupt comes, so it would result in 2 stacked interrupts (a
SCSI and a DMA one). However, in the emulation there is no interrupt
stacking, so there is a good chance that the 2 interrupts will get to
the interrupt handler at the same time.

Nevertheless, it should not make a big difference in interrupt handling,
since in both cases both interrupts have to be fetched first, and after
that the new task (that failed during the selection phase) has to be
prepared/reset for a later restart, and the reconnected device has to be
serviced.

The changes do not modify the host adapter's behavior if this interrupt
is not enabled.

See also LSI53C895A technical manual, SCID and SIEN0.

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |   43 ---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 480fbca..32236aa 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -152,6 +152,9 @@ do { fprintf(stderr, "lsi_scsi: error: " fmt , ## 
__VA_ARGS__);} while (0)
 #define LSI_CCNTL1_DDAC  0x08
 #define LSI_CCNTL1_ZMOD  0x80
 
+/* Enable Response to Reselection */
+#define LSI_SCID_RRE  0x60
+
 #define LSI_CCNTL1_40BIT (LSI_CCNTL1_EN64TIBMV|LSI_CCNTL1_64TIMOD)
 
 #define PHASE_DO  0
@@ -270,6 +273,11 @@ typedef struct {
 uint32_t script_ram[2048];
 } LSIState;
 
+static inline int lsi_irq_on_rsl(LSIState *s)
+{
+return (s->sien0 & LSI_SIST0_RSL) && (s->scid & LSI_SCID_RRE);
+}
+
 static void lsi_soft_reset(LSIState *s)
 {
 DPRINTF("Reset\n");
@@ -360,6 +368,7 @@ static int lsi_dma_64bit(LSIState *s)
 static uint8_t lsi_reg_readb(LSIState *s, int offset);
 static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val);
 static void lsi_execute_script(LSIState *s);
+static void lsi_reselect(LSIState *s, uint32_t tag);
 
 static inline uint32_t read_dword(LSIState *s, uint32_t addr)
 {
@@ -380,6 +389,7 @@ static void lsi_stop_script(LSIState *s)
 
 static void lsi_update_irq(LSIState *s)
 {
+int i;
 int level;
 static int last_level;
 
@@ -411,6 +421,17 @@ static void lsi_update_irq(LSIState *s)
 last_level = level;
 }
 qemu_set_irq(s->dev.irq[0], level);
+
+if (!level && lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON)) {
+DPRINTF("Handled IRQs & disconnected, looking for pending "
+"processes\n");
+for (i = 0; i < s->active_commands; i++) {
+if (s->queue[i].pending) {
+lsi_reselect(s, s->queue[i].tag);
+break;
+}
+}
+}
 }
 
 /* Stop SCRIPTS execution and raise a SCSI interrupt.  */
@@ -605,6 +626,10 @@ static void lsi_reselect(LSIState *s, uint32_t tag)
 if (n != s->active_commands) {
 s->queue[n] = s->queue[s->active_commands];
 }
+
+if (lsi_irq_on_rsl(s)) {
+lsi_script_scsi_interrupt(s, LSI_SIST0_RSL, 0);
+}
 }
 
 /* Record that data is available for a queued command.  Returns zero if
@@ -620,7 +645,14 @@ static int lsi_queue_tag(LSIState *s, uint32_t tag, 
uint32_t arg)
 BADF("Multiple IO pending for tag %d\n", tag);
 }
 p->pending = arg;
-if (s->waiting == 1) {
+/* Reselect if waiting for it, or if reselection triggers an IRQ
+   and the bus is free.
+   Since no interrupt stacking is implemented in the emulation, it
+   is also required that there are no pending interrupts waiting
+   for service from the device driver. */
+if (s->waiting == 1 ||
+(lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
+ !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP {
 /* Reselect device.  */
 lsi_reselect(s, tag);
 return 0;
@@ -657,10 +689,13 @@ static void lsi_command_complete(SCSIBus *bus, int 
reason, uint32_t tag,
 return;
 }
 
-if (s->waiting == 1 || tag != s->current_tag) {
+if (s->waiting == 1 || tag != s->current_tag ||
+(lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
 if (lsi_queue_

[Qemu-devel] [PATCH 2/7] lsi53c895a: Whitespace and typo fixes

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 7e47fc9..e618bf2 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -10,7 +10,7 @@
 /* ??? Need to check if the {read,write}[wl] routines work properly on
big-endian targets.  */
 
-#include  \
+#include 
 
 #include "hw.h"
 #include "pci.h"
@@ -183,7 +183,7 @@ typedef struct {
 int carry; /* ??? Should this be an a visible register somewhere?  */
 int sense;
 /* Action to take at the end of a MSG IN phase.
-   0 = COMMAND, 1 = disconect, 2 = DATA OUT, 3 = DATA IN.  */
+   0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
 int msg_action;
 int msg_len;
 uint8_t msg[LSI_MAX_MSGIN_LEN];
@@ -1060,7 +1060,7 @@ again:
 lsi_set_phase(s, PHASE_MO);
 break;
 case 1: /* Disconnect */
-DPRINTF("Wait Disconect\n");
+DPRINTF("Wait Disconnect\n");
 s->scntl1 &= ~LSI_SCNTL1_CON;
 break;
 case 2: /* Wait Reselect */
@@ -1552,9 +1552,9 @@ static void lsi_reg_writeb(LSIState *s, int offset, 
uint8_t val)
SCRIPTS register move instructions are.  */
 s->sfbr = val;
 break;
-case 0x0a: case 0x0b: 
+case 0x0a: case 0x0b:
 /* Openserver writes to these readonly registers on startup */
-   return;
+   return;
 case 0x0c: case 0x0d: case 0x0e: case 0x0f:
 /* Linux writes to these readonly registers on startup.  */
 return;





[Qemu-devel] [PATCH 4/7] lsi53c895a: Fix message code of DISCONNECT

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

See SCSI-2, 6.5 Message system description/message codes.

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index ee69b0a..2fb928d 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -788,7 +788,7 @@ static void lsi_do_msgout(LSIState *s)
 s->sfbr = msg;
 
 switch (msg) {
-case 0x00:
+case 0x04:
 DPRINTF("MSG: Disconnect\n");
 lsi_disconnect(s);
 break;





[Qemu-devel] [PATCH 1/7] SCSI: Fix Standard INQUIRY data

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

Vendor identification, product identification and product revision level
should be padded with spaces without a terminating NULL character, see
SCSI-2 standard, 8.2.5.1 Standard INQUIRY data.

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/scsi-disk.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a92b62f..f3f4fda 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -593,12 +593,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t 
tag,
} else if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM) {
outbuf[0] = 5;
 outbuf[1] = 0x80;
-   memcpy(&outbuf[16], "QEMU CD-ROM", 16);
+   memcpy(&outbuf[16], "QEMU CD-ROM ", 16);
} else {
outbuf[0] = 0;
-   memcpy(&outbuf[16], "QEMU HARDDISK  ", 16);
+   memcpy(&outbuf[16], "QEMU HARDDISK   ", 16);
}
-   memcpy(&outbuf[8], "QEMU   ", 8);
+   memcpy(&outbuf[8], "QEMU", 8);
 memcpy(&outbuf[32], QEMU_VERSION, 4);
 /* Identify device as SCSI-3 rev 1.
Some later commands are also implemented. */





[Qemu-devel] [PATCH 3/7] lsi53c895a: Add support for LSI53C700 Family Compatibility bit

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index e618bf2..ee69b0a 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -583,6 +583,10 @@ static void lsi_reselect(LSIState *s, uint32_t tag)
 }
 id = (tag >> 8) & 0xf;
 s->ssid = id | 0x80;
+/* LSI53C700 Family Compatibility, see LSI53C895A 4-73 */
+if (!s->dcntl & LSI_DCNTL_COM) {
+s->sfbr = 1 << (id & 0x7);
+}
 DPRINTF("Reselected target %d\n", id);
 s->current_dev = s->bus.devs[id];
 s->current_tag = tag;





[Qemu-devel] [PATCH 6/7] lsi53c895a: Use alternative address when already reselected

2009-11-19 Thread Jan Kiszka
From: Laszlo Ast 

See SCRIPTS, 3.2.17 SELECT.

Signed-off-by: Laszlo Ast 
Signed-off-by: Jan Kiszka 
---

 hw/lsi53c895a.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 4c638be..480fbca 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1038,8 +1038,9 @@ again:
 switch (opcode) {
 case 0: /* Select */
 s->sdid = id;
-if (s->current_dma_len && (s->ssid & 0xf) == id) {
-DPRINTF("Already reselected by target %d\n", id);
+if (s->scntl1 & LSI_SCNTL1_CON) {
+DPRINTF("Already reselected, jumping to alternative 
address\n");
+s->dsp = s->dnad;
 break;
 }
 s->sstat0 |= LSI_SSTAT0_WOA;





[Qemu-devel] [PATCH 0/7] scsi & lsi53c895a fixes

2009-11-19 Thread Jan Kiszka
This series is required to use the lsi53c895a emulation with an old
proprietary driver. All changes should move the emulation closer to the
spec and the real hardware.

Find the patches also at git://git.kiszka.org/qemu.git queues/scsi

Laszlo Ast (7):
  SCSI: Fix Standard INQUIRY data
  lsi53c895a: Whitespace and typo fixes
  lsi53c895a: Add support for LSI53C700 Family Compatibility bit
  lsi53c895a: Fix message code of DISCONNECT
  lsi53c895a: Fix SDID in SELECT ID command
  lsi53c895a: Use alternative address when already reselected
  lsi53c895a: Implement IRQ on reselection

 hw/lsi53c895a.c |   66 +--
 hw/scsi-disk.c  |6 ++--
 2 files changed, 57 insertions(+), 15 deletions(-)






Re: [Qemu-devel] Fwd: qemu code review

2009-11-19 Thread Kevin Wolf
Am 18.11.2009 20:06, schrieb Stefan Weil:
> Kevin Wolf schrieb:
>> Hi all,
>>
>> as Steve suggests, I'm forwarding the list of issues he found to the
>> mailing list. I've already looked at a few points in the block code and
>> sent patches. If everyone picks up one point, we should get through the
>> list quickly. Who volunteers for the TCG ones? ;-)
>>
>> Kevin
>>
>>  Original-Nachricht 
>> Betreff: [virt-devel] qemu code review
>> Datum: Tue, 17 Nov 2009 14:05:33 -0500
>> Von: Steve Grubb 
>>
>> Hello,
>>
>> I took a few hours to run qemu through an analysis tool. Below are the
>> results
>> of checking everything. I don't interact with the qemu community and
>> thought
>> someone here might want to take these finding upstream. The review was
>> against
>> 0.11.0-11 in rawhide.
>>
>> Thanks,
>> -Steve
>>
>> -
>>
>> ...
>> In hw/e1000.c at line 89, vlan is declared to be 4 bytes. At line 382 is an
>> attempt to do a memmove over it with a size of 12.
>>   
> 
> Obviously this was intentional. Would replacing
> memmove(tp->vlan, tp->data, 12);
> by
> memmove(tp->data - 4, tp->data, 12);
> be better and satisfy the analysis tool? Or even better
> (hopefully the compiler will combine both statements)
> memmove(tp->vlan, tp->data, 4);
> memmove(tp->data, tp->data + 4, 8);

I don't even know which analysis tool he used. Steve?

But I think splitting it into two memmoves is better anyway. There is no
warning in the declaration of the struct that these fields need to be
consecutive, so someone might have the idea of reordering the fields or
inserting a new one in between and things break...

Kevin




Re: [Qemu-devel] [PATCH 07/10] Introduce QError

2009-11-19 Thread Markus Armbruster
Luiz Capitulino  writes:

> On Wed, 18 Nov 2009 16:16:11 +0100
> Markus Armbruster  wrote:
>
>> Luiz Capitulino  writes:
>
> [...]
>
>> > +static const char *append_field(QString *outstr, const QError *qerror,
>> > +const char *start)
>> > +{
>> > +QObject *obj;
>> > +QDict *qdict;
>> > +QString *key_qs;
>> > +const char *end, *key;
>> > +
>> > +if (*start != '%')
>> > +parse_error(qerror, '%');
>> 
>> Can't happen, because it gets called only with *start == '%'.  Taking
>> pointer to the character following the '%' as argument would sidestep
>> the issue.  But I'm fine with leaving it as is.
>
>  It's just an assertion.

It's not coded as an assertion.  If we ever do coverage testing, it'll
stick out.  But again, I'm fine with it.

>> > +start++;
>> > +if (*start != '(')
>> > +parse_error(qerror, '(');
>> > +start++;
>> > +
>> > +end = strchr(start, ')');
>> > +if (!end)
>> > +parse_error(qerror, ')');
>> > +
>> > +key_qs = qstring_from_substr(start, 0, end - start - 1);
>> > +key = qstring_get_str(key_qs);
>> > +
>> > +qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
>> > +obj = qdict_get(qdict, key);
>> > +if (!obj) {
>> > +qerror_abort(qerror, "key '%s' not found in QDict", key);
>> > +}
>> > +
>> > +switch (qobject_type(obj)) {
>> > +case QTYPE_QSTRING:
>> > +qstring_append(outstr, qdict_get_str(qdict, key));
>> > +break;
>> > +case QTYPE_QINT:
>> > +qstring_append_int(outstr, qdict_get_int(qdict, key));
>> > +break;
>> > +default:
>> > +qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
>> > +}
>> > +
>> > +QDECREF(key_qs);
>> 
>> Looks like you create key_qs just because it's a convenient way to
>> extract key zero-terminated.  Correct?
>
>  Yes, as a substring of 'desc', which is passed through 'start'.

Funny that the convenient way to extract a substring is to go through
QString.  Fine with me.

> [...]
>
>> > diff --git a/qjson.c b/qjson.c
>> > index 12e6cf0..60c904d 100644
>> > --- a/qjson.c
>> > +++ b/qjson.c
>> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
>> >  }
>> >  break;
>> >  }
>> > +case QTYPE_QERROR:
>> > +/* XXX: should QError be emitted? */
>> 
>> Pros & cons?
>
>  It's probably convenient to have qjson emitting QError, I'm unsure
> if we should do that for all kinds of QObjects though.

For a general purpose system, I'd recommend to cover all types.  But as
long as this has just one user (QEMU), it can use the special purpose
excuse not to.

>> >  case QTYPE_NONE:
>> >  break;
>> >  }
>> > diff --git a/qobject.h b/qobject.h
>> > index 2270ec1..07de211 100644
>> > --- a/qobject.h
>> > +++ b/qobject.h
>> > @@ -43,6 +43,7 @@ typedef enum {
>> >  QTYPE_QLIST,
>> >  QTYPE_QFLOAT,
>> >  QTYPE_QBOOL,
>> > +QTYPE_QERROR,
>> >  } qtype_code;
>> >  
>> >  struct QObject;
>> 
>> Erroneous QERRs are detected only when they're passed to
>> qerror_from_info() at run-time, i.e. when the error happens.  Likewise
>> for erroneous qerror_table[].desc.  Perhaps a unit test to ensure
>> qerror_table[] is sane would make sense.  Can't protect from passing
>> unknown errors to qerror_from_info(), but that shouldn't be a problem in
>> practice.
>
>  We could also have a debug function that could run once at startup
> and do the check.

Yes.