Re: [Qemu-devel] [PATCH][STABLE] Fix corner case in chardev udp: parameter

2010-01-18 Thread Gerd Hoffmann

On 01/15/10 21:07, Jan Kiszka wrote:

The missing '@' broke 'udp::port@:port' parsing.



  if (sscanf(p, %64[^:]:%32[^@,]%n, host, port,pos)  2) {
  host[0] = 0;
-if (sscanf(p, :%32[^,]%n, port,pos)  1) {
+if (sscanf(p, :%32[^@,]%n, port,pos)  1) {


Indeed.


  fprintf(stderr, udp #1\n);


While you are at it, can you also zap this debug leftover?

thanks,
  Gerd





Re: [Qemu-devel] [PATCH] Documentation: Add missing documentation for qdev related command line options

2010-01-18 Thread Markus Armbruster
Stefan Weil w...@mail.berlios.de writes:

 The command line options -device, -nodefaults, -readconfig,
 -writeconfig had entries for command line help, but
 documentation for texi and derived formats (man, html, info)
 was missing.

 This also required moving @end table to the end of
 qemu-options.hx again.

 Signed-off-by: Stefan Weil w...@mail.berlios.de
 ---
  qemu-options.hx |   25 +
  1 files changed, 21 insertions(+), 4 deletions(-)

 diff --git a/qemu-options.hx b/qemu-options.hx
 index e2edd71..b2d04e2 100644
 --- a/qemu-options.hx
 +++ b/qemu-options.hx
 @@ -404,6 +404,12 @@ ETEXI
  
  DEF(device, HAS_ARG, QEMU_OPTION_device,
  -device driver[,options]  add device\n)
 +STEXI
 +...@item -device @var{driver}[,@var{option}[,...]]
 +Add device @var{driver}. Depending on the device type,
 +...@var{option} (typically @var{ke...@var{value}) may be useful.
 +ETEXI
 +

While there, would you mind improving --help for -device a bit?  It's
too terse, and it doesn't start the help text in column 16 like the
other options do.

  DEF(name, HAS_ARG, QEMU_OPTION_name,
[...]
 +STEXI
 +...@item -writeconfig @var{file}
 +Write device configuration to @var{file}.
 +ETEXI
 +
 +HXCOMM This is the last statement. Insert new options before this line!
 +STEXI
 +...@end table
 +ETEXI

Neat, thanks!




Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

2010-01-18 Thread Markus Armbruster
Nathan Baum nat...@parenthephobia.org.uk writes:

 On Fri, 2010-01-15 at 19:14 +0100, Markus Armbruster wrote:
 Nathan Baum nat...@parenthephobia.org.uk writes:
 
  Returns a QObject with information about a USB device.
 
  Signed-off-by: Nathan Baum nat...@parenthephobia.org.uk
  ---
   hw/usb-bus.c |   13 +
   1 files changed, 13 insertions(+), 0 deletions(-)
 
  diff --git a/hw/usb-bus.c b/hw/usb-bus.c
  index 54027df..6d02807 100644
  --- a/hw/usb-bus.c
  +++ b/hw/usb-bus.c
  @@ -3,6 +3,7 @@
   #include qdev.h
   #include sysemu.h
   #include monitor.h
  +#include qjson.h
   
   static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int 
  indent);
   
  @@ -232,6 +233,18 @@ static void usb_bus_dev_print(Monitor *mon, 
  DeviceState *qdev, int indent)
  dev-attached ? , attached : );
   }
   
  +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
  +{
  +USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
  +USBBus *bus = usb_bus_from_device(dev);
  +return qobject_from_jsonf({'busnr': %d, 'addr':%d, 'speed': %s, 
  'desc': %s, 'attached': %i},
  +  bus-busnr,
 
 As for PCI, 'busnr' belongs to the bus, not the device.

 Hmm. In cases like this, is it appropriate to modify the output of the
 existing info qtree when it is modified to use the QObject data?

 Would it be sensible to go the (probably small amount of) effort to
 change the print functions to print exactly they do now, and put changes
 to their output in different patches so they can easily be dropped if
 necessary?

We might want to change info qtree output anyway if we show bus
information separately there...

 Hmm, we don't have the infrastructure to return bus information, yet.
 info qtree hardcodes printing of name and type.  Gerd, what do you
 think?

... as I implied here.

Regardless, I think we should first decide what data we want to transmit
over QMP, and how to structure it, then figure out if and how to change
its human readable presentation.

[...]




Re: [Qemu-devel] [PATCH][STABLE] Fix corner case in chardev udp: parameter

2010-01-18 Thread Jan Kiszka
Gerd Hoffmann wrote:
 On 01/15/10 21:07, Jan Kiszka wrote:
 The missing '@' broke 'udp::port@:port' parsing.
 
   if (sscanf(p, %64[^:]:%32[^@,]%n, host, port,pos)  2) {
   host[0] = 0;
 -if (sscanf(p, :%32[^,]%n, port,pos)  1) {
 +if (sscanf(p, :%32[^@,]%n, port,pos)  1) {
 
 Indeed.
 
   fprintf(stderr, udp #1\n);
 
 While you are at it, can you also zap this debug leftover?

Looks like there are three of them in qemu_chr_parse_compat - kill them all?

Jan



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to

2010-01-18 Thread Markus Armbruster
Michael S. Tsirkin m...@redhat.com writes:

 On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
 Instead of using the field 'readonly' of the BlockDriverState struct for 
 passing the request,
 pass the request in the flags parameter to the function.
 
 Signed-off-by: Naphtali Sprei nsp...@redhat.com

 Many changes seem to be about passing 0 to bdrv_open. This is not what
 the changelog says the patch does. Better split unrelated changes to a
 separate patch.

 One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
 this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
 0.

BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
BDRV_DONT_SNAPSHOT, either.

The old code can't quite devide whether BDRV_O_RDWR is a flag, or
whether to use bits BDRV_O_ACCESS for an access mode, with possible
values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
up, and recommended to go with flag rather than access mode:

In my opinion, any benefit in readability you might hope gain by
having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
need to keep knowledge of its encoding out of its users.

http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html

[...]
 @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
  return 0;
  }
  action = SNAPSHOT_LIST;
 +bdrv_oflags = ~BDRV_O_RDWR; /* no need for RW */

 bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
 for comment then?

BDRV_O_RDWR is a flag, and this is the clean way to clear it.

bdrv_oflags = BDRV_O_RDONLY assumes that everything but the access
mode in bdrv_oflags is clear.  Tolerable, because the correctness
argument is fairly local, but the clean way to do it would be

bdrv_oflags = (bdrv_oflags  ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;

That's what I meant by tortuous bit twiddling.

[...]




Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

2010-01-18 Thread Gerd Hoffmann

On 01/18/10 11:15, Markus Armbruster wrote:

Nathan Baumnat...@parenthephobia.org.uk  writes:


+static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
+{
+USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
+USBBus *bus = usb_bus_from_device(dev);
+return qobject_from_jsonf({'busnr': %d, 'addr':%d, 'speed': %s, 'desc': %s, 
'attached': %i},
+  bus-busnr,


As for PCI, 'busnr' belongs to the bus, not the device.


You want be able to figure which bus the device is attached to.

I think you actually can because the command returns the device tree 
converted into a qobject tree, correct?


Note: busnr is *not* fixed, it can be changed by the guest (maybe not 
for the primary, but surely for any secondary by writing to a pci bridge 
register).



Hmm. In cases like this, is it appropriate to modify the output of the
existing info qtree when it is modified to use the QObject data?



Would it be sensible to go the (probably small amount of) effort to
change the print functions to print exactly they do now, and put changes
to their output in different patches so they can easily be dropped if
necessary?


We might want to change info qtree output anyway if we show bus
information separately there...


Indeed.


Hmm, we don't have the infrastructure to return bus information, yet.
info qtree hardcodes printing of name and type.  Gerd, what do you
think?


Adding a -print_bus() function (or whatever the qmp aequivalent will 
be) to BusInfo is fine with me.



Regardless, I think we should first decide what data we want to transmit
over QMP, and how to structure it, then figure out if and how to change
its human readable presentation.


Sounds like a plan ;)

cheers,
  Gerd




Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

2010-01-18 Thread Gerd Hoffmann

However, because there are both device properties and bus properties
(really: device properties common to all devices on this bus), their
names can clash.  Device properties take precedence (see
qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
properties, not sure that's appropriate.  Gerd?


IMHO they must not clash.  This isn't enforced in any way though.

cheers,
  Gerd





Re: [Qemu-devel] [PATCH][STABLE] Fix corner case in chardev udp: parameter

2010-01-18 Thread Gerd Hoffmann

On 01/18/10 11:21, Jan Kiszka wrote:

Gerd Hoffmann wrote:

On 01/15/10 21:07, Jan Kiszka wrote:

The missing '@' broke 'udp::port@:port' parsing.



   if (sscanf(p, %64[^:]:%32[^@,]%n, host, port,pos)   2) {
   host[0] = 0;
-if (sscanf(p, :%32[^,]%n, port,pos)   1) {
+if (sscanf(p, :%32[^@,]%n, port,pos)   1) {


Indeed.


   fprintf(stderr, udp #1\n);


While you are at it, can you also zap this debug leftover?


Looks like there are three of them in qemu_chr_parse_compat - kill them all?


Yes.

thanks,
  Gerd





Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
 Michael S. Tsirkin m...@redhat.com writes:
 
  On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
  Instead of using the field 'readonly' of the BlockDriverState struct for 
  passing the request,
  pass the request in the flags parameter to the function.
  
  Signed-off-by: Naphtali Sprei nsp...@redhat.com
 
  Many changes seem to be about passing 0 to bdrv_open. This is not what
  the changelog says the patch does. Better split unrelated changes to a
  separate patch.
 
  One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
  this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
  0.
 
 BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
 BDRV_DONT_SNAPSHOT, either.

Well, this just mirros the file access macros: we have RDONLY, WRONLY
and RDRW. I assume this similarity is just historical?

 The old code can't quite devide whether BDRV_O_RDWR is a flag, or
 whether to use bits BDRV_O_ACCESS for an access mode, with possible
 values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
 up, and recommended to go with flag rather than access mode:
 
 In my opinion, any benefit in readability you might hope gain by
 having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
 need to keep knowledge of its encoding out of its users.
 
 http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html
 
 [...]
  @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
   return 0;
   }
   action = SNAPSHOT_LIST;
  +bdrv_oflags = ~BDRV_O_RDWR; /* no need for RW */
 
  bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
  for comment then?
 
 BDRV_O_RDWR is a flag, and this is the clean way to clear it.
 
 bdrv_oflags = BDRV_O_RDONLY assumes that everything but the access
 mode in bdrv_oflags is clear.  Tolerable, because the correctness
 argument is fairly local, but the clean way to do it would be
 
 bdrv_oflags = (bdrv_oflags  ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;
 
 That's what I meant by tortuous bit twiddling.
 
 [...]

Thinking about it, /* no need for RW */ comment can just go.  But other
places in code just do flags = 0 maybe they should all do =
~BDRV_O_RDWR?  I don't really have an opinion here but I do think this
patch needs a better commit log (all it says pass the request in the
flags parameter to the function) and be split up:
patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS
patch 2 - pass the request in the flags parameter to the function
patch 3 - any other fixups

As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as
well, and it's hard to see why.

-- 
MST




[Qemu-devel] [PATCHv2 0/3] rwhandler: introduce and switch pci_host to it

2010-01-18 Thread Michael S. Tsirkin
Alexander, so I assume the following patchset should be enough for you
to implement u3 support, simply by creating your own rwhandler, and
using pci_data_read/write directly there. I have pushed it to a
temporary branch in my tree:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git rwhandler

Paul, any comments on this approach?

I'll push this to my pci tree if this turns out to be helpful.
Hope this helps, and sorry about the churn.

Michael S. Tsirkin (3):
  bwap: add qemu_bswap helper
  rwhandler: simplified way to register for mem/io
  pci_host: rewrite using rwhandler

 Makefile.target|1 +
 bswap.h|6 ++
 hw/pci_host.c  |  172 +++
 hw/pci_host.h  |4 +
 hw/pci_host_template.h |  109 --
 rwhandler.c|   91 +
 rwhandler.h|   27 
 7 files changed, 199 insertions(+), 211 deletions(-)
 delete mode 100644 hw/pci_host_template.h
 create mode 100644 rwhandler.c
 create mode 100644 rwhandler.h




[Qemu-devel] [PATCHv2 1/3] bwap: add qemu_bswap helper

2010-01-18 Thread Michael S. Tsirkin
add helper that can swap values of 4, 2, 1 bytes

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 bswap.h |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/bswap.h b/bswap.h
index 4558704..aace9b7 100644
--- a/bswap.h
+++ b/bswap.h
@@ -214,4 +214,10 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+/* len must be one of 1, 2, 4 */
+static inline uint32_t qemu_bswap_len(uint32_t value, int len)
+{
+return bswap32(value)  (32 - 8 * len);
+}
+
 #endif /* BSWAP_H */
-- 
1.6.6.144.g5c3af





[Qemu-devel] [PATCHv2 2/3] rwhandler: simplified way to register for mem/io

2010-01-18 Thread Michael S. Tsirkin
Some users prefer a single callback with length passed as parameter to
using b/w/l callbacks. It would maybe be cleaner to just pass length to
existing callbacks but that's a lot of churn.  So for now add a wrapper.
For convenience use uint64_t for address so a single callback can be
used for ioport/memory.

I did have to resort to preprocessor to reduce code duplication.  It is
however slightly more straightforward, and better contained than what we
had with pci_host_template.h. Again, it would go away if we just passed
len to existing callbacks.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 Makefile.target |1 +
 rwhandler.c |   91 +++
 rwhandler.h |   27 
 3 files changed, 119 insertions(+), 0 deletions(-)
 create mode 100644 rwhandler.c
 create mode 100644 rwhandler.h

diff --git a/Makefile.target b/Makefile.target
index e661478..f9f4a2e 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -46,6 +46,7 @@ all: $(PROGS)
 # cpu emulator library
 libobj-y = exec.o translate-all.o cpu-exec.o translate.o
 libobj-y += tcg/tcg.o
+libobj-y += rwhandler.o
 libobj-$(CONFIG_SOFTFLOAT) += fpu/softfloat.o
 libobj-$(CONFIG_NOSOFTFLOAT) += fpu/softfloat-native.o
 libobj-y += op_helper.o helper.o
diff --git a/rwhandler.c b/rwhandler.c
new file mode 100644
index 000..5e258c1
--- /dev/null
+++ b/rwhandler.c
@@ -0,0 +1,91 @@
+#include rwhandler.h
+#include ioport.h
+#include cpu-all.h
+
+#if !defined(CONFIG_USER_ONLY)
+
+#define RWHANDLER_WRITE(name, len, type) \
+static void name(void *opaque, type addr, uint32_t value) \
+{\
+struct ReadWriteHandler *handler = opaque;\
+handler-write(handler, addr, value, len);\
+}
+
+#define RWHANDLER_READ(name, len, type) \
+static uint32_t name(void *opaque, type addr) \
+{ \
+struct ReadWriteHandler *handler = opaque; \
+return handler-read(handler, addr, len); \
+}
+
+RWHANDLER_WRITE(cpu_io_memory_simple_writeb, 1, target_phys_addr_t);
+RWHANDLER_READ(cpu_io_memory_simple_readb, 1, target_phys_addr_t);
+RWHANDLER_WRITE(cpu_io_memory_simple_writew, 2, target_phys_addr_t);
+RWHANDLER_READ(cpu_io_memory_simple_readw, 2, target_phys_addr_t);
+RWHANDLER_WRITE(cpu_io_memory_simple_writel, 4, target_phys_addr_t);
+RWHANDLER_READ(cpu_io_memory_simple_readl, 4, target_phys_addr_t);
+
+static CPUWriteMemoryFunc * const cpu_io_memory_simple_write[] = {
+cpu_io_memory_simple_writeb,
+cpu_io_memory_simple_writew,
+cpu_io_memory_simple_writel,
+};
+
+static CPUReadMemoryFunc * const cpu_io_memory_simple_read[] = {
+cpu_io_memory_simple_readb,
+cpu_io_memory_simple_readw,
+cpu_io_memory_simple_readl,
+};
+
+int cpu_register_io_memory_simple(struct ReadWriteHandler *handler)
+{
+if (!handler-read || !handler-write) {
+return -1;
+}
+return cpu_register_io_memory(cpu_io_memory_simple_read,
+  cpu_io_memory_simple_write,
+  handler);
+}
+
+RWHANDLER_WRITE(ioport_simple_writeb, 1, uint32_t);
+RWHANDLER_READ(ioport_simple_readb, 1, uint32_t);
+RWHANDLER_WRITE(ioport_simple_writew, 2, uint32_t);
+RWHANDLER_READ(ioport_simple_readw, 2, uint32_t);
+RWHANDLER_WRITE(ioport_simple_writel, 4, uint32_t);
+RWHANDLER_READ(ioport_simple_readl, 4, uint32_t);
+
+int register_ioport_simple(ReadWriteHandler* handler,
+   pio_addr_t start, int length, int size)
+{
+IOPortWriteFunc *write;
+IOPortReadFunc *read;
+int r;
+switch (size) {
+case 1:
+write = ioport_simple_writeb;
+read = ioport_simple_readb;
+   break;
+case 2:
+write = ioport_simple_writew;
+read = ioport_simple_readw;
+   break;
+default:
+write = ioport_simple_writel;
+read = ioport_simple_readl;
+}
+if (handler-write) {
+r = register_ioport_write(start, length, size, write, handler);
+if (r  0) {
+return r;
+}
+}
+if (handler-read) {
+r = register_ioport_read(start, length, size, read, handler);
+if (r  0) {
+return r;
+}
+}
+return 0;
+}
+
+#endif
diff --git a/rwhandler.h b/rwhandler.h
new file mode 100644
index 000..aa44b28
--- /dev/null
+++ b/rwhandler.h
@@ -0,0 +1,27 @@
+#ifndef READ_WRITE_HANDLER_H
+#define READ_WRITE_HANDLER_H
+
+#include qemu-common.h
+#include ioport.h
+
+typedef struct ReadWriteHandler ReadWriteHandler;
+
+/* len is guaranteed to be one of 1, 2 or 4, addr is guaranteed to fit in an
+ * appropriate type (io/memory/etc). They do not need to be range checked. */
+typedef void WriteHandlerFunc(ReadWriteHandler *, uint64_t addr,
+ uint32_t value, int len);
+typedef uint32_t ReadHandlerFunc(ReadWriteHandler *, uint64_t addr, int len);
+
+struct ReadWriteHandler {
+WriteHandlerFunc *write;
+ReadHandlerFunc *read;
+};
+
+/* Helpers for when we want to use a single routine with 

[Qemu-devel] [PATCHv2 3/3] pci_host: rewrite using rwhandler

2010-01-18 Thread Michael S. Tsirkin
Save a ton of code by switching pcihost to use rwhandler.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 hw/pci_host.c  |  172 +++
 hw/pci_host.h  |4 +
 hw/pci_host_template.h |  109 --
 3 files changed, 74 insertions(+), 211 deletions(-)
 delete mode 100644 hw/pci_host_template.h

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 6289ead..e96f617 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -79,152 +79,120 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 return val;
 }
 
-static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
-   uint32_t val)
+static void pci_host_config_write(ReadWriteHandler *handler,
+ uint64_t addr, uint32_t val, int len)
 {
-PCIHostState *s = opaque;
+PCIHostState *s = container_of(handler, PCIHostState, conf_handler);
 
+PCI_DPRINTF(%s addr % PRIx64  %d val %PRIx32\n,
+__func__, addr, len, val);
 #ifdef TARGET_WORDS_BIGENDIAN
-val = bswap32(val);
+val = qemu_bswap_len(val, len);
 #endif
-PCI_DPRINTF(%s addr  TARGET_FMT_plx  val %PRIx32\n,
-__func__, addr, val);
 s-config_reg = val;
 }
 
-static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
+static uint32_t pci_host_config_read(ReadWriteHandler *handler,
+uint64_t addr, int len)
 {
-PCIHostState *s = opaque;
+PCIHostState *s = container_of(handler, PCIHostState, conf_handler);
 uint32_t val = s-config_reg;
-
 #ifdef TARGET_WORDS_BIGENDIAN
-val = bswap32(val);
+val = qemu_bswap_len(val, len);
 #endif
-PCI_DPRINTF(%s addr  TARGET_FMT_plx  val %PRIx32\n,
-__func__, addr, val);
+PCI_DPRINTF(%s addr % PRIx64  len %d val %PRIx32\n,
+__func__, addr, len, val);
 return val;
 }
 
-static CPUWriteMemoryFunc * const pci_host_config_write[] = {
-pci_host_config_writel,
-pci_host_config_writel,
-pci_host_config_writel,
-};
-
-static CPUReadMemoryFunc * const pci_host_config_read[] = {
-pci_host_config_readl,
-pci_host_config_readl,
-pci_host_config_readl,
-};
-
-int pci_host_conf_register_mmio(PCIHostState *s)
-{
-return cpu_register_io_memory(pci_host_config_read,
-  pci_host_config_write, s);
-}
-
-static void pci_host_config_writel_noswap(void *opaque,
-  target_phys_addr_t addr,
-  uint32_t val)
+static void pci_host_config_write_noswap(ReadWriteHandler *handler,
+uint64_t addr, uint32_t val, int len)
 {
-PCIHostState *s = opaque;
+PCIHostState *s = container_of(handler, PCIHostState, conf_noswap_handler);
 
-PCI_DPRINTF(%s addr  TARGET_FMT_plx  val %PRIx32\n,
-__func__, addr, val);
+PCI_DPRINTF(%s addr % PRIx64  %d val %PRIx32\n,
+__func__, addr, len, val);
 s-config_reg = val;
 }
 
-static uint32_t pci_host_config_readl_noswap(void *opaque,
- target_phys_addr_t addr)
+static uint32_t pci_host_config_read_noswap(ReadWriteHandler *handler,
+uint64_t addr, int len)
 {
-PCIHostState *s = opaque;
+PCIHostState *s = container_of(handler, PCIHostState, conf_noswap_handler);
 uint32_t val = s-config_reg;
 
-PCI_DPRINTF(%s addr  TARGET_FMT_plx  val %PRIx32\n,
-__func__, addr, val);
+PCI_DPRINTF(%s addr % PRIx64  len %d val %PRIx32\n,
+__func__, addr, len, val);
 return val;
 }
 
-static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
-pci_host_config_writel_noswap,
-pci_host_config_writel_noswap,
-pci_host_config_writel_noswap,
-};
-
-static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
-pci_host_config_readl_noswap,
-pci_host_config_readl_noswap,
-pci_host_config_readl_noswap,
-};
-
-int pci_host_conf_register_mmio_noswap(PCIHostState *s)
+static void pci_host_data_write(ReadWriteHandler *handler,
+   uint64_t addr, uint32_t val, int len)
 {
-return cpu_register_io_memory(pci_host_config_read_noswap,
-  pci_host_config_write_noswap, s);
+PCIHostState *s = container_of(handler, PCIHostState, data_handler);
+#ifdef TARGET_WORDS_BIGENDIAN
+val = qemu_bswap_len(val, len);
+#endif
+PCI_DPRINTF(write addr % PRIx64  len %d val %x\n,
+addr, len, val);
+if (s-config_reg  (1u  31))
+pci_data_write(s-bus, s-config_reg | (addr  3), val, len);
 }
 
-static void pci_host_config_writel_ioport(void *opaque,
-  uint32_t addr, uint32_t val)
+static uint32_t pci_host_data_read(ReadWriteHandler *handler,
+   

[Qemu-devel] [PATCH][STABLE] Drop debug printfs from qemu_chr_parse_compat

2010-01-18 Thread Jan Kiszka
Gerd Hoffmann wrote:
 On 01/18/10 11:21, Jan Kiszka wrote:
 Gerd Hoffmann wrote:
 On 01/15/10 21:07, Jan Kiszka wrote:
 The missing '@' broke 'udp::port@:port' parsing.

if (sscanf(p, %64[^:]:%32[^@,]%n, host, port,pos)   2) {
host[0] = 0;
 -if (sscanf(p, :%32[^,]%n, port,pos)   1) {
 +if (sscanf(p, :%32[^@,]%n, port,pos)   1) {

 Indeed.

fprintf(stderr, udp #1\n);

 While you are at it, can you also zap this debug leftover?

 Looks like there are three of them in qemu_chr_parse_compat - kill
 them all?
 
 Yes.
 
 thanks,
   Gerd
 

Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---

 qemu-char.c |3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index a8a92f5..ef7823f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2315,7 +2315,6 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
char *filename)
 if (sscanf(p, %64[^:]:%32[^@,]%n, host, port, pos)  2) {
 host[0] = 0;
 if (sscanf(p, :%32[^@,]%n, port, pos)  1) {
-fprintf(stderr, udp #1\n);
 goto fail;
 }
 }
@@ -2326,7 +2325,6 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
char *filename)
 if (sscanf(p, %64[^:]:%32[^,]%n, host, port, pos)  2) {
 host[0] = 0;
 if (sscanf(p, :%32[^,]%n, port, pos)  1) {
-fprintf(stderr, udp #2\n);
 goto fail;
 }
 }
@@ -2354,7 +2352,6 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
char *filename)
 }
 
 fail:
-fprintf(stderr, %s: fail on \%s\\n, __FUNCTION__, filename);
 qemu_opts_del(opts);
 return NULL;
 }



signature.asc
Description: OpenPGP digital signature


[Qemu-devel] Re: [PATCH][STABLE] Drop debug printfs from qemu_chr_parse_compat

2010-01-18 Thread Gerd Hoffmann

While you are at it, can you also zap this debug leftover?


Looks like there are three of them in qemu_chr_parse_compat - kill
them all?


Yes.

thanks,
   Gerd



Signed-off-by: Jan Kiszkajan.kis...@siemens.com


Acked-by: Gerd Hoffmann kra...@redhat.com


-fprintf(stderr, udp #1\n);
-fprintf(stderr, udp #2\n);
-fprintf(stderr, %s: fail on \%s\\n, __FUNCTION__, filename);


cheers,
  Gerd




[Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to exp

2010-01-18 Thread Naphtali Sprei
Michael S. Tsirkin wrote:
 On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
 Instead of using the field 'readonly' of the BlockDriverState struct for 
 passing the request,
 pass the request in the flags parameter to the function.

 Signed-off-by: Naphtali Sprei nsp...@redhat.com
 
 Many changes seem to be about passing 0 to bdrv_open. This is not what
 the changelog says the patch does. Better split unrelated changes to a
 separate patch.

Thanks for the review.

Unfortunately, some of my comments went to the subject and are not part of the 
changelog.
So here's the (intended) changelog. This will clear-up some of your comments.

Clean-up a little bit the RW related bits of BDRV_O_FLAGS.
BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS).
Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request 
READ-WRITE.
Instead of using the field 'readonly' of the BlockDriverState struct for 
passing the request,
pass the request in the flags parameter to the function.



 
 One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
 this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
 0.

See Markus reply (thanks Markus).

 
 ---
  block.c   |   31 +--
  block.h   |2 --
  block/raw-posix.c |2 +-
  block/raw-win32.c |4 ++--
  block/vmdk.c  |9 +
  hw/xen_disk.c |2 +-
  monitor.c |2 +-
  qemu-img.c|   10 ++
  qemu-io.c |   14 ++
  qemu-nbd.c|2 +-
  vl.c  |8 
  11 files changed, 44 insertions(+), 42 deletions(-)

 diff --git a/block.c b/block.c
 index 115e591..8def3c4 100644
 --- a/block.c
 +++ b/block.c
 @@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char 
 *filename)
  if (drv  strcmp(drv-format_name, vvfat) == 0)
  return drv;
  
 -ret = bdrv_file_open(bs, filename, BDRV_O_RDONLY);
 +ret = bdrv_file_open(bs, filename, 0);
  if (ret  0)
  return NULL;
  ret = bdrv_pread(bs, 0, buf, sizeof(buf));
 @@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char 
 *filename, int flags)
  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 BlockDriver *drv)
  {
 -int ret, open_flags, try_rw;
 +int ret, open_flags;
  char tmp_filename[PATH_MAX];
  char backing_filename[PATH_MAX];
  
 @@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  
  /* Note: for compatibility, we open disk image files as RDWR, and
 RDONLY as fallback */
 -try_rw = !bs-read_only || bs-is_temporary;
 -if (!(flags  BDRV_O_FILE))
 -open_flags = (try_rw ? BDRV_O_RDWR : 0) |
 -(flags  (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
 -else
 +bs-read_only = (flags  BDRV_O_RDWR) == 0;
 
 !(flags  BDRV_O_RDWR) is a standard idiom, and it's more readable IMO.
 
 +if (!(flags  BDRV_O_FILE)) {
 +open_flags = (flags  (BDRV_O_RDWR | 
 BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
 +if (bs-is_temporary) { /* snapshot should be writeable */
 +open_flags |= BDRV_O_RDWR;
 +}
 +} else {
  open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
 -if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv))
 +}
 +if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv)) {
  ret = -ENOTSUP;
 -else
 +} else {
  ret = drv-bdrv_open(bs, filename, open_flags);
 -if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 -ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 -bs-read_only = 1;
 +if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 +ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 +bs-read_only = 1;
 +}
  }
  if (ret  0) {
  qemu_free(bs-opaque);
 @@ -481,14 +485,13 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  /* if there is a backing file, use it */
  BlockDriver *back_drv = NULL;
  bs-backing_hd = bdrv_new();
 -/* pass on read_only property to the backing_hd */
 -bs-backing_hd-read_only = bs-read_only;
  path_combine(backing_filename, sizeof(backing_filename),
   filename, bs-backing_file);
  if (bs-backing_format[0] != '\0')
  back_drv = bdrv_find_format(bs-backing_format);
  ret = bdrv_open2(bs-backing_hd, backing_filename, open_flags,
   back_drv);
 +bs-backing_hd-read_only =  (open_flags  BDRV_O_RDWR) == 0;
 
 !(open_flags  BDRV_O_RDWR) is a standard idiom and it's more readable
 IMO.

Sorry, I prefer the more verbose style. The extra bytes on me. Seems more 
readable for me.

 Further, pls don't put two spaces after ==.

Sure, will correct.

 
  if (ret  0) {
  bdrv_close(bs);
  return 

[Qemu-devel] Re: [PATCH] Check for sdl-config before calling it

2010-01-18 Thread Paolo Bonzini

On 01/17/2010 05:06 PM, Loïc Minier wrote:

On Sun, Jan 17, 2010, Stefan Weil wrote:

On systems were sdl-config isn't installed, ./configure triggers this
warning:
./configure: 957: sdl-config: not found


which version did you test?
Git master has no sdl-config call at configure:957.

But I get warning messages, too, when pkg-config or
sdl-config are missing.


  Yes, the line is bogus for me as well; not sure why.  I'm using master.


Your patch fixes the warning for sdl-config and sets
sdl=no. If configure was called with --enable-sdl,
this solution is wrong: configure should abort with
an error message (feature_not_found).


  Ack; how about the attached one instead?


Acked-By: Paolo Bonzini pbonz...@redhat.com

Paolo





[Qemu-devel] Re: [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

2010-01-18 Thread Naphtali Sprei
Michael S. Tsirkin wrote:
 On Sun, Jan 17, 2010 at 04:48:15PM +0200, Naphtali Sprei wrote:
 Signed-off-by: Naphtali Sprei nsp...@redhat.com
 ---
  block.c |6 --
  1 files changed, 0 insertions(+), 6 deletions(-)

 diff --git a/block.c b/block.c
 index 8def3c4..f90e983 100644
 --- a/block.c
 +++ b/block.c
 @@ -444,8 +444,6 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  if (flags  (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))
  bs-enable_write_cache = 1;
  
 -/* Note: for compatibility, we open disk image files as RDWR, and
 -   RDONLY as fallback */
  bs-read_only = (flags  BDRV_O_RDWR) == 0;
  if (!(flags  BDRV_O_FILE)) {
  open_flags = (flags  (BDRV_O_RDWR | 
 BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
 @@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char 
 *filename, int flags,
  ret = -ENOTSUP;
  } else {
  ret = drv-bdrv_open(bs, filename, open_flags);
 -if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
 -ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
 -bs-read_only = 1;
 -}
 
 Maybe print an error message explaining the problem and
 suggesting the solution?

Printing done in (some of the) callers.
Should add suggestion. How is it done in QMP ?

 
  }
  if (ret  0) {
  qemu_free(bs-opaque);
 -- 
 1.6.3.3







[Qemu-devel] [PATCH 00/10] qcow2 error path fixes

2010-01-18 Thread Kevin Wolf
In qcow2 (and partly also in general block code), error handling doesn't quite
do what it should. In some places, errors are silently ignored. In other
places, the error code in not passed to the called but rather turned into
false, NULL or -EIO. Besides reporting the right error to the user, providing
the right error code is also important for werror=enospc (and possible some
more functions).

This patch series tries to fix the most obvious of them. Don't assume it's
complete and expect a second part some time in the future.

Kevin Wolf (10):
  qcow2: Fix error handling in qcow2_grow_l1_table
  qcow2: Fix error handling in qcow_save_vmstate
  qcow2: Return 0/-errno in get_cluster_table
  qcow2: Return 0/-errno in qcow2_alloc_cluster_offset
  block: Return original error codes in bdrv_pread/write
  qcow2: Fix error handling in grow_refcount_table
  qcow2: Improve error handling in update_refcount
  qcow2: Allow updating no refcounts
  qcow2: Don't ignore update_refcount return value
  qcow2: Don't ignore qcow2_alloc_clusters return value

 block.c|   34 ++-
 block/qcow2-cluster.c  |   84 
 block/qcow2-refcount.c |   82 ---
 block/qcow2-snapshot.c |   11 ++-
 block/qcow2.c  |   41 ---
 block/qcow2.h  |4 +-
 6 files changed, 171 insertions(+), 85 deletions(-)





[Qemu-devel] [PATCH 01/10] qcow2: Fix error handling in qcow2_grow_l1_table

2010-01-18 Thread Kevin Wolf
Return the appropriate error value instead of always using EIO. Don't free the
L1 table on errors, we still need it.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-cluster.c |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f88118c..adddf56 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -67,9 +67,10 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 /* set new table */
 cpu_to_be32w((uint32_t*)data, new_l1_size);
 cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
-if (bdrv_pwrite(s-hd, offsetof(QCowHeader, l1_size), data,
-sizeof(data)) != sizeof(data))
+ret = bdrv_pwrite(s-hd, offsetof(QCowHeader, l1_size), data,sizeof(data));
+if (ret != sizeof(data)) {
 goto fail;
+}
 qemu_free(s-l1_table);
 qcow2_free_clusters(bs, s-l1_table_offset, s-l1_size * sizeof(uint64_t));
 s-l1_table_offset = new_l1_table_offset;
@@ -77,8 +78,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 s-l1_size = new_l1_size;
 return 0;
  fail:
-qemu_free(s-l1_table);
-return -EIO;
+return ret  0 ? ret : -EIO;
 }
 
 void qcow2_l2_cache_reset(BlockDriverState *bs)
-- 
1.6.2.5





[Qemu-devel] [PATCH 02/10] qcow2: Fix error handling in qcow_save_vmstate

2010-01-18 Thread Kevin Wolf
Don't assume success but pass the bdrv_pwrite return value on.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6622eba..e06f4dd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1163,12 +1163,13 @@ static int qcow_save_vmstate(BlockDriverState *bs, 
const uint8_t *buf,
 {
 BDRVQcowState *s = bs-opaque;
 int growable = bs-growable;
+int ret;
 
 bs-growable = 1;
-bdrv_pwrite(bs, qcow_vm_state_offset(s) + pos, buf, size);
+ret = bdrv_pwrite(bs, qcow_vm_state_offset(s) + pos, buf, size);
 bs-growable = growable;
 
-return size;
+return ret;
 }
 
 static int qcow_load_vmstate(BlockDriverState *bs, uint8_t *buf,
-- 
1.6.2.5





[Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

2010-01-18 Thread Kevin Wolf
Returning 0/-errno allows it to distingush different errors classes. The
cluster offset of newly allocated clusters is now returned in the QCowL2Meta
struct.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-cluster.c |   28 ++--
 block/qcow2.c |   36 +++-
 block/qcow2.h |4 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 424bedd..3e877a4 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -609,12 +609,12 @@ static int write_l2_entries(BDRVQcowState *s, uint64_t 
*l2_table,
 return 0;
 }
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
-QCowL2Meta *m)
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
 {
 BDRVQcowState *s = bs-opaque;
 int i, j = 0, l2_index, ret;
 uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
+uint64_t cluster_offset = m-cluster_offset;
 
 if (m-nb_clusters == 0)
 return 0;
@@ -675,16 +675,22 @@ err:
 /*
  * alloc_cluster_offset
  *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
+ * For a given offset of the disk image, return cluster offset in qcow2 file.
  * If the offset is not found, allocate a new cluster.
  *
- * Return the cluster offset if successful,
- * Return 0, otherwise.
+ * If the cluster was already allocated, m-nb_clusters is set to 0,
+ * m-depends_on is set to NULL and the other fields in m are meaningless.
+ *
+ * If the cluster is newly allocated, m-nb_clusters is set to the number of
+ * contiguous clusters that have been allocated. This may be 0 if the request
+ * conflict with another write request in flight; in this case, m-depends_on
+ * is set and the remaining fields of m are meaningless.
  *
+ * If m-nb_clusters is non-zero, the other fields of m are valid and contain
+ * information about the first allocated cluster.
+ *
+ * Return 0 on success and -errno in error cases
  */
-
 uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 uint64_t offset,
 int n_start, int n_end,
@@ -698,7 +704,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 ret = get_cluster_table(bs, offset, l2_table, l2_offset, l2_index);
 if (ret  0) {
-return 0;
+return ret;
 }
 
 nb_clusters = size_to_clusters(s, n_end  9);
@@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 cluster_offset = ~QCOW_OFLAG_COPIED;
 m-nb_clusters = 0;
+m-depends_on = NULL;
 
 goto out;
 }
@@ -793,10 +800,11 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 out:
 m-nb_available = MIN(nb_clusters  (s-cluster_bits - 9), n_end);
+m-cluster_offset = cluster_offset;
 
 *num = m-nb_available - n_start;
 
-return cluster_offset;
+return 0;
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index e06f4dd..773d381 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -561,7 +561,7 @@ static void qcow_aio_write_cb(void *opaque, int ret)
 acb-hd_aiocb = NULL;
 
 if (ret = 0) {
-ret = qcow2_alloc_cluster_link_l2(bs, acb-cluster_offset, 
acb-l2meta);
+ret = qcow2_alloc_cluster_link_l2(bs, acb-l2meta);
 }
 
 run_dependent_requests(acb-l2meta);
@@ -585,21 +585,23 @@ static void qcow_aio_write_cb(void *opaque, int ret)
 n_end  QCOW_MAX_CRYPT_CLUSTERS * s-cluster_sectors)
 n_end = QCOW_MAX_CRYPT_CLUSTERS * s-cluster_sectors;
 
-acb-cluster_offset = qcow2_alloc_cluster_offset(bs, acb-sector_num  9,
-  index_in_cluster,
-  n_end, acb-n, acb-l2meta);
+ret = qcow2_alloc_cluster_offset(bs, acb-sector_num  9,
+index_in_cluster, n_end, acb-n, acb-l2meta);
+if (ret  0) {
+goto done;
+}
+
+acb-cluster_offset = acb-l2meta.cluster_offset;
 
 /* Need to wait for another request? If so, we are done for now. */
-if (!acb-cluster_offset  acb-l2meta.depends_on != NULL) {
+if (acb-l2meta.nb_clusters == 0  acb-l2meta.depends_on != NULL) {
 QLIST_INSERT_HEAD(acb-l2meta.depends_on-dependent_requests,
 acb, next_depend);
 return;
 }
 
-if (!acb-cluster_offset || (acb-cluster_offset  511) != 0) {
-ret = -EIO;
-goto done;
-}
+assert((acb-cluster_offset  511) == 0);
+
 if (s-crypt_method) {
 if (!acb-cluster_data) {
 acb-cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
@@ -782,27 +784,27 @@ static int get_bits_from_size(size_t size)
 static int preallocate(BlockDriverState *bs)
 {
 BDRVQcowState *s = bs-opaque;
-uint64_t cluster_offset = 0;
 uint64_t nb_sectors;
 uint64_t offset;
 int num;

[Qemu-devel] [PATCH 03/10] qcow2: Return 0/-errno in get_cluster_table

2010-01-18 Thread Kevin Wolf
Switching to 0/-errno allows it to distinguish different error cases.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-cluster.c |   30 ++
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index adddf56..424bedd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -479,8 +479,8 @@ out:
  * the l2 table offset in the qcow2 file and the cluster index
  * in the l2 table are given to the caller.
  *
+ * Returns 0 on success, -errno in failure case
  */
-
 static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
  uint64_t **new_l2_table,
  uint64_t *new_l2_offset,
@@ -496,8 +496,9 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t 
offset,
 l1_index = offset  (s-l2_bits + s-cluster_bits);
 if (l1_index = s-l1_size) {
 ret = qcow2_grow_l1_table(bs, l1_index + 1);
-if (ret  0)
-return 0;
+if (ret  0) {
+return ret;
+}
 }
 l2_offset = s-l1_table[l1_index];
 
@@ -507,14 +508,16 @@ static int get_cluster_table(BlockDriverState *bs, 
uint64_t offset,
 /* load the l2 table in memory */
 l2_offset = ~QCOW_OFLAG_COPIED;
 l2_table = l2_load(bs, l2_offset);
-if (l2_table == NULL)
-return 0;
+if (l2_table == NULL) {
+return -EIO;
+}
 } else {
 if (l2_offset)
 qcow2_free_clusters(bs, l2_offset, s-l2_size * sizeof(uint64_t));
 l2_table = l2_allocate(bs, l1_index);
-if (l2_table == NULL)
-return 0;
+if (l2_table == NULL) {
+return -EIO;
+}
 l2_offset = s-l1_table[l1_index]  ~QCOW_OFLAG_COPIED;
 }
 
@@ -526,7 +529,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t 
offset,
 *new_l2_offset = l2_offset;
 *new_l2_index = l2_index;
 
-return 1;
+return 0;
 }
 
 /*
@@ -552,8 +555,9 @@ uint64_t 
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
 int nb_csectors;
 
 ret = get_cluster_table(bs, offset, l2_table, l2_offset, l2_index);
-if (ret == 0)
+if (ret  0) {
 return 0;
+}
 
 cluster_offset = be64_to_cpu(l2_table[l2_index]);
 if (cluster_offset  QCOW_OFLAG_COPIED)
@@ -633,10 +637,11 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
uint64_t cluster_offset,
 goto err;
 }
 
-ret = -EIO;
 /* update L2 table */
-if (!get_cluster_table(bs, m-offset, l2_table, l2_offset, l2_index))
+ret = get_cluster_table(bs, m-offset, l2_table, l2_offset, l2_index);
+if (ret  0) {
 goto err;
+}
 
 for (i = 0; i  m-nb_clusters; i++) {
 /* if two concurrent writes happen to the same unallocated cluster
@@ -692,8 +697,9 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 QCowL2Meta *old_alloc;
 
 ret = get_cluster_table(bs, offset, l2_table, l2_offset, l2_index);
-if (ret == 0)
+if (ret  0) {
 return 0;
+}
 
 nb_clusters = size_to_clusters(s, n_end  9);
 
-- 
1.6.2.5





[Qemu-devel] [PATCH 06/10] qcow2: Fix error handling in grow_refcount_table

2010-01-18 Thread Kevin Wolf
Return the appropriate error code instead of -EIO.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-refcount.c |9 ++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3a2d44a..6f449c6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -168,9 +168,12 @@ static int grow_refcount_table(BlockDriverState *bs, int 
min_size)
 
 cpu_to_be64w((uint64_t*)data, table_offset);
 cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
-if (bdrv_pwrite(s-hd, offsetof(QCowHeader, refcount_table_offset),
-data, sizeof(data)) != sizeof(data))
+ret = bdrv_pwrite(s-hd, offsetof(QCowHeader, refcount_table_offset),
+data, sizeof(data));
+if (ret != sizeof(data)) {
 goto fail;
+}
+
 qemu_free(s-refcount_table);
 old_table_offset = s-refcount_table_offset;
 old_table_size = s-refcount_table_size;
@@ -183,7 +186,7 @@ static int grow_refcount_table(BlockDriverState *bs, int 
min_size)
 return 0;
  fail:
 qemu_free(new_table);
-return -EIO;
+return ret  0 ? ret : -EIO;
 }
 
 
-- 
1.6.2.5





[Qemu-devel] [PATCH 05/10] block: Return original error codes in bdrv_pread/write

2010-01-18 Thread Kevin Wolf
Don't assume -EIO but return the real error.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block.c |   34 ++
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 115e591..a4c4953 100644
--- a/block.c
+++ b/block.c
@@ -717,6 +717,7 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
 uint8_t tmp_buf[BDRV_SECTOR_SIZE];
 int len, nb_sectors, count;
 int64_t sector_num;
+int ret;
 
 count = count1;
 /* first read to align to sector start */
@@ -725,8 +726,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
 len = count;
 sector_num = offset  BDRV_SECTOR_BITS;
 if (len  0) {
-if (bdrv_read(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 memcpy(buf, tmp_buf + (offset  (BDRV_SECTOR_SIZE - 1)), len);
 count -= len;
 if (count == 0)
@@ -738,8 +739,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
 /* read the sectors in place */
 nb_sectors = count  BDRV_SECTOR_BITS;
 if (nb_sectors  0) {
-if (bdrv_read(bs, sector_num, buf, nb_sectors)  0)
-return -EIO;
+if ((ret = bdrv_read(bs, sector_num, buf, nb_sectors))  0)
+return ret;
 sector_num += nb_sectors;
 len = nb_sectors  BDRV_SECTOR_BITS;
 buf += len;
@@ -748,8 +749,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
 
 /* add data from the last sector */
 if (count  0) {
-if (bdrv_read(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 memcpy(buf, tmp_buf, count);
 }
 return count1;
@@ -761,6 +762,7 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 uint8_t tmp_buf[BDRV_SECTOR_SIZE];
 int len, nb_sectors, count;
 int64_t sector_num;
+int ret;
 
 count = count1;
 /* first write to align to sector start */
@@ -769,11 +771,11 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 len = count;
 sector_num = offset  BDRV_SECTOR_BITS;
 if (len  0) {
-if (bdrv_read(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 memcpy(tmp_buf + (offset  (BDRV_SECTOR_SIZE - 1)), buf, len);
-if (bdrv_write(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_write(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 count -= len;
 if (count == 0)
 return count1;
@@ -784,8 +786,8 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 /* write the sectors in place */
 nb_sectors = count  BDRV_SECTOR_BITS;
 if (nb_sectors  0) {
-if (bdrv_write(bs, sector_num, buf, nb_sectors)  0)
-return -EIO;
+if ((ret = bdrv_write(bs, sector_num, buf, nb_sectors))  0)
+return ret;
 sector_num += nb_sectors;
 len = nb_sectors  BDRV_SECTOR_BITS;
 buf += len;
@@ -794,11 +796,11 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 
 /* add data from the last sector */
 if (count  0) {
-if (bdrv_read(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 memcpy(tmp_buf, buf, count);
-if (bdrv_write(bs, sector_num, tmp_buf, 1)  0)
-return -EIO;
+if ((ret = bdrv_write(bs, sector_num, tmp_buf, 1))  0)
+return ret;
 }
 return count1;
 }
-- 
1.6.2.5





[Qemu-devel] [PATCH 07/10] qcow2: Improve error handling in update_refcount

2010-01-18 Thread Kevin Wolf
If update_refcount fails, try to undo any changes made so far to avoid
inconsistencies in the image file.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-refcount.c |   32 +---
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6f449c6..a84620f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -278,6 +278,7 @@ static int update_refcount(BlockDriverState *bs,
 int64_t refcount_block_offset = 0;
 int64_t table_index = -1, old_table_index;
 int first_index = -1, last_index = -1;
+int ret;
 
 #ifdef DEBUG_ALLOC2
 printf(update_refcount: offset=% PRId64  size=% PRId64  addend=%d\n,
@@ -292,6 +293,7 @@ static int update_refcount(BlockDriverState *bs,
 {
 int block_index, refcount;
 int64_t cluster_index = cluster_offset  s-cluster_bits;
+int64_t new_block;
 
 /* Only write refcount block to disk when we are done with it */
 old_table_index = table_index;
@@ -309,10 +311,12 @@ static int update_refcount(BlockDriverState *bs,
 }
 
 /* Load the refcount block and allocate it if needed */
-refcount_block_offset = alloc_refcount_block(bs, cluster_index);
-if (refcount_block_offset  0) {
-return refcount_block_offset;
+new_block = alloc_refcount_block(bs, cluster_index);
+if (new_block  0) {
+ret = new_block;
+goto fail;
 }
+refcount_block_offset = new_block;
 
 /* we can update the count and save it */
 block_index = cluster_index 
@@ -326,24 +330,38 @@ static int update_refcount(BlockDriverState *bs,
 
 refcount = be16_to_cpu(s-refcount_block_cache[block_index]);
 refcount += addend;
-if (refcount  0 || refcount  0x)
-return -EINVAL;
+if (refcount  0 || refcount  0x) {
+ret = -EINVAL;
+goto fail;
+}
 if (refcount == 0  cluster_index  s-free_cluster_index) {
 s-free_cluster_index = cluster_index;
 }
 s-refcount_block_cache[block_index] = cpu_to_be16(refcount);
 }
 
+ret = 0;
+fail:
+
 /* Write last changed block to disk */
 if (refcount_block_offset != 0) {
 if (write_refcount_block_entries(s, refcount_block_offset,
 first_index, last_index)  0)
 {
-return -EIO;
+return ret  0 ? ret : -EIO;
 }
 }
 
-return 0;
+/*
+ * Try do undo any updates if an error is returned (This may succeed in
+ * some cases like ENOSPC for allocating a new refcount block)
+ */
+if (ret  0) {
+int dummy;
+dummy = update_refcount(bs, offset, cluster_offset - offset, -addend);
+}
+
+return ret;
 }
 
 /* addend must be 1 or -1 */
-- 
1.6.2.5





[Qemu-devel] [PATCH 08/10] qcow2: Allow updating no refcounts

2010-01-18 Thread Kevin Wolf
There's absolutely no problem with updating the refcounts of 0 clusters.
At least snapshot code is doing this and would fail once the result of
update_refcount isn't ignored any more.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-refcount.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index a84620f..3dfadf1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -284,8 +284,10 @@ static int update_refcount(BlockDriverState *bs,
 printf(update_refcount: offset=% PRId64  size=% PRId64  addend=%d\n,
offset, length, addend);
 #endif
-if (length = 0)
+if (length  0) {
 return -EINVAL;
+}
+
 start = offset  ~(s-cluster_size - 1);
 last = (offset + length - 1)  ~(s-cluster_size - 1);
 for(cluster_offset = start; cluster_offset = last;
-- 
1.6.2.5





[Qemu-devel] [PATCH 10/10] qcow2: Don't ignore qcow2_alloc_clusters return value

2010-01-18 Thread Kevin Wolf
Now that qcow2_alloc_clusters can return error codes, we must handle them in
the callers of qcow2_alloc_clusters.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-cluster.c  |   18 --
 block/qcow2-refcount.c |6 ++
 block/qcow2-snapshot.c |   11 ++-
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3e877a4..5a63918 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -33,7 +33,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 BDRVQcowState *s = bs-opaque;
 int new_l1_size, new_l1_size2, ret, i;
 uint64_t *new_l1_table;
-uint64_t new_l1_table_offset;
+int64_t new_l1_table_offset;
 uint8_t data[12];
 
 new_l1_size = s-l1_size;
@@ -55,6 +55,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 
 /* write new table (align to cluster) */
 new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
+if (new_l1_table_offset  0) {
+return new_l1_table_offset;
+}
 
 for(i = 0; i  s-l1_size; i++)
 new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
@@ -220,6 +223,9 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int 
l1_index)
 /* allocate a new l2 entry */
 
 l2_offset = qcow2_alloc_clusters(bs, s-l2_size * sizeof(uint64_t));
+if (l2_offset  0) {
+return NULL;
+}
 
 /* update the L1 entry */
 
@@ -567,6 +573,10 @@ uint64_t 
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
 qcow2_free_any_clusters(bs, cluster_offset, 1);
 
 cluster_offset = qcow2_alloc_bytes(bs, compressed_size);
+if (cluster_offset  0) {
+return 0;
+}
+
 nb_csectors = ((cluster_offset + compressed_size - 1)  9) -
   (cluster_offset  9);
 
@@ -698,7 +708,8 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 {
 BDRVQcowState *s = bs-opaque;
 int l2_index, ret;
-uint64_t l2_offset, *l2_table, cluster_offset;
+uint64_t l2_offset, *l2_table;
+int64_t cluster_offset;
 unsigned int nb_clusters, i = 0;
 QCowL2Meta *old_alloc;
 
@@ -792,6 +803,9 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 /* allocate a new cluster */
 
 cluster_offset = qcow2_alloc_clusters(bs, nb_clusters * s-cluster_size);
+if (cluster_offset  0) {
+return cluster_offset;
+}
 
 /* save info needed for meta data update */
 m-offset = offset;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b9a825b..890de7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -433,6 +433,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 assert(size  0  size = s-cluster_size);
 if (s-free_byte_offset == 0) {
 s-free_byte_offset = qcow2_alloc_clusters(bs, s-cluster_size);
+if (s-free_byte_offset  0) {
+return s-free_byte_offset;
+}
 }
  redo:
 free_in_cluster = s-cluster_size -
@@ -448,6 +451,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 update_cluster_refcount(bs, offset  s-cluster_bits, 1);
 } else {
 offset = qcow2_alloc_clusters(bs, s-cluster_size);
+if (offset  0) {
+return offset;
+}
 cluster_offset = s-free_byte_offset  ~(s-cluster_size - 1);
 if ((cluster_offset + s-cluster_size) == offset) {
 /* we are lucky: contiguous data */
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d63c7e1..8ddaea2 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -139,6 +139,9 @@ static int qcow_write_snapshots(BlockDriverState *bs)
 
 snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
 offset = snapshots_offset;
+if (offset  0) {
+return offset;
+}
 
 for(i = 0; i  s-nb_snapshots; i++) {
 sn = s-snapshots + i;
@@ -235,6 +238,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
 QCowSnapshot *snapshots1, sn1, *sn = sn1;
 int i, ret;
 uint64_t *l1_table = NULL;
+int64_t l1_table_offset;
 
 memset(sn, 0, sizeof(*sn));
 
@@ -263,7 +267,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
 goto fail;
 
 /* create the L1 table of the snapshot */
-sn-l1_table_offset = qcow2_alloc_clusters(bs, s-l1_size * 
sizeof(uint64_t));
+l1_table_offset = qcow2_alloc_clusters(bs, s-l1_size * sizeof(uint64_t));
+if (l1_table_offset  0) {
+goto fail;
+}
+
+sn-l1_table_offset = l1_table_offset;
 sn-l1_size = s-l1_size;
 
 if (s-l1_size != 0) {
-- 
1.6.2.5





[Qemu-devel] [PATCH 09/10] qcow2: Don't ignore update_refcount return value

2010-01-18 Thread Kevin Wolf
update_refcount can return errors that need to be handled by the callers.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-refcount.c |   31 +++
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3dfadf1..b9a825b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -269,9 +269,8 @@ static int write_refcount_block_entries(BDRVQcowState *s,
 }
 
 /* XXX: cache several refcount block clusters ? */
-static int update_refcount(BlockDriverState *bs,
-int64_t offset, int64_t length,
-int addend)
+static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
+int64_t offset, int64_t length, int addend)
 {
 BDRVQcowState *s = bs-opaque;
 int64_t start, last, cluster_offset;
@@ -413,9 +412,13 @@ retry:
 int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t size)
 {
 int64_t offset;
+int ret;
 
 offset = alloc_clusters_noref(bs, size);
-update_refcount(bs, offset, size, 1);
+ret = update_refcount(bs, offset, size, 1);
+if (ret  0) {
+return ret;
+}
 return offset;
 }
 
@@ -462,7 +465,13 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 void qcow2_free_clusters(BlockDriverState *bs,
   int64_t offset, int64_t size)
 {
-update_refcount(bs, offset, size, -1);
+int ret;
+
+ret = update_refcount(bs, offset, size, -1);
+if (ret  0) {
+fprintf(stderr, qcow2_free_clusters failed: %s\n, strerror(-ret));
+abort();
+}
 }
 
 /*
@@ -571,9 +580,15 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
 if (offset  QCOW_OFLAG_COMPRESSED) {
 nb_csectors = ((offset  s-csize_shift) 
s-csize_mask) + 1;
-if (addend != 0)
-update_refcount(bs, (offset  
s-cluster_offset_mask)  ~511,
-nb_csectors * 512, addend);
+if (addend != 0) {
+int ret;
+ret = update_refcount(bs,
+(offset  s-cluster_offset_mask)  ~511,
+nb_csectors * 512, addend);
+if (ret  0) {
+goto fail;
+}
+}
 /* compressed clusters are never modified */
 refcount = 2;
 } else {
-- 
1.6.2.5





Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

2010-01-18 Thread Markus Armbruster
Gerd Hoffmann kra...@redhat.com writes:

 However, because there are both device properties and bus properties
 (really: device properties common to all devices on this bus), their
 names can clash.  Device properties take precedence (see
 qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
 properties, not sure that's appropriate.  Gerd?

 IMHO they must not clash.  This isn't enforced in any way though.

If they must not clash, then it makes no sense to invent a fancy prefix
to cope with clashes, I think.

Alternatively, we could declare devices overriding properties inherited
from the bus a feature.  Does it make any sense to show the shadowed
properties then?




Re: [Qemu-devel] [RFC,PATCH 08/11] qdev: Add usb_bus_dev_info

2010-01-18 Thread Markus Armbruster
Gerd Hoffmann kra...@redhat.com writes:

 On 01/18/10 11:15, Markus Armbruster wrote:
 Nathan Baumnat...@parenthephobia.org.uk  writes:

 +static QObject *usb_bus_dev_info(Monitor *mon, DeviceState *qdev)
 +{
 +USBDevice *dev = DO_UPCAST(USBDevice, qdev, qdev);
 +USBBus *bus = usb_bus_from_device(dev);
 +return qobject_from_jsonf({'busnr': %d, 'addr':%d, 'speed': %s, 
 'desc': %s, 'attached': %i},
 +  bus-busnr,

 As for PCI, 'busnr' belongs to the bus, not the device.

 You want be able to figure which bus the device is attached to.

 I think you actually can because the command returns the device tree
 converted into a qobject tree, correct?

Correct.

[...]




Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to

2010-01-18 Thread Markus Armbruster
Michael S. Tsirkin m...@redhat.com writes:

 On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
 Michael S. Tsirkin m...@redhat.com writes:
 
  On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
  Instead of using the field 'readonly' of the BlockDriverState struct for 
  passing the request,
  pass the request in the flags parameter to the function.
  
  Signed-off-by: Naphtali Sprei nsp...@redhat.com
 
  Many changes seem to be about passing 0 to bdrv_open. This is not what
  the changelog says the patch does. Better split unrelated changes to a
  separate patch.
 
  One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
  this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
  0.
 
 BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
 BDRV_DONT_SNAPSHOT, either.

 Well, this just mirros the file access macros: we have RDONLY, WRONLY
 and RDRW. I assume this similarity is just historical?

Plausible.

 The old code can't quite devide whether BDRV_O_RDWR is a flag, or
 whether to use bits BDRV_O_ACCESS for an access mode, with possible
 values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
 up, and recommended to go with flag rather than access mode:
 
 In my opinion, any benefit in readability you might hope gain by
 having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
 need to keep knowledge of its encoding out of its users.
 
 http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html
 
 [...]
  @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
   return 0;
   }
   action = SNAPSHOT_LIST;
  +bdrv_oflags = ~BDRV_O_RDWR; /* no need for RW */
 
  bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
  for comment then?
 
 BDRV_O_RDWR is a flag, and this is the clean way to clear it.
 
 bdrv_oflags = BDRV_O_RDONLY assumes that everything but the access
 mode in bdrv_oflags is clear.  Tolerable, because the correctness
 argument is fairly local, but the clean way to do it would be
 
 bdrv_oflags = (bdrv_oflags  ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;
 
 That's what I meant by tortuous bit twiddling.
 
 [...]

 Thinking about it, /* no need for RW */ comment can just go.

Agree.

   But other
 places in code just do flags = 0 maybe they should all do =
 ~BDRV_O_RDWR?  I don't really have an opinion here but I do think this
 patch needs a better commit log (all it says pass the request in the
 flags parameter to the function) and be split up:
 patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS
 patch 2 - pass the request in the flags parameter to the function
 patch 3 - any other fixups

 As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as
 well, and it's hard to see why.

Fair enough.




Re: [Qemu-devel] [RFC, PATCH 10/11] qdev: Add do_info_qbus and friends.

2010-01-18 Thread Gerd Hoffmann

On 01/18/10 13:34, Markus Armbruster wrote:

However, because there are both device properties and bus properties
(really: device properties common to all devices on this bus), their
names can clash.  Device properties take precedence (see
qdev_prop_find()).  Hmm, qdev_printf() prints even overridden bus
properties, not sure that's appropriate.  Gerd?


IMHO they must not clash.  This isn't enforced in any way though.


If they must not clash, then it makes no sense to invent a fancy prefix
to cope with clashes, I think.


I've added the bus- and dev- prefixes to make clear where the properties 
come from (BusInfo or DeviceInfo) for informational purposes, not to 
avoid clashes.  We could just drop that ...



Alternatively, we could declare devices overriding properties inherited
from the bus a feature.


No, this is just asking for trouble IMHO.

cheers,
  Gerd





Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Avi Kivity

On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:

This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly.  Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.


+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1  L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2  L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+   TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS  32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS  (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1  L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}
   


This looks pretty frightening.  What is it needed for?

I think we should stick with range operations, but maybe I misunderstood 
something here.


Otherwise, I like this patchset.

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





[Qemu-devel] Please remove from mailing-list

2010-01-18 Thread Chip Panarchy
Hi

Sorry about this post, tried removing myself from this list about a
week ago, yet I keep getting messages.

Please remove from list

Thanks in advance,

Panarchy




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:
 On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
 This adds notifiers for phys memory changes: a set of callbacks that
 vhost can register and update kernel accordingly.  Down the road, kvm
 code can be switched to use these as well, instead of calling kvm code
 directly from exec.c as is done now.


 +
 +static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
 + CPUPhysMemoryClient *client)
 +{
 +PhysPageDesc *pd;
 +int l1, l2;
 +
 +for (l1 = 0; l1  L1_SIZE; ++l1) {
 +pd = phys_map[l1];
 +if (!pd) {
 +continue;
 +}
 +for (l2 = 0; l2  L2_SIZE; ++l2) {
 +if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
 +continue;
 +}
 +client-set_memory(client, pd[l2].region_offset,
 +   TARGET_PAGE_SIZE, pd[l2].phys_offset);
 +}
 +}
 +}
 +
 +static void phys_page_for_each(CPUPhysMemoryClient *client)
 +{
 +#if TARGET_PHYS_ADDR_SPACE_BITS  32
 +
 +#if TARGET_PHYS_ADDR_SPACE_BITS  (32 + L1_BITS)
 +#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
 +#endif
 +void **phys_map = (void **)l1_phys_map;
 +int l1;
 +if (!l1_phys_map) {
 +return;
 +}
 +for (l1 = 0; l1  L1_SIZE; ++l1) {
 +if (phys_map[l1]) {
 +phys_page_for_each_in_l1_map(phys_map[l1], client);
 +}
 +}
 +#else
 +if (!l1_phys_map) {
 +return;
 +}
 +phys_page_for_each_in_l1_map(l1_phys_map, client);
 +#endif
 +}


 This looks pretty frightening.  What is it needed for?

The point is that clients can be registered at any point.

A client that registered when memory is present needs to
be notified about it.


 I think we should stick with range operations, but maybe I misunderstood  
 something here.
 Otherwise, I like this patchset.

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




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Avi Kivity

On 01/18/2010 03:52 PM, Michael S. Tsirkin wrote:

On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:
   

On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
 

This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly.  Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.


+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1   L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2   L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+   TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS   32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS   (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1   L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}

   

This looks pretty frightening.  What is it needed for?
 

The point is that clients can be registered at any point.

A client that registered when memory is present needs to
be notified about it.
   


It looks very expensive.  Maybe we mandate clients be registered at 
init-time?


Long term we need to move to a range based memory description.

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





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

2010-01-18 Thread Luiz Capitulino
On Fri, 15 Jan 2010 13:54:29 -0600
Adam Litke a...@us.ibm.com wrote:

 This version improves support for multiple monitors and has been ported up to
 HEAD as of 01/14.

 Overall review on the Monitor related changes seems ok, but I'm not sure
how I should enable it.




Re: [Qemu-devel] [PATCH 2/6] QError: New QERR_FOPEN_FAILED

2010-01-18 Thread Luiz Capitulino
On Fri, 15 Jan 2010 17:25:25 +0100
Markus Armbruster arm...@redhat.com wrote:

 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  qerror.c |4 
  qerror.h |3 +++
  2 files changed, 7 insertions(+), 0 deletions(-)
 
 diff --git a/qerror.c b/qerror.c
 index 5f8fc5d..e7b8ca7 100644
 --- a/qerror.c
 +++ b/qerror.c
 @@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
  .desc  = No file descriptor supplied via SCM_RIGHTS,
  },
  {
 +.error_fmt = QERR_FOPEN_FAILED,
 +.desc  = Could not open '%{filename}',
 +},

 Shouldn't this be something like QERR_OPEN_FAILED, so that we
can use the same error for all open functions?

 Also, we have to think a way to specify the reason from errno.




Re: [Qemu-devel] [PATCH 2/6] QError: New QERR_FOPEN_FAILED

2010-01-18 Thread Luiz Capitulino
On Mon, 18 Jan 2010 12:23:28 -0200
Luiz Capitulino lcapitul...@redhat.com wrote:

 On Fri, 15 Jan 2010 17:25:25 +0100
 Markus Armbruster arm...@redhat.com wrote:
 
  
  Signed-off-by: Markus Armbruster arm...@redhat.com
  ---
   qerror.c |4 
   qerror.h |3 +++
   2 files changed, 7 insertions(+), 0 deletions(-)
  
  diff --git a/qerror.c b/qerror.c
  index 5f8fc5d..e7b8ca7 100644
  --- a/qerror.c
  +++ b/qerror.c
  @@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
   .desc  = No file descriptor supplied via SCM_RIGHTS,
   },
   {
  +.error_fmt = QERR_FOPEN_FAILED,
  +.desc  = Could not open '%{filename}',
  +},
 
  Shouldn't this be something like QERR_OPEN_FAILED, so that we
 can use the same error for all open functions?

 Something I forgot to mention, 'FopenFailed' doesn't seem
interesting to clients. Maybe 'OpenFileFailed'?




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 03:58:51PM +0200, Avi Kivity wrote:
 On 01/18/2010 03:52 PM, Michael S. Tsirkin wrote:
 On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:

 On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
  
 This adds notifiers for phys memory changes: a set of callbacks that
 vhost can register and update kernel accordingly.  Down the road, kvm
 code can be switched to use these as well, instead of calling kvm code
 directly from exec.c as is done now.


 +
 +static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
 + CPUPhysMemoryClient *client)
 +{
 +PhysPageDesc *pd;
 +int l1, l2;
 +
 +for (l1 = 0; l1   L1_SIZE; ++l1) {
 +pd = phys_map[l1];
 +if (!pd) {
 +continue;
 +}
 +for (l2 = 0; l2   L2_SIZE; ++l2) {
 +if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
 +continue;
 +}
 +client-set_memory(client, pd[l2].region_offset,
 +   TARGET_PAGE_SIZE, pd[l2].phys_offset);
 +}
 +}
 +}
 +
 +static void phys_page_for_each(CPUPhysMemoryClient *client)
 +{
 +#if TARGET_PHYS_ADDR_SPACE_BITS   32
 +
 +#if TARGET_PHYS_ADDR_SPACE_BITS   (32 + L1_BITS)
 +#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
 +#endif
 +void **phys_map = (void **)l1_phys_map;
 +int l1;
 +if (!l1_phys_map) {
 +return;
 +}
 +for (l1 = 0; l1   L1_SIZE; ++l1) {
 +if (phys_map[l1]) {
 +phys_page_for_each_in_l1_map(phys_map[l1], client);
 +}
 +}
 +#else
 +if (!l1_phys_map) {
 +return;
 +}
 +phys_page_for_each_in_l1_map(l1_phys_map, client);
 +#endif
 +}


 This looks pretty frightening.  What is it needed for?
  
 The point is that clients can be registered at any point.

 A client that registered when memory is present needs to
 be notified about it.


 It looks very expensive.

Shouldn't be hard to optimize ...

 Maybe we mandate clients be registered at init-time?


This might be tricky - vhost currently only registers when the
first device is hot-added.

 Long term we need to move to a range based memory description.

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




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Avi Kivity

On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote:


   

The point is that clients can be registered at any point.

A client that registered when memory is present needs to
be notified about it.

   

It looks very expensive.
 

Shouldn't be hard to optimize ...
   


It's O(memory size), which can be very big.


Maybe we mandate clients be registered at init-time?
 


This might be tricky - vhost currently only registers when the
first device is hot-added.
   


I see.

Maybe coalesce adjacent pages and call the callback with the ranges?

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





Re: [Qemu-devel] [PATCH 2/6] QError: New QERR_FOPEN_FAILED

2010-01-18 Thread Markus Armbruster
Luiz Capitulino lcapitul...@redhat.com writes:

 On Fri, 15 Jan 2010 17:25:25 +0100
 Markus Armbruster arm...@redhat.com wrote:

 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 ---
  qerror.c |4 
  qerror.h |3 +++
  2 files changed, 7 insertions(+), 0 deletions(-)
 
 diff --git a/qerror.c b/qerror.c
 index 5f8fc5d..e7b8ca7 100644
 --- a/qerror.c
 +++ b/qerror.c
 @@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
  .desc  = No file descriptor supplied via SCM_RIGHTS,
  },
  {
 +.error_fmt = QERR_FOPEN_FAILED,
 +.desc  = Could not open '%{filename}',
 +},

  Shouldn't this be something like QERR_OPEN_FAILED, so that we
 can use the same error for all open functions?

Whatever name you like best.  The intention is certainly to use this for
*file* open errors regardless of the precise function used to open.

  Also, we have to think a way to specify the reason from errno.

Yes only if client programs use this for handling the error, not just to
pass it to a human user.

Although most errors are standardized, their numeric encoding is not, so
we can't just transmit errno.  strerror() is right out, because that's
for humans.




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 04:52:10PM +0200, Avi Kivity wrote:
 On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote:


 The point is that clients can be registered at any point.

 A client that registered when memory is present needs to
 be notified about it.


 It looks very expensive.
  
 Shouldn't be hard to optimize ...


 It's O(memory size), which can be very big.
 Maybe we mandate clients be registered at init-time?
  

 This might be tricky - vhost currently only registers when the
 first device is hot-added.


 I see.

 Maybe coalesce adjacent pages and call the callback with the ranges?

Yes, I'll do that.




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 04:52:10PM +0200, Avi Kivity wrote:
 On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote:


 The point is that clients can be registered at any point.

 A client that registered when memory is present needs to
 be notified about it.


 It looks very expensive.
  
 Shouldn't be hard to optimize ...


 It's O(memory size), which can be very big.

cpu_register_physical_memory_offset already is O(memory size) btw.

 Maybe we mandate clients be registered at init-time?
  

 This might be tricky - vhost currently only registers when the
 first device is hot-added.


 I see.

 Maybe coalesce adjacent pages and call the callback with the ranges?

Hmm, it turns out to be tricky: it seems whether we can do this
really depends on what get_ram_ptr returns ...
Can't we just rely on callback to do the coalescing?


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




Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Avi Kivity

On 01/18/2010 05:45 PM, Michael S. Tsirkin wrote:


cpu_register_physical_memory_offset already is O(memory size) btw.
   


Right, but we'd like to replace it with a range API.

   

Maybe we mandate clients be registered at init-time?

 

This might be tricky - vhost currently only registers when the
first device is hot-added.

   

I see.

Maybe coalesce adjacent pages and call the callback with the ranges?
 

Hmm, it turns out to be tricky: it seems whether we can do this
really depends on what get_ram_ptr returns ...
Can't we just rely on callback to do the coalescing?
   


If the callback can do the coalescing, surely the caller can as well?

This way we don't introduce a new per-page API.

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





Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 06:04:40PM +0200, Avi Kivity wrote:
 On 01/18/2010 05:45 PM, Michael S. Tsirkin wrote:

 cpu_register_physical_memory_offset already is O(memory size) btw.


 Right, but we'd like to replace it with a range API.

So, when we do the implementation of notifiers can follow?

 Maybe we mandate clients be registered at init-time?

  
 This might be tricky - vhost currently only registers when the
 first device is hot-added.


 I see.

 Maybe coalesce adjacent pages and call the callback with the ranges?
  
 Hmm, it turns out to be tricky: it seems whether we can do this
 really depends on what get_ram_ptr returns ...
 Can't we just rely on callback to do the coalescing?


 If the callback can do the coalescing, surely the caller can as well?

The callback calls qemu_ram_ptr and coalesces when the virtual memory
pointers are matching. caller can do this as well but it looks ugly: we
don't know this is what caller does ...

 This way we don't introduce a new per-page API.

What do you mean by new per-page API?
The API is range-based, implementation
currently scans all pages.

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




[Qemu-devel] Re: sparc32 do_unassigned_access overhaul v2

2010-01-18 Thread Artyom Tarasenko
Btw, what is the following hack for in do_unassigned_access?


saved_env = env;
env = cpu_single_env;
//...
env = saved_env;

I wonder whether I modify the correct env here:

   env = saved_env;

+/* flush neverland mappings created during no-fault mode,
+   so the sequential MMU faults report proper fault types */
+if (env-mmuregs[0]  MMU_NF) {
+tlb_flush(env, 1);
+}

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/




Re: [Qemu-devel] [PATCH 2/6] QError: New QERR_FOPEN_FAILED

2010-01-18 Thread Luiz Capitulino
On Mon, 18 Jan 2010 15:52:13 +0100
Markus Armbruster arm...@redhat.com wrote:

 Luiz Capitulino lcapitul...@redhat.com writes:
 
  On Fri, 15 Jan 2010 17:25:25 +0100
  Markus Armbruster arm...@redhat.com wrote:
 
  
  Signed-off-by: Markus Armbruster arm...@redhat.com
  ---
   qerror.c |4 
   qerror.h |3 +++
   2 files changed, 7 insertions(+), 0 deletions(-)
  
  diff --git a/qerror.c b/qerror.c
  index 5f8fc5d..e7b8ca7 100644
  --- a/qerror.c
  +++ b/qerror.c
  @@ -73,6 +73,10 @@ static const QErrorStringTable qerror_table[] = {
   .desc  = No file descriptor supplied via SCM_RIGHTS,
   },
   {
  +.error_fmt = QERR_FOPEN_FAILED,
  +.desc  = Could not open '%{filename}',
  +},
 
   Shouldn't this be something like QERR_OPEN_FAILED, so that we
  can use the same error for all open functions?
 
 Whatever name you like best.  The intention is certainly to use this for
 *file* open errors regardless of the precise function used to open.

 Probably OpenFileFailed or CantOpenFile.

   Also, we have to think a way to specify the reason from errno.
 
 Yes only if client programs use this for handling the error, not just to
 pass it to a human user.
 
 Although most errors are standardized, their numeric encoding is not, so
 we can't just transmit errno.  strerror() is right out, because that's
 for humans.

 IIRC we had a conversation about this and the conclusion was that we
will need our own qerror_strerror() or something like that.




[Qemu-devel] [PATCH] Add definitions for current cpu models..

2010-01-18 Thread john cooper
This is a rework of the prior version which adds definitions
for contemporary processors selected via -cpu model, as an
alternative to the existing use of -cpu qemu64 augmented
with a series of feature flags.

The primary motivation was determination of a least common
denominator within a given processor class to simplify guest
migration.  It is still possible to modify an arbitrary model
via additional feature flags however the goal here was to
make doing so unnecessary in typical usage.  The other
consideration was providing models names reflective of
current processors.  Both AMD and Intel have reviewed the
models in terms of balancing generality of migration vs.
excessive feature downgrade relative to released silicon. 

Concerning the prior version of the patch, the proposed name
used for a given model drew a fair amount of debate, the
main concern being use of names as mnemonic as possible to
the wisest group of users.  Another suggestion was to use
the vendor name of released silicon corresponding to a least
common denominator CPU within the class, rational being doing
so is more definitive of the intended functionality.  However
something like:

 -cpu Intel Core 2 Duo P9xxx

probably isn't all that easy to remember nor type when
selecting a Penryn class cpu.  So I struck what I believe to
be a reasonable compromise where the original x86_def_t.name
was for the most part retained with the x86_def_t.model_id
capturing the marketing name of the cpu being used as the
least common denominator for the class.  To make it easier for
a user to associate a *.name with *.model_id, -cpu ? invoked
rather as -cpu ?? will append *.model_id to the generated
table:

:
x86   Conroe  Intel Celeron_4x0 (Conroe/Merom Class Core 2)   
x86   Penryn  Intel Core 2 Duo P9xxx (Penryn Class Core 2)
x86  Nehalem  Intel Core i7 9xx (Nehalem Class Core i7)   
x86   Opteron_G1  AMD Opteron 240 (Gen 1 Class Opteron)   
x86   Opteron_G2  AMD Opteron 22xx (Gen 2 Class Opteron)  
x86   Opteron_G3  AMD Opteron 23xx (Gen 3 Class Opteron)
: 

As before a cpu feature 'check' option is added which warns when
feature flags (either implicit in a cpu model or explicit on the
command line) would have otherwise been quietly unavailable to a
guest:

# qemu-system-x86_64 ... -cpu Nehalem,check
warning: host cpuid _0001 lacks requested flag 'sse4.2' [0x0010]
warning: host cpuid _0001 lacks requested flag 'popcnt' [0x0080]

This patch was tested relative to qemu.git.

Signed-off-by: john cooper john.coo...@redhat.com
---

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f3834b3..58400ab 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -722,8 +722,8 @@ typedef struct CPUX86State {
 CPUX86State *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void cpu_x86_close(CPUX86State *s);
-void x86_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt,
- ...));
+void x86_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+const char *optarg);
 int cpu_get_pic_interrupt(CPUX86State *s);
 /* MSDOS compatibility mode FPU exception support */
 void cpu_set_ferr(CPUX86State *s);
@@ -875,7 +875,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define cpu_exec cpu_x86_exec
 #define cpu_gen_code cpu_x86_gen_code
 #define cpu_signal_handler cpu_x86_signal_handler
-#define cpu_list x86_cpu_list
+#define cpu_list_id x86_cpu_list
 
 #define CPU_SAVE_VERSION 11
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 730e396..34f4936 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -42,7 +42,7 @@ static const char *feature_name[] = {
 static const char *ext_feature_name[] = {
 pni /* Intel,AMD sse3 */, NULL, NULL, monitor, ds_cpl, vmx, NULL 
/* Linux smx */, est,
 tm2, ssse3, cid, NULL, NULL, cx16, xtpr, NULL,
-NULL, NULL, dca, NULL, NULL, NULL, NULL, popcnt,
+NULL, NULL, dca, sse4.1, sse4.2, x2apic, NULL, popcnt,
 NULL, NULL, NULL, NULL, NULL, NULL, NULL, hypervisor,
 };
 static const char *ext2_feature_name[] = {
@@ -58,6 +58,18 @@ static const char *ext3_feature_name[] = {
 NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+/* collects per-function cpuid data
+ */
+typedef struct model_features_t {
+uint32_t *guest_feat;
+uint32_t *host_feat;
+uint32_t check_feat;
+const char **flag_names;
+uint32_t cpuid;
+} model_features_t;
+
+int check_cpuid = 0;
+
 static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
 uint32_t *ext_features,
 uint32_t *ext2_features,
@@ -111,10 +123,25 @@ typedef struct x86_def_t {
   CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \
   CPUID_PSE36 | CPUID_FXSR)
 

Re: [Qemu-devel] [PATCH 4/5] PCI: do_pci_info(): PCI bridge support

2010-01-18 Thread Markus Armbruster
Luiz Capitulino lcapitul...@redhat.com writes:

 This commit adds the pci_bridge key to the PCI device QDict,
 it also adds support for printing it in the user protocol.

 IMPORTANT: This code is being added separately because I could
 NOT test it properly. According to Michael Tsirkin, it depends
 on ultrasparc and it would take time to do the proper setup.

 Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
 ---
  hw/pci.c |   76 +++--
  1 files changed, 73 insertions(+), 3 deletions(-)

 diff --git a/hw/pci.c b/hw/pci.c
 index 8275ceb..d5e4866 100644
 --- a/hw/pci.c
 +++ b/hw/pci.c
 @@ -1102,6 +1102,7 @@ void pci_for_each_device(PCIBus *bus, int bus_num,
  
  static void pci_device_print(Monitor *mon, QDict *device)
  {
 +int class;
  QDict *qdict;
  QListEntry *entry;
  uint64_t addr, size;
 @@ -1113,10 +1114,11 @@ static void pci_device_print(Monitor *mon, QDict 
 *device)
  monitor_printf(mon, );
  
  qdict = qdict_get_qdict(device, class_info);
 +class = qdict_get_int(qdict, class);
  if (qdict_haskey(qdict, desc)) {
  monitor_printf(mon, %s, qdict_get_str(qdict, desc));
  } else {
 -monitor_printf(mon, Class %04 PRId64, qdict_get_int(qdict, 
 class));
 +monitor_printf(mon, Class %d, class);
  }
  
  qdict = qdict_get_qdict(device, id);

This change seems unrelated.  Is it intentional?

[...]




Re: [Qemu-devel] [PATCH 3/5] PCI: Convert pci_info() to QObject

2010-01-18 Thread Markus Armbruster
Luiz Capitulino lcapitul...@redhat.com writes:

 The returned QObject is a QList of all buses. Each bus is
 represented by a QDict, which has a key with a QList of all
 PCI devices attached to it. Each device is represented by
 a QDict.

 IMPORTANT: support for printing PCI bridge information and
 its devices is NOT part of this commit, it's going to be
 added by next commits.

 Finally, as has happended to other complex conversions,
 it's hard to split this commit as part of it are new
 functions which are called by each other.

 Signed-off-by: Luiz Capitulino lcapitul...@redhat.com

Looks pretty good.  A few remarks follow inline.

 ---
  hw/pci.c  |  308 
 +
  hw/pci.h  |4 +-
  monitor.c |3 +-
  3 files changed, 234 insertions(+), 81 deletions(-)

 diff --git a/hw/pci.c b/hw/pci.c
 index 08e51f8..8275ceb 100644
 --- a/hw/pci.c
 +++ b/hw/pci.c
 @@ -27,6 +27,7 @@
  #include net.h
  #include sysemu.h
  #include loader.h
 +#include qemu-objects.h
  
  //#define DEBUG_PCI
  #ifdef DEBUG_PCI
 @@ -1075,119 +1076,268 @@ static const pci_class_desc 
 pci_class_descriptions[] =
  { 0, NULL}
  };
  
 -static void pci_info_device(PCIBus *bus, PCIDevice *d)
 +static void pci_for_each_device_under_bus(PCIBus *bus,
 +  void (*fn)(PCIBus *b, PCIDevice 
 *d))
  {
 -Monitor *mon = cur_mon;
 -int i, class;
 -PCIIORegion *r;
 -const pci_class_desc *desc;
 +PCIDevice *d;
 +int devfn;
  
 -monitor_printf(mon,   Bus %2d, device %3d, function %d:\n,
 -   pci_bus_num(d-bus),
 -   PCI_SLOT(d-devfn), PCI_FUNC(d-devfn));
 -class = pci_get_word(d-config + PCI_CLASS_DEVICE);
 +for(devfn = 0; devfn  ARRAY_SIZE(bus-devices); devfn++) {
 +d = bus-devices[devfn];
 +if (d) {
 +fn(bus, d);
 +}
 +}
 +}
 +
 +void pci_for_each_device(PCIBus *bus, int bus_num,
 + void (*fn)(PCIBus *b, PCIDevice *d))
 +{
 +bus = pci_find_bus(bus, bus_num);
 +
 +if (bus) {
 +pci_for_each_device_under_bus(bus, fn);
 +}
 +}
 +
 +static void pci_device_print(Monitor *mon, QDict *device)
 +{
 +QDict *qdict;
 +QListEntry *entry;
 +uint64_t addr, size;
 +
 +monitor_printf(mon,   Bus %2 PRId64 , , qdict_get_int(device, 
 bus));
 +monitor_printf(mon, device %3 PRId64 , function % PRId64 :\n,
 +qdict_get_int(device, slot),
 +qdict_get_int(device, function));
  monitor_printf(mon, );
 +
 +qdict = qdict_get_qdict(device, class_info);
 +if (qdict_haskey(qdict, desc)) {
 +monitor_printf(mon, %s, qdict_get_str(qdict, desc));
 +} else {
 +monitor_printf(mon, Class %04 PRId64, qdict_get_int(qdict, 
 class));
 +}
 +
 +qdict = qdict_get_qdict(device, id);
 +monitor_printf(mon, : PCI device %04 PRIx64 :%04 PRIx64 \n,
 +qdict_get_int(qdict, device),
 +qdict_get_int(qdict, vendor));
 +
 +if (qdict_haskey(device, irq)) {
 +monitor_printf(mon,   IRQ % PRId64 .\n,
 +qdict_get_int(device, irq));
 +}
 +
 +/* TODO: PCI bridge info */
 +
 +QLIST_FOREACH_ENTRY(qdict_get_qlist(device, regions), entry) {
 +qdict = qobject_to_qdict(qlist_entry_obj(entry));
 +monitor_printf(mon,   BAR%d: , (int) qdict_get_int(qdict, 
 bar));
 +
 +addr = qdict_get_int(qdict, address);
 +size = qdict_get_int(qdict, size);
 +
 +if (!strcmp(qdict_get_str(qdict, type), io)) {
 +monitor_printf(mon, I/O at 0x%04FMT_PCIBUS
 + [0x%04FMT_PCIBUS].\n,
 +addr, addr + size - 1);
 +} else {
 +monitor_printf(mon, %d bit%s memory at 0x%08FMT_PCIBUS
 +[0x%08FMT_PCIBUS].\n,
 +qdict_get_bool(qdict, mem_type_64) ? 64 : 
 32,
 +qdict_get_bool(qdict, prefetch) ?
 + prefetchable : , addr, addr + size - 1);
 +}
 +}
 +
 +monitor_printf(mon,   id \%s\\n, qdict_get_str(device, 
 qdev_id));
 +
 +/* TODO: PCI bridge devices */
 +}
 +
 +void do_pci_info_print(Monitor *mon, const QObject *data)
 +{
 +QListEntry *bus, *dev;
 +
 +QLIST_FOREACH_ENTRY(qobject_to_qlist(data), bus) {
 +QDict *qdict = qobject_to_qdict(qlist_entry_obj(bus));
 +QLIST_FOREACH_ENTRY(qdict_get_qlist(qdict, devices), dev) {
 +pci_device_print(mon, qobject_to_qdict(qlist_entry_obj(dev)));
 +}
 +}
 +}
 +
 +static QObject *pci_get_dev_class(const PCIDevice *dev)
 +{
 +int class;
 +const char *str = ;
 +const pci_class_desc *desc;
 +
 +class = pci_get_word(dev-config + PCI_CLASS_DEVICE);
  desc = pci_class_descriptions;
  while (desc-desc  

Re: [Qemu-devel] [PATCH v1 0/5]: Convert pci_info() to QObject

2010-01-18 Thread Markus Armbruster
Luiz Capitulino lcapitul...@redhat.com writes:

  Hi,

  This is a new version of the pci_info() conversion.

 changelog
 -

 V0 - V1

 - Coding style fixes
 - Make 'BAR' and 'IRQ' keys lowercase
 - Add 'irq' key to the documentation

  Thanks.

PATCH 3/5 regresses info pci, 4/5 and 5/5 fix it.  Do we care?  They're
separate because they're untested.




[Qemu-devel] Re: sparc32 do_unassigned_access overhaul v2

2010-01-18 Thread Blue Swirl
On Mon, Jan 18, 2010 at 4:17 PM, Artyom Tarasenko
atar4q...@googlemail.com wrote:
 Btw, what is the following hack for in do_unassigned_access?


    saved_env = env;
    env = cpu_single_env;
 //...
    env = saved_env;

env is a host CPU register, see for example target-sparc/exec.h. Code
which is called directly from translated code (and cpu-exec.c) is
compiled this way.

I'm not sure if do_unassigned_access will ever be called from outside
of translated code, grep hits were from exec.c, cpu-exec.c and
op_helper.c.

 I wonder whether I modify the correct env here:

   env = saved_env;

 +    /* flush neverland mappings created during no-fault mode,
 +       so the sequential MMU faults report proper fault types */
 +    if (env-mmuregs[0]  MMU_NF) {
 +        tlb_flush(env, 1);
 +    }

Right, if env was NULL when entering the function, it will crash.




Re: [Qemu-devel] [PATCH] Improve error reporting on file access

2010-01-18 Thread Markus Armbruster
Andreas Färber andreas.faer...@web.de writes:

 Hello,

 Am 27.10.2009 um 18:38 schrieb malc:

 On Tue, 27 Oct 2009, Markus Armbruster wrote:

 Mark McLoughlin mar...@redhat.com writes:

 On Thu, 2009-10-01 at 09:42 -0500, Justin M. Forbes wrote:
 Author: Justin M. Forbes jfor...@redhat.com
 Date:   Thu Oct 1 09:34:56 2009 -0500

Improve error reporting on file access

By making the error reporting include strerror(errno), it
 gives the user
a bit more indication as to why qemu failed.  This is
 particularly
important for people running qemu as a non root user.

Signed-off-by: Justin M. Forbes jfor...@redhat.com

 Only concern is that errno might not be getting propagated
 correctly by
 some of these functions, but we can fix that later if so.

 Here's one:

 diff --git a/vl.c b/vl.c
 index 7bfd415..70fd2ca 100644
 --- a/vl.c
 +++ b/vl.c
 @@ -2232,8 +2232,8 @@ DriveInfo *drive_init(QemuOpts *opts, void
 *opaque,
 }

 if (bdrv_open2(dinfo-bdrv, file, bdrv_flags, drv)  0) {
 -fprintf(stderr, qemu: could not open disk image %s\n,
 -file);
 +fprintf(stderr, qemu: could not open disk image %s: %s\n,
 +file, strerror(errno));
 return NULL;
 }


 bdrv_open2 is not guaranteed to use POSIX functions for it's file
 manipulation, hence the patch is wrong.

 It appears, the patch was applied in
 850810d01b45e6ce99ac6696773e967890db2937 (Oct 5).

 On OpenSolaris 2009.06 amd64 I now get:

 qemu: could not open disk image /[...].iso: Not owner

 I am owner though. If I run it with pfexec (priviledged), I get:

 qemu: could not open disk image /[...].iso: No such file or directory

 The file is there and my script used to work before Juan's Makefile
 reorganization with the --whole-archive workaround I posted.

 So my guess is, we do see a stray errno here?

 Andreas

As malc said, the patch is wrong.  It should be reverted until somebody
comes up with a fix.




[Qemu-devel] Re: [PATCHv2 0/3] rwhandler: introduce and switch pci_host to it

2010-01-18 Thread Blue Swirl
On Mon, Jan 18, 2010 at 10:56 AM, Michael S. Tsirkin m...@redhat.com wrote:
 Alexander, so I assume the following patchset should be enough for you
 to implement u3 support, simply by creating your own rwhandler, and
 using pci_data_read/write directly there. I have pushed it to a
 temporary branch in my tree:
 git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git rwhandler

 Paul, any comments on this approach?

 I'll push this to my pci tree if this turns out to be helpful.
 Hope this helps, and sorry about the churn.

I proposed earlier something similar for MMIO. The thread could be interesting:
http://lists.gnu.org/archive/html/qemu-devel/2009-05/msg00095.html




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

2010-01-18 Thread Adam Litke
On Mon, 2010-01-18 at 12:12 -0200, Luiz Capitulino wrote:
 On Fri, 15 Jan 2010 13:54:29 -0600
 Adam Litke a...@us.ibm.com wrote:
 
  This version improves support for multiple monitors and has been ported up 
  to
  HEAD as of 01/14.
 
  Overall review on the Monitor related changes seems ok, but I'm not sure
 how I should enable it.

You must run a Linux guest with the virtio balloon enabled (-balloon
virtio) and the guest kernel must support the stats API.  Rusty Russell
has accepted my patch into his tree and will be pushing it upstream.  In
the meantime, you could build a 2.6.33-rc3 kernel with the following
patch applied...

commit b9bb81c2f7ce82d636d47089d6aa279d9be704f2
Author: li...@us.ibm.com agli...@kernel.beaverton.ibm.com
Date:   Fri Jan 8 14:17:45 2010 -0800

balloon memory stats patch

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 9dd5880..f95be86 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -28,7 +28,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;
@@ -49,6 +49,10 @@ struct virtio_balloon
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
u32 pfns[256];
+
+   /* Memory statistics */
+   int need_stats_update;
+   struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
 };
 
 static struct virtio_device_id id_table[] = {
@@ -154,6 +158,72 @@ static void leak_balloon(struct virtio_balloon *vb, size_t 
num)
}
 }
 
+static inline void update_stat(struct virtio_balloon *vb, int idx,
+  u16 tag, u64 val)
+{
+   BUG_ON(idx = VIRTIO_BALLOON_S_NR);
+   vb-stats[idx].tag = tag;
+   vb-stats[idx].val = val;
+}
+
+#define pages_to_bytes(x) ((u64)(x)  PAGE_SHIFT)
+
+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,
+   pages_to_bytes(events[PSWPIN]));
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
+   pages_to_bytes(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,
+   pages_to_bytes(i.freeram));
+   update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
+   pages_to_bytes(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.  Since stats collection can sleep,
+ * we notify our kthread which does the actual work via stats_handle_request().
+ */
+static void stats_request(struct virtqueue *vq)
+{
+   struct virtio_balloon *vb;
+   unsigned int len;
+
+   vb = vq-vq_ops-get_buf(vq, len);
+   if (!vb)
+   return;
+   vb-need_stats_update = 1;
+   wake_up(vb-config_change);
+}
+
+static void stats_handle_request(struct virtio_balloon *vb)
+{
+   struct virtqueue *vq;
+   struct scatterlist sg;
+
+   vb-need_stats_update = 0;
+   update_balloon_stats(vb);
+
+   vq = vb-stats_vq;
+   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;
@@ -190,8 +260,11 @@ static int balloon(void *_vballoon)
try_to_freeze();
wait_event_interruptible(vb-config_change,
 (diff = towards_target(vb)) != 0
+|| vb-need_stats_update
 || kthread_should_stop()
 || freezing(current));
+   if (vb-need_stats_update)
+   stats_handle_request(vb);
if (diff  0)
fill_balloon(vb, diff);
else if (diff  0)
@@ -204,10 +277,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, 

[Qemu-devel] Re: [PATCHv2 0/3] rwhandler: introduce and switch pci_host to it

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 05:50:35PM +, Blue Swirl wrote:
 On Mon, Jan 18, 2010 at 10:56 AM, Michael S. Tsirkin m...@redhat.com wrote:
  Alexander, so I assume the following patchset should be enough for you
  to implement u3 support, simply by creating your own rwhandler, and
  using pci_data_read/write directly there. I have pushed it to a
  temporary branch in my tree:
  git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git rwhandler
 
  Paul, any comments on this approach?
 
  I'll push this to my pci tree if this turns out to be helpful.
  Hope this helps, and sorry about the churn.
 
 I proposed earlier something similar for MMIO. The thread could be 
 interesting:
 http://lists.gnu.org/archive/html/qemu-devel/2009-05/msg00095.html

IIUC that patch seems to do much more, and also seems to involve all io?
This one is just a library that devices can use. Intended users are not
performance-critical like pci config.

-- 
MST




Re: [Qemu-devel] [PATCH] Documentation: Add missing documentation for qdev related command line options

2010-01-18 Thread Stefan Weil
Markus Armbruster schrieb:
 Stefan Weil w...@mail.berlios.de writes:

 The command line options -device, -nodefaults, -readconfig,
 -writeconfig had entries for command line help, but
 documentation for texi and derived formats (man, html, info)
 was missing.

 This also required moving @end table to the end of
 qemu-options.hx again.

 Signed-off-by: Stefan Weil w...@mail.berlios.de
 ---
 qemu-options.hx | 25 +
 1 files changed, 21 insertions(+), 4 deletions(-)

 diff --git a/qemu-options.hx b/qemu-options.hx
 index e2edd71..b2d04e2 100644
 --- a/qemu-options.hx
 +++ b/qemu-options.hx
 @@ -404,6 +404,12 @@ ETEXI

 DEF(device, HAS_ARG, QEMU_OPTION_device,
 -device driver[,options] add device\n)
 +STEXI
 +...@item -device @var{driver}[,@var{option}[,...]]
 +Add device @var{driver}. Depending on the device type,
 +...@var{option} (typically @var{ke...@var{value}) may be useful.
 +ETEXI
 +

 While there, would you mind improving --help for -device a bit? It's
 too terse, and it doesn't start the help text in column 16 like the
 other options do.

Hi Markus,

this needs a little more work. I just had a look on the code,
and there is no online help for the possible options (key, value).

If you (and especially those who have commit rights) agree,
I could provide these three additional patches:

* Add online help for properties (qemu -device driver,?)
* Add online help for property value (qemu -device driver,property=?)
* Update documentation for command line option -device

There is already an online help for the driver (qemu -device ?).

Regards,

Stefan





[Qemu-devel] Re: [PATCHv2 0/3] rwhandler: introduce and switch pci_host to it

2010-01-18 Thread Michael S. Tsirkin
On Mon, Jan 18, 2010 at 07:53:25PM +0200, Michael S. Tsirkin wrote:
 On Mon, Jan 18, 2010 at 05:50:35PM +, Blue Swirl wrote:
  On Mon, Jan 18, 2010 at 10:56 AM, Michael S. Tsirkin m...@redhat.com 
  wrote:
   Alexander, so I assume the following patchset should be enough for you
   to implement u3 support, simply by creating your own rwhandler, and
   using pci_data_read/write directly there. I have pushed it to a
   temporary branch in my tree:
   git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git rwhandler
  
   Paul, any comments on this approach?
  
   I'll push this to my pci tree if this turns out to be helpful.
   Hope this helps, and sorry about the churn.
  
  I proposed earlier something similar for MMIO. The thread could be 
  interesting:
  http://lists.gnu.org/archive/html/qemu-devel/2009-05/msg00095.html
 
 IIUC that patch seems to do much more, and also seems to involve all io?
 This one is just a library that devices can use. Intended users are not
 performance-critical like pci config.

To put it in other words: could you please be more explicit please?  I
failed to see the relevance ...


-- 
MST




[Qemu-devel] Re: [PATCHv2 0/3] rwhandler: introduce and switch pci_host to it

2010-01-18 Thread Blue Swirl
On Mon, Jan 18, 2010 at 7:41 PM, Michael S. Tsirkin m...@redhat.com wrote:
 On Mon, Jan 18, 2010 at 07:53:25PM +0200, Michael S. Tsirkin wrote:
 On Mon, Jan 18, 2010 at 05:50:35PM +, Blue Swirl wrote:
  On Mon, Jan 18, 2010 at 10:56 AM, Michael S. Tsirkin m...@redhat.com 
  wrote:
   Alexander, so I assume the following patchset should be enough for you
   to implement u3 support, simply by creating your own rwhandler, and
   using pci_data_read/write directly there. I have pushed it to a
   temporary branch in my tree:
   git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git rwhandler
  
   Paul, any comments on this approach?
  
   I'll push this to my pci tree if this turns out to be helpful.
   Hope this helps, and sorry about the churn.
 
  I proposed earlier something similar for MMIO. The thread could be 
  interesting:
  http://lists.gnu.org/archive/html/qemu-devel/2009-05/msg00095.html

 IIUC that patch seems to do much more, and also seems to involve all io?
 This one is just a library that devices can use. Intended users are not
 performance-critical like pci config.

 To put it in other words: could you please be more explicit please?  I
 failed to see the relevance ...

Well, I thought the stuff I proposed earlier may be useful for
comparison. It was also opt-in like yours, by the way.

Also the end result that Paul wasn't interested in my patch may (or
may not) be relevant with yours too.




[Qemu-devel] [PATCH] sparc64: reimplement tick timers v2

2010-01-18 Thread Igor V. Kovalenko
From: Igor V. Kovalenko igor.v.kovale...@gmail.com

sparc64 timer has tick counter which can be set and read,
and tick compare value used as deadline to fire timer interrupt.
The timer is not used as periodic timer, instead deadline
is set each time new timer interrupt is needed.

v1 - v2:
- new conversion helpers cpu_to_timer_ticks and timer_to_cpu_ticks
- save offset from clock source to implement cpu_tick_set_count
- renamed struct sun4u_timer to CPUTimer
- load and save cpu timers

v0 - v1:
- coding style

Signed-off-by: Igor V. Kovalenko igor.v.kovale...@gmail.com
---
 hw/sun4u.c |  214 +---
 target-sparc/cpu.h |9 ++
 target-sparc/machine.c |   12 +--
 3 files changed, 197 insertions(+), 38 deletions(-)

diff --git a/hw/sun4u.c b/hw/sun4u.c
index a39b28e..f9db758 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -280,6 +280,12 @@ void cpu_check_irqs(CPUState *env)
 }
 }
 
+static void cpu_kick_irq(CPUState *env)
+{
+env-halted = 0;
+cpu_check_irqs(env);
+}
+
 static void cpu_set_irq(void *opaque, int irq, int level)
 {
 CPUState *env = opaque;
@@ -301,6 +307,68 @@ typedef struct ResetData {
 uint64_t prom_addr;
 } ResetData;
 
+struct CPUTimer
+{
+const char *name;
+uint32_tfrequency;
+uint32_tdisabled;
+uint64_tdisabled_mask;
+int64_t clock_offset;
+QEMUTimer  *qtimer;
+};
+
+typedef struct CPUTimer CPUTimer;
+
+void cpu_put_timer(QEMUFile *f, CPUTimer *s)
+{
+qemu_put_be32s(f, s-frequency);
+qemu_put_be32s(f, s-disabled);
+qemu_put_be64s(f, s-disabled_mask);
+qemu_put_sbe64s(f, s-clock_offset);
+if (s-qtimer) {
+qemu_put_timer(f, s-qtimer);
+}
+}
+
+void cpu_get_timer(QEMUFile *f, CPUTimer *s)
+{
+qemu_get_be32s(f, s-frequency);
+qemu_get_be32s(f, s-disabled);
+qemu_get_be64s(f, s-disabled_mask);
+qemu_get_sbe64s(f, s-clock_offset);
+if (s-qtimer) {
+qemu_get_timer(f, s-qtimer);
+}
+}
+
+static CPUTimer* cpu_timer_create(const char* name, CPUState *env,
+  QEMUBHFunc *cb, uint32_t frequency,
+  uint64_t disabled_mask)
+{
+CPUTimer *timer;
+
+timer = qemu_mallocz(sizeof (CPUTimer));
+
+timer-name = name;
+timer-frequency = frequency;
+timer-disabled_mask = disabled_mask;
+
+timer-disabled = 1;
+timer-clock_offset = qemu_get_clock(vm_clock);
+
+timer-qtimer = qemu_new_timer(vm_clock, cb, env);
+
+return timer;
+}
+
+static void cpu_timer_reset(CPUTimer *timer)
+{
+timer-disabled = 1;
+timer-clock_offset = qemu_get_clock(vm_clock);
+
+qemu_del_timer(timer-qtimer);
+}
+
 static void main_cpu_reset(void *opaque)
 {
 ResetData *s = (ResetData *)opaque;
@@ -308,15 +376,11 @@ static void main_cpu_reset(void *opaque)
 static unsigned int nr_resets;
 
 cpu_reset(env);
-env-tick_cmpr = TICK_INT_DIS | 0;
-ptimer_set_limit(env-tick, TICK_MAX, 1);
-ptimer_run(env-tick, 1);
-env-stick_cmpr = TICK_INT_DIS | 0;
-ptimer_set_limit(env-stick, TICK_MAX, 1);
-ptimer_run(env-stick, 1);
-env-hstick_cmpr = TICK_INT_DIS | 0;
-ptimer_set_limit(env-hstick, TICK_MAX, 1);
-ptimer_run(env-hstick, 1);
+
+cpu_timer_reset(env-tick);
+cpu_timer_reset(env-stick);
+cpu_timer_reset(env-hstick);
+
 env-gregs[1] = 0; // Memory start
 env-gregs[2] = ram_size; // Memory size
 env-gregs[3] = 0; // Machine description XXX
@@ -333,44 +397,133 @@ static void tick_irq(void *opaque)
 {
 CPUState *env = opaque;
 
-if (!(env-tick_cmpr  TICK_INT_DIS)) {
-env-softint |= SOFTINT_TIMER;
-cpu_interrupt(env, CPU_INTERRUPT_TIMER);
+CPUTimer* timer = (CPUTimer*) env-tick;
+
+if (timer-disabled) {
+CPUIRQ_DPRINTF(tick_irq: softint disabled\n);
+return;
+} else {
+CPUIRQ_DPRINTF(tick: fire\n);
 }
+
+env-softint |= SOFTINT_TIMER;
+cpu_kick_irq(env);
 }
 
 static void stick_irq(void *opaque)
 {
 CPUState *env = opaque;
 
-if (!(env-stick_cmpr  TICK_INT_DIS)) {
-env-softint |= SOFTINT_STIMER;
-cpu_interrupt(env, CPU_INTERRUPT_TIMER);
+CPUTimer* timer = (CPUTimer*) env-stick;
+
+if (timer-disabled) {
+CPUIRQ_DPRINTF(stick_irq: softint disabled\n);
+return;
+} else {
+CPUIRQ_DPRINTF(stick: fire\n);
 }
+
+env-softint |= SOFTINT_STIMER;
+cpu_kick_irq(env);
 }
 
 static void hstick_irq(void *opaque)
 {
 CPUState *env = opaque;
 
-if (!(env-hstick_cmpr  TICK_INT_DIS)) {
-cpu_interrupt(env, CPU_INTERRUPT_TIMER);
+CPUTimer* timer = (CPUTimer*) env-hstick;
+
+if (timer-disabled) {
+CPUIRQ_DPRINTF(hstick_irq: softint disabled\n);
+return;
+} else {
+CPUIRQ_DPRINTF(hstick: fire\n);
 }
+
+env-softint |= SOFTINT_STIMER;
+cpu_kick_irq(env);
+}
+
+static int64_t cpu_to_timer_ticks(int64_t cpu_ticks, uint32_t 

[Qemu-devel] KVM call agenda for Jan 19

2010-01-18 Thread Chris Wright
Please send in any agenda items you are interested in covering in
tomorrow's call.

thanks,
-chris