Re: [Qemu-devel] [PATCH][STABLE] Fix corner case in chardev udp: parameter
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
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
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
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
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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.
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
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
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
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
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)
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
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
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
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
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
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
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
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
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
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
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
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..
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
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
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
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
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
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
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)
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
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
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
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
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
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
Please send in any agenda items you are interested in covering in tomorrow's call. thanks, -chris