Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command block_set for dynamic block params change
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
Index: qemu/block.c
===
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,40 @@ unlink_and_fail:
return ret;
}
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+BlockDriver *drv = bs-drv;
+int ret = 0;
+
+/* Quiesce IO for the given block device */
+qemu_aio_flush();
+if (bdrv_flush(bs)) {
+qerror_report(QERR_DATA_SYNC_FAILED, bs-device_name);
+return ret;
+}
+bdrv_close(bs);
+
+
+ret = bdrv_open(bs, bs-filename, bdrv_flags, drv);
+if (ret 0) {
+/* Reopen failed. Try to open with original flags */
+error_report(Opening file with changed flags...);
+qerror_report(QERR_REOPEN_FILE_FAILED, bs-filename);
If the next open fails too then there will be two qerror_report() calls.
This causes a warning and the new qerror is dropped. Please consolidate
the error reporting so there is only one error before returning from
this function.
+
+ret = bdrv_open(bs, bs-filename, bs-open_flags, drv);
bs-open_flags has been clobbered by the previous bdrv_open(). It would
be best to take a copy of bs-open_flags before bdrv_close(bs) above.
+if (ret 0) {
+/*
+ * Reopen failed with orig and modified flags
+*/
+error_report(Opening file with original flags...);
+qerror_report(QERR_REOPEN_FILE_FAILED, bs-filename);
+abort();
+}
+}
+
+return ret;
+}
+
void bdrv_close(BlockDriverState *bs)
{
if (bs-drv) {
@@ -691,6 +725,32 @@ void bdrv_close_all(void)
}
}
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
+{
+int bdrv_flags = bs-open_flags;
+
+/* set hostcache flags (without changing WCE/flush bits) */
+if (enable_host_cache) {
+bdrv_flags = ~BDRV_O_NOCACHE;
+} else {
+bdrv_flags |= BDRV_O_NOCACHE;
+}
+
+/* If no change in flags, no need to reopen */
+if (bdrv_flags == bs-open_flags) {
+return 0;
+}
+
+if (bdrv_is_inserted(bs)) {
+/* Reopen file with changed set of flags */
+return bdrv_reopen(bs, bdrv_flags);
+} else {
+/* Save hostcache change for future use */
+bs-open_flags = bdrv_flags;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Index: qemu/blockdev.c
===
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -793,3 +793,63 @@ int do_block_resize(Monitor *mon, const
return 0;
}
+
+
+/*
+ * Handle changes to block device settings, like hostcache,
+ * while guest is running.
+*/
+int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+BlockDriverState *bs = NULL;
+QemuOpts *opts;
+int enable;
+const char *device, *driver;
+int ret;
+
+/* Validate device */
+device = qdict_get_str(qdict, device);
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+return -1;
+}
+
+opts = qemu_opts_from_qdict(qemu_find_opts(device), qdict);
+if (opts == NULL) {
+return -1;
+}
+
+/* If input not in param=value format, display error */
+driver = qemu_opt_get(opts, driver);
+if (driver != NULL) {
+error_report(Invalid parameter %s, driver);
error_report() only works for HMP. Please use qerror_report() so both
HMP and QMP see the error. Same issue further down.
Stefan
Re: [Qemu-devel] [PATCH v1 1/1] Submit the codes for QEMU disk I/O limits.
On Mon, Jul 25, 2011 at 1:40 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Jul 25, 2011 at 5:25 AM, Zhi Yong Wu zwu.ker...@gmail.com wrote:
On Fri, Jul 22, 2011 at 6:54 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Jul 22, 2011 at 10:20 AM, Zhi Yong Wu wu...@linux.vnet.ibm.com
wrote:
+static void bdrv_block_timer(void *opaque)
+{
+ BlockDriverState *bs = opaque;
+ BlockQueue *queue = bs-block_queue;
+ uint64_t intval = 1;
+
+ while (!QTAILQ_EMPTY(queue-requests)) {
+ BlockIORequest *request;
+ int ret;
+
+ request = QTAILQ_FIRST(queue-requests);
+ QTAILQ_REMOVE(queue-requests, request, entry);
+
+ ret = queue-handler(request);
Please remove the function pointer and call qemu_block_queue_handler()
directly. This indirection is not needed and makes it harder to
follow the code.
Should it keep the same style with other queue implemetations such as
network queue? As you have known, network queue has one queue handler.
You mean net/queue.c:queue-deliver? There are two deliver functions,
yeah
qemu_deliver_packet() and qemu_vlan_deliver_packet(), which is why a
function pointer is necessary.
OK, The handler has been removed and invoked directly.
In this case there is only one handler function so the indirection is
not necessary.
+ if (ret == 0) {
+ QTAILQ_INSERT_HEAD(queue-requests, request, entry);
+ break;
+ }
+
+ qemu_free(request);
+ }
+
+ qemu_mod_timer(bs-block_timer, (intval * 10) +
qemu_get_clock_ns(vm_clock));
intval is always 1. The timer should be set to the next earliest deadline.
pls see bdrv_aio_readv/writev:
+ /* throttling disk read I/O */
+ if (bs-io_limits != NULL) {
+ if (bdrv_exceed_io_limits(bs, nb_sectors, false, wait_time)) {
+ ret = qemu_block_queue_enqueue(bs-block_queue, bs,
bdrv_aio_readv,
+ sector_num, qiov, nb_sectors, cb, opaque);
+ qemu_mod_timer(bs-block_timer, wait_time +
qemu_get_clock_ns(vm_clock));
The timer has been modified when the blk request is enqueued.
The current algorithm seems to be:
1. Queue requests that exceed the limit and set the timer.
2. Dequeue all requests when the timer fires.
Yeah, but for each dequeued requests, it will be still determined if
current I/O rate exceed that limits, if yes, it will still be enqueued
into block queue again.
3. Set 1s periodic timer.
Why is the timer set up as a periodic 1 second timer in bdrv_block_timer()?
I was aslo considering if we need to set up this type of timer before.
Since the timer has been modified after qemu_block_queue_enqueue()
function, this timer should be not modified here. I will remove this
line of line. thanks.
+ bs-slice_start[is_write] = real_time;
+
+ bs-io_disps-bytes[is_write] = 0;
+ if (bs-io_limits-bps[2]) {
+ bs-io_disps-bytes[!is_write] = 0;
+ }
All previous data should be discarded when a new time slice starts:
bs-io_disps-bytes[IO_LIMIT_READ] = 0;
bs-io_disps-bytes[IO_LIMIT_WRITE] = 0;
If only bps_rd is specified, bs-io_disps-bytes[IO_LIMIT_WRITE] will
never be used; i think that it should not be cleared here. right?
I think there is no advantage in keeping slices separate for
read/write. The code would be simpler and work the same if there is
only one slice and past history is cleared when a new slice starts.
Done
Stefan
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH v1 1/1] Submit the codes for QEMU disk I/O limits.
On Fri, Jul 22, 2011 at 6:54 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Jul 22, 2011 at 10:20 AM, Zhi Yong Wu wu...@linux.vnet.ibm.com
wrote:
+static void bdrv_block_timer(void *opaque)
+{
+ BlockDriverState *bs = opaque;
+ BlockQueue *queue = bs-block_queue;
+ uint64_t intval = 1;
+
+ while (!QTAILQ_EMPTY(queue-requests)) {
+ BlockIORequest *request;
+ int ret;
+
+ request = QTAILQ_FIRST(queue-requests);
+ QTAILQ_REMOVE(queue-requests, request, entry);
+
+ ret = queue-handler(request);
Please remove the function pointer and call qemu_block_queue_handler()
directly. This indirection is not needed and makes it harder to
follow the code.
+ if (ret == 0) {
+ QTAILQ_INSERT_HEAD(queue-requests, request, entry);
+ break;
+ }
+
+ qemu_free(request);
+ }
+
+ qemu_mod_timer(bs-block_timer, (intval * 10) +
qemu_get_clock_ns(vm_clock));
intval is always 1. The timer should be set to the next earliest deadline.
+}
+
void bdrv_register(BlockDriver *bdrv)
{
if (!bdrv-bdrv_aio_readv) {
@@ -476,6 +508,11 @@ static int bdrv_open_common(BlockDriverState *bs, const
char *filename,
goto free_and_fail;
}
+ /* throttling disk I/O limits */
+ if (bs-io_limits != NULL) {
+ bs-block_queue = qemu_new_block_queue(qemu_block_queue_handler);
+ }
+
#ifndef _WIN32
if (bs-is_temporary) {
unlink(filename);
@@ -642,6 +679,16 @@ int bdrv_open(BlockDriverState *bs, const char
*filename, int flags,
bs-change_cb(bs-change_opaque, CHANGE_MEDIA);
}
+ /* throttling disk I/O limits */
+ if (bs-io_limits != NULL) {
block_queue is allocated in bdrv_open_common() but these variables are
initialized in bdrv_open(). Can you move them together, I don't see a
reason to keep them apart?
+ bs-io_disps = qemu_mallocz(sizeof(BlockIODisp));
+ bs-block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
+ qemu_mod_timer(bs-block_timer, qemu_get_clock_ns(vm_clock));
Why is the timer being scheduled immediately? There are no queued requests.
+
+ bs-slice_start[0] = qemu_get_clock_ns(vm_clock);
+ bs-slice_start[1] = qemu_get_clock_ns(vm_clock);
+ }
+
return 0;
unlink_and_fail:
@@ -680,6 +727,15 @@ void bdrv_close(BlockDriverState *bs)
if (bs-change_cb)
bs-change_cb(bs-change_opaque, CHANGE_MEDIA);
}
+
+ /* throttling disk I/O limits */
+ if (bs-block_queue) {
+ qemu_del_block_queue(bs-block_queue);
3 space indent, should be 4 spaces.
+ }
+
+ if (bs-block_timer) {
qemu_free_timer() will only free() the timer memory but it will not
cancel the timer. Use qemu_del_timer() first to ensure that the timer
is not pending:
qemu_del_timer(bs-block_timer);
+ qemu_free_timer(bs-block_timer);
+ }
}
void bdrv_close_all(void)
@@ -1312,6 +1368,13 @@ void bdrv_get_geometry_hint(BlockDriverState *bs,
*psecs = bs-secs;
}
+/* throttling disk io limits */
+void bdrv_set_io_limits(BlockDriverState *bs,
+ BlockIOLimit *io_limits)
+{
+ bs-io_limits = io_limits;
This function takes ownership of io_limits but never frees it. I
suggest not taking ownership and just copying io_limits into
bs-io_limits so that the caller does not need to malloc() and the
lifecycle of bs-io_limits is completely under our control.
Easiest would be to turn BlockDriverState.io_limits into:
BlockIOLimit io_limits;
and just copy in bdrv_set_io_limits():
bs-io_limits = *io_limits;
bdrv_exceed_io_limits() returns quite quickly if no limits are set, so
I wouldn't worry about optimizing it out yet.
+}
+
/* Recognize floppy formats */
typedef struct FDFormat {
FDriveType drive;
@@ -2111,6 +2174,154 @@ char *bdrv_snapshot_dump(char *buf, int buf_size,
QEMUSnapshotInfo *sn)
return buf;
}
+bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
+ bool is_write, uint64_t *wait) {
+ int64_t real_time;
+ uint64_t bps_limit = 0;
+ double bytes_limit, bytes_disp, bytes_res, elapsed_time;
+ double slice_time = 0.1, wait_time;
+
+ if (bs-io_limits-bps[2]) {
+ bps_limit = bs-io_limits-bps[2];
Please define a constant (IO_LIMIT_TOTAL?) instead of using magic number 2.
+ } else if (bs-io_limits-bps[is_write]) {
+ bps_limit = bs-io_limits-bps[is_write];
+ } else {
+ if (wait) {
+ *wait = 0;
+ }
+
+ return false;
+ }
+
+ real_time = qemu_get_clock_ns(vm_clock);
+ if (bs-slice_start[is_write] + 1 = real_time) {
Please define a constant for the 100 ms time slice instead of using
1 directly.
+ bs-slice_start[is_write] = real_time;
+
+ bs-io_disps-bytes[is_write] = 0;
+
Re: [Qemu-devel] SeaBIOS image is lacking CONFIG_AHCI
On 2011-05-09 08:03, Jan Kiszka wrote: Hi Anthony, please rebuild SeaBIOS after enabling AHCI. QEMU's current version is still not able to boot from such controllers. This unfortunately still applies to 0.15-rc0. Please fix. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] Boot order problem
On Sun, 24 Jul 2011 09:30:49 +0300 Gleb Natapov g...@redhat.com wrote: On Fri, Jul 22, 2011 at 09:51:16AM +0900, Minoru Usui wrote: Hi, everyone I'm in trouble about boot order of VM. If anyone know cause of this problem, please let me know. The cause of the problem is the design. booindex and -boot only modifies the order in which bios will search for bootable device. It does not exclude devices from a boot device list. On following environment, I tried to boot from IDE CD-ROM device without inserting any bootable media, which is expected to fail, but VM was booting up from virtio HDD which was not specified as bootable device. * host : RHEL6.1(x86_64) guest: RHEL6.1(x86_64) * VM has IDE CD-ROM and virtio HDD. * There is no bootable media in IDE CD-ROM. * RHEL6.1 is installed in virtio HDD * Only IDE CD-ROM was spcified as bootable device. * XML configuration of libvirt is below. I tested boot dev and boot order setting, but both are booting up from virtio HDD. --- [boot dev setting version] os type arch='x86_64' machine='rhel6.1.0'hvm/type boot dev='cdrom'/ bootmenu enable='no'/ /os [boot order setting version] disk type='file' device='cdrom' driver name='qemu' type='raw'/ target dev='hdc' bus='ide'/ boot order='1'/ readonly/ address type='drive' controller='0' bus='1' unit='0'/ /disk --- I tested another one about boot order case on RHEL6.1, and I also faced another problem. VM has two virtio HDD. HDD1 is installed RHEL6.1, HDD2 is empty. I specified boot order to HDD1:1, HDD2:2, VM booted up from HDD1, but boot order HDD1:2, HDD2:1 case, VM couldn't boot up from HDD2. (It searched CD-ROM, NIC(gPXE), and finally stopped booting.) It seems seabios searches only 1 device per device list(HDD, CD-ROM, NET, FLOPPY). Is it true? boot order can specify per device, so shouldn't seabios search all device, even if it specifies multiple device per device list? -- Minoru Usui u...@mxm.nes.nec.co.jp
Re: [Qemu-devel] [RFC PATCH 0/5] qcow: coroutines cleanup
Am 22.07.2011 22:09, schrieb Frediano Ziglio: Il giorno ven, 22/07/2011 alle 12.10 +0200, Kevin Wolf ha scritto: Am 22.07.2011 11:26, schrieb Frediano Ziglio: 2011/7/22 Kevin Wolf kw...@redhat.com: Am 20.07.2011 15:56, schrieb Frediano Ziglio: These patches mostly cleanup some AIO code using coroutines. These patches apply to Kevin's repository, branch coroutine-block. Mostly they use stack instead of allocated AIO structure. Frediano Ziglio (5): qcow: allocate QCowAIOCB structure using stack qcow: QCowAIOCB field cleanup qcow: move some blocks of code to avoid useless variable initialization avoid dandling pointers qcow: small optimization initializing QCowAIOCB block/qcow.c | 210 + block/qcow2.c | 38 +++--- 2 files changed, 102 insertions(+), 146 deletions(-) Most of it looks good now. Did you include the RFC in the subject just because the coroutine work is in RFC state, too, or did you intend to tell me that I shouldn't merge yet? Kevin As these patches are first quite big patches I send (typo or small fixes do not counts) I just want to mark that I could write something really wrong. Just a way to avoid somebody having to send more patches and get more attention. Some projects are quite prone to merge even not that fine ones. I prefer to have some (a bit) pedantic comments and a real fix/improve. Now I removed the RFC from last update. The main reason is that I found your qemu-iotests repository which, I think should be merged to main repository, but it's just my opinion. Oh... qcow fails 004 test (even origin/coroutines-block) with a I/O error. Yup, you're right, I must have messed it up. Care to fix it or should I look into it? Care but I don't know if I'll have time before Thursday. However I found the problem, really strange. bdrv_read returns 0 for errors 0 for success and... bytes read on partial read! Now a qcow image of 128m is 560 bytes so when you read sector 1 you get 48 which is not a problem for qcow code. But if you replace bdrv_read with a bdrv_co_readv (your latest patch on coroutine-block) bdrv_co_readv return -EINVAL on partial read. Oh, that one. I think I have a fix for it somewhere, I'll include it in my series. Kevin
[Qemu-devel] [PATCH] qemu-char: fix the commit qemu-char: Print strerror message on failure
The commit 6e1db57b2ac9025c2443c665a0d9e78748637b26 missed patching
qemu_chr_open_win_file().
Signed-off-by: TeLeMan gele...@gmail.com
---
qemu-char.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index dcf7065..ea7abfe 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1782,7 +1782,7 @@ static int qemu_chr_open_win_pipe(QemuOpts *opts,
CharDriverState **_chr)
return 0;
}
-static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
+static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **_chr)
{
CharDriverState *chr;
WinCharState *s;
@@ -1793,12 +1793,14 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE
fd_out)
chr-opaque = s;
chr-chr_write = win_chr_write;
qemu_chr_generic_open(chr);
-return chr;
+
+*_chr = chr;
+return 0;
}
static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **_chr)
{
-return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr);
+return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), _chr);
}
static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr)
--
1.7.3.1.msysgit.0
Re: [Qemu-devel] [V11 00/15] virtio-9p: Use chroot to safely access files in passthrough security model
On Fri, Jun 24, 2011 at 01:52:09PM +0530, M. Mohan Kumar wrote: In passthrough security model, following symbolic links in the server side could result in TOCTTOU vulnerabilities. (http://en.wikipedia.org/wiki/Time-of-check-to-time-of-use) This patchset resolves this issue by creating a dedicated process which chroots into the share path and all file object access is done in the chroot environment. This patchset implements chroot enviroment, provides necessary functions that can be used by the passthrough function calls. This patchset is rebased on top of 9p coroutines patches posted to qemu-devel list http://lists.nongnu.org/archive/html/qemu-devel/2011-05/msg02796.html Changes from version V10: * Added support to do lstat and readlink from chroot process * Fixed an issue with dealing fds when qemu process reached maxfds limit Changes from version V9: * Error handling in special file object creation in virtio-9p-local.c Changes from version V8: * Make chmod and chown also operate under chroot process * Check for invalid path requests, minor cleanups Changes from version V7: * Add two chroot methods remove and rename * Minor cleanups like consolidating functions Changes from version V6: * Send only fd/errno in socket operations instead of FdInfo structure * Minor cleanups Changes from version V5: * Return errno on failure instead of setting errno * Minor cleanups like updated comments, enable CONFIG_THREAD if CONFIG_VIRTFS is enabled Changes from version V4: * Avoid using malloc/free inside chroot process * Seperate chroot server and client functions Changes from version V3 * Return EIO incase of socket read/write fail instead of exiting * Changed data types as suggested by Blue Swirl * Chroot process reports error through qemu process Changes from version V2 * Treat socket IO errors as fatal, ie qemu will exit * Split patchset based on chroot side (server) and qemu side(client) functionalities M. Mohan Kumar (15): Implement qemu_read_full virtio-9p: Enable CONFIG_THREAD if CONFIG_VIRTFS is enabled virtio-9p: Provide chroot worker side interfaces virtio-9p: Add qemu side interfaces for chroot environment virtio-9p: Add support to open a file in chroot environment virtio-9p: Create support in chroot environment virtio-9p: Support for creating special files virtio-9p: Add support for removing file or directory virtio-9p: Add support to rename virtio-9p: Move file post creation changes to none security model virtio-9p: Add support for chmod virtio-9p: Add support for chown virtio-9p: Chroot environment for other functions virtio-9p: Add stat functionality to chroot virtio-9p: Add readlink support to chroot Makefile.objs |1 + configure |1 + fsdev/file-op-9p.h|3 + hw/9pfs/virtio-9p-chroot-worker.c | 418 + hw/9pfs/virtio-9p-chroot.c| 174 +++ hw/9pfs/virtio-9p-chroot.h| 54 + hw/9pfs/virtio-9p-device.c| 24 ++ hw/9pfs/virtio-9p-local.c | 248 ++ osdep.c | 32 +++ qemu-common.h |2 + 10 files changed, 910 insertions(+), 47 deletions(-) create mode 100644 hw/9pfs/virtio-9p-chroot-worker.c create mode 100644 hw/9pfs/virtio-9p-chroot.c create mode 100644 hw/9pfs/virtio-9p-chroot.h -- 1.7.5.1 Reviewed-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
Re: [Qemu-devel] [PATCH 1/3] Fix chrdev return value conversion
Am 23.07.2011 23:24, schrieb Blue Swirl: 6e1db57b2ac9025c2443c665a0d9e78748637b26 didn't convert brlapi or win32 chrdevs, breaking build for those. Fix by converting the chrdevs. Signed-off-by: Blue Swirl blauwir...@gmail.com Sorry, I should have run a mingw build after touching this code. Acked-by: Kevin Wolf kw...@redhat.com
Re: [Qemu-devel] [PATCH] qemu-char: fix the commit qemu-char: Print strerror message on failure
Oh, Blue Swirl did it, please ignore this.
--
SUN OF A BEACH
On Mon, Jul 25, 2011 at 16:16, TeLeMan gele...@gmail.com wrote:
The commit 6e1db57b2ac9025c2443c665a0d9e78748637b26 missed patching
qemu_chr_open_win_file().
Signed-off-by: TeLeMan gele...@gmail.com
---
qemu-char.c | 8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index dcf7065..ea7abfe 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1782,7 +1782,7 @@ static int qemu_chr_open_win_pipe(QemuOpts *opts,
CharDriverState **_chr)
return 0;
}
-static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
+static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **_chr)
{
CharDriverState *chr;
WinCharState *s;
@@ -1793,12 +1793,14 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE
fd_out)
chr-opaque = s;
chr-chr_write = win_chr_write;
qemu_chr_generic_open(chr);
- return chr;
+
+ *_chr = chr;
+ return 0;
}
static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **_chr)
{
- return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr);
+ return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), _chr);
}
static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr)
--
1.7.3.1.msysgit.0
[Qemu-devel] [PATCH] monitor: fix build breakage with --disable-vnc
The breakage was introduced by the commit
13661089810d3e59931f3e80d7cb541b99af7071
Signed-off-by: TeLeMan gele...@gmail.com
---
monitor.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/monitor.c b/monitor.c
index 92cdd05..52ae5f2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1200,10 +1200,12 @@ static int add_graphics_client(Monitor *mon, const
QDict *qdict, QObject **ret_d
}
qerror_report(QERR_ADD_CLIENT_FAILED);
return -1;
+#ifdef CONFIG_VNC
} else if (strcmp(protocol, vnc) == 0) {
int fd = monitor_get_fd(mon, fdname);
vnc_display_add_client(NULL, fd, skipauth);
return 0;
+#endif
} else if ((s = qemu_chr_find(protocol)) != NULL) {
int fd = monitor_get_fd(mon, fdname);
if (qemu_chr_add_client(s, fd) 0) {
--
1.7.3.1.msysgit.0
Re: [Qemu-devel] [PATCH] user: Restore debug usage message for '-d ?' in user mode emulation
Ping? Since this is a regression in our command line handling
I think it should also go into 0.15...
thanks
-- PMM
On 18 July 2011 11:44, Peter Maydell peter.mayd...@linaro.org wrote:
The code which prints the debug usage message on '-d ?' for *-user
has to come before the check for not enough arguments, so that
qemu-foo -d ? prints the list of possible debug log items rather than
the generic usage message. (This was inadvertently broken in commit
c235d73.)
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
NB that I've tested the linux-user part of this fix but don't have access to
bsd/darwin to test those files; however the change is identical for all three
files so it should be OK...
bsd-user/main.c | 8 +---
darwin-user/main.c | 8 +---
linux-user/main.c | 11 ++-
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 6018a41..a63b877 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -856,9 +856,6 @@ int main(int argc, char **argv)
usage();
}
}
- if (optind = argc)
- usage();
- filename = argv[optind];
/* init debug */
cpu_set_log_filename(log_file);
@@ -877,6 +874,11 @@ int main(int argc, char **argv)
cpu_set_log(mask);
}
+ if (optind = argc) {
+ usage();
+ }
+ filename = argv[optind];
+
/* Zero out regs */
memset(regs, 0, sizeof(struct target_pt_regs));
diff --git a/darwin-user/main.c b/darwin-user/main.c
index 35196a1..72307ad 100644
--- a/darwin-user/main.c
+++ b/darwin-user/main.c
@@ -809,9 +809,6 @@ int main(int argc, char **argv)
usage();
}
}
- if (optind = argc)
- usage();
- filename = argv[optind];
/* init debug */
cpu_set_log_filename(log_file);
@@ -830,6 +827,11 @@ int main(int argc, char **argv)
cpu_set_log(mask);
}
+ if (optind = argc) {
+ usage();
+ }
+ filename = argv[optind];
+
/* Zero out regs */
memset(regs, 0, sizeof(struct target_pt_regs));
diff --git a/linux-user/main.c b/linux-user/main.c
index 289054b..8976b60 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3019,11 +3019,6 @@ int main(int argc, char **argv, char **envp)
usage();
}
}
- if (optind = argc)
- usage();
- filename = argv[optind];
- exec_path = argv[optind];
-
/* init debug */
cpu_set_log_filename(log_file);
if (log_mask) {
@@ -3041,6 +3036,12 @@ int main(int argc, char **argv, char **envp)
cpu_set_log(mask);
}
+ if (optind = argc) {
+ usage();
+ }
+ filename = argv[optind];
+ exec_path = argv[optind];
+
/* Zero out regs */
memset(regs, 0, sizeof(struct target_pt_regs));
--
1.7.1
Re: [Qemu-devel] [PATCH 05/28] PPC: Set MPIC IDE for IPI to 0
On 07/23/2011 12:49 PM, Alexander Graf wrote:
@@ -1304,6 +1304,10 @@ static void mpic_reset (void *opaque)
mpp-src[i].ipvp = 0x8080;
mpp-src[i].ide = 0x0001;
}
+/* Set IDE for IPIs to 0 so we don't get spurious interrupts */
+for (i = mpp-irq_ipi0; i MAX_IPI; i++) {
I suppose you meant i mpp-irq_ipi0 + MAX_IPI in that loop condition right? ;)
+mpp-src[i].ide = 0;
+}
/* Initialise IRQ destinations */
for (i = 0; i MAX_CPU; i++) {
mpp-dst[i].pctp = 0x000F;
Re: [Qemu-devel] [PATCH 05/28] PPC: Set MPIC IDE for IPI to 0
On 25.07.2011, at 10:46, Elie Richa wrote:
On 07/23/2011 12:49 PM, Alexander Graf wrote:
@@ -1304,6 +1304,10 @@ static void mpic_reset (void *opaque)
mpp-src[i].ipvp = 0x8080;
mpp-src[i].ide = 0x0001;
}
+/* Set IDE for IPIs to 0 so we don't get spurious interrupts */
+for (i = mpp-irq_ipi0; i MAX_IPI; i++) {
I suppose you meant i mpp-irq_ipi0 + MAX_IPI in that loop condition right?
;)
Ouch. How did that happen? :)
Alex
[Qemu-devel] [PATCH] Introduce QEMU_NEW()
qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity a...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type + void qemu_mutex_lock_iothread(void); void qemu_mutex_unlock_iothread(void); -- 1.7.5.3
Re: [Qemu-devel] coroutines and block I/O considerations
On 07/19/2011 12:57 PM, Stefan Hajnoczi wrote: From what I understand committed on Windows means that physical pages have been allocated and pagefile space has been set aside: http://msdn.microsoft.com/en-us/library/ms810627.aspx Yes, memory that is reserved on Windows is just a contiguous part of the address space that is set aside, like MAP_NORESERVE under Linux. Memory that is committed is really allocated. The question is how can we get the same effect on Windows and does the current Fibers implementation not already work? Windows thread and fiber stacks have both a reserved and a committed part. The dwStackSize argument to CreateFiber indeed represents _committed_ stack size, so we're now committing 4 MB of stack per fiber. The maximum size that the stack can grow to is set to the (per-executable) default. If you want to specify both the reserved and committed stack sizes, you can do that with CreateFiberEx. http://msdn.microsoft.com/en-us/library/ms682406%28v=vs.85%29.aspx 4 MB is quite a lot of address space anyway to waste for a thread. A coroutine should not need that much, even on Linux. I think for Windows 64 KB of initial stack size and 1 MB of maximum size should do (for Linux it would 1 MB overall). Paolo
Re: [Qemu-devel] [PATCH] fix network interface tap backend
On 07/23/11 18:17, Anthony Liguori wrote:
On 06/17/2011 03:56 AM, Christoph Egger wrote:
Fix network interface tap backend work on NetBSD.
It uses an ioctl to get the tap name.
From Manuel Bouyerbou...@netbsd.org
Signed-off-by: Christoph Eggerchristoph.eg...@amd.com
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 2f3efde..577aafe 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -28,6 +28,8 @@
#include qemu-error.h
#ifdef __NetBSD__
+#includesys/ioctl.h
Your mailer munged this patch.
... or by the MS Exchange Server.
Resending the patch as attachment, the only one
way I have that works for everyone. Sorry.
Fix network interface tap backend work on NetBSD.
It uses an ioctl to get the tap name.
From Manuel Bouyerbou...@netbsd.org
Signed-off-by: Christoph Eggerchristoph.eg...@amd.com
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 2f3efde..577aafe 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -28,6 +28,8 @@
#include qemu-error.h
#ifdef __NetBSD__
+#include sys/ioctl.h
+#include net/if.h
#include net/if_tap.h
#endif
@@ -40,8 +42,12 @@
int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int
vnet_hdr_required)
{
int fd;
+#ifdef TAPGIFNAME
+struct ifreq ifr;
+#else
char *dev;
struct stat s;
+#endif
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__OpenBSD__)
/* if no ifname is given, always start the search from tap0/tun0. */
@@ -77,14 +83,30 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
int vnet_hdr_required
#else
TFR(fd = open(/dev/tap, O_RDWR));
if (fd 0) {
-fprintf(stderr, warning: could not open /dev/tap: no virtual network
emulation\n);
+fprintf(stderr,
+warning: could not open /dev/tap: no virtual network emulation:
%s\n,
+strerror(errno));
return -1;
}
#endif
-fstat(fd, s);
+#ifdef TAPGIFNAME
+if (ioctl(fd, TAPGIFNAME, (void *)ifr) 0) {
+fprintf(stderr, warning: could not get tap name: %s\n,
+strerror(errno));
+return -1;
+}
+pstrcpy(ifname, ifname_size, ifr.ifr_name);
+#else
+if (fstat(fd, s) 0) {
+fprintf(stderr,
+warning: could not stat /dev/tap: no virtual network emulation:
%s\n,
+strerror(errno));
+return -1;
+}
dev = devname(s.st_rdev, S_IFCHR);
pstrcpy(ifname, ifname_size, dev);
+#endif
if (*vnet_hdr) {
/* BSD doesn't have IFF_VNET_HDR */
Re: [Qemu-devel] [PATCH] use mmap to allocate execute memory
On 07/23/11 18:17, Anthony Liguori wrote:
On 06/17/2011 05:11 AM, Christoph Egger wrote:
Use mmap to allocate executable memory on NetBSD as well.
From: Tobias Nygrent...@netbsd.org
Signed-off-by: Christoph Eggerchristoph.eg...@amd.com
diff --git a/exec.c b/exec.c
index 09928a3..1954a1c 100644
--- a/exec.c
+++ b/exec.c
@@ -520,7 +520,8 @@ static void code_gen_alloc(unsigned long tb_size)
}
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) \
- || defined(__DragonFly__) || defined(__OpenBSD__)
Your mailer munged this patch.
... or by the MS Exchange Server.
Resending the patch as attachment, the only one
way I have that works for everyone. Sorry.
Use mmap to allocate executable memory on NetBSD as well.
From: Tobias Nygren t...@netbsd.org
Signed-off-by: Christoph Egger christoph.eg...@amd.com
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
diff --git a/exec.c b/exec.c
index 09928a3..1954a1c 100644
--- a/exec.c
+++ b/exec.c
@@ -520,7 +520,8 @@ static void code_gen_alloc(unsigned long tb_size)
}
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) \
-|| defined(__DragonFly__) || defined(__OpenBSD__)
+|| defined(__DragonFly__) || defined(__OpenBSD__) \
+|| defined(__NetBSD__)
{
int flags;
void *addr = NULL;
Re: [Qemu-devel] Boot order problem
On Mon, Jul 25, 2011 at 04:07:12PM +0900, Minoru Usui wrote: Hi, Gleb Thank you for your reply. On Sun, 24 Jul 2011 09:30:49 +0300 Gleb Natapov g...@redhat.com wrote: On Fri, Jul 22, 2011 at 09:51:16AM +0900, Minoru Usui wrote: Hi, everyone I'm in trouble about boot order of VM. If anyone know cause of this problem, please let me know. The cause of the problem is the design. booindex and -boot only modifies the order in which bios will search for bootable device. It does not exclude devices from a boot device list. Hmm, this design is little bit strange to me. We can specify BIOS boot order on baremetal BIOS. In this case, we won't expected boots up from not specified boot device, generally. My bios has 4 boot device slots filled by factory defaults. If I modify the first one (like -boot c does) it leaves other slots with their previous values. If first slot is not bootable it will fall back to next one. Isn't it intuitive that -boot and bootindex option can specify not only boot order, but also boot device list? What is the use case? If this is what it will do you will be required to specify full boot order list each time you start a VM. Much more common scenario is to modify boot order priority by moving some devices at the top. It is possible to add an option that will exclude device from a boot list. Need some work both in qemu and seabios. On following environment, I tried to boot from IDE CD-ROM device without inserting any bootable media, which is expected to fail, but VM was booting up from virtio HDD which was not specified as bootable device. * host : RHEL6.1(x86_64) guest: RHEL6.1(x86_64) * VM has IDE CD-ROM and virtio HDD. * There is no bootable media in IDE CD-ROM. * RHEL6.1 is installed in virtio HDD * Only IDE CD-ROM was spcified as bootable device. * XML configuration of libvirt is below. I tested boot dev and boot order setting, but both are booting up from virtio HDD. --- [boot dev setting version] os type arch='x86_64' machine='rhel6.1.0'hvm/type boot dev='cdrom'/ bootmenu enable='no'/ /os [boot order setting version] disk type='file' device='cdrom' driver name='qemu' type='raw'/ target dev='hdc' bus='ide'/ boot order='1'/ readonly/ address type='drive' controller='0' bus='1' unit='0'/ /disk --- I installed latest qemu-kvm to /usr/local/qemu, and replaced /usr/libexec/qemu-kvm to /user/local/qemu/bin/qemu-system-x86_64, but it was booting up from virtio HDD. On RHEL6.0 host, I tested boot dev setting version, VM didn't boot up from virtio HDD. it cannot boot up from CD-ROM. (expected behaviour) This is not expected behaviour. Expected behaviour is VM boots from HDD. The only way I can explain behaviour you describe above is that the bios you are using for RHEL6.0 rpm does not support booting from virtio HDD. You can test this but making HDD to be ide and retry your test. I changed virtio HDD to IDE HDD, and retry my test on RHEL6.0(seabios-0.5.1-3.el6.x86_64), but VM didn't boot up from IDE HDD.(same result) Hmm. Yes, I can reproduce this with rhel60 bios. rhel6.1 works as expected though. Need to check why rhel60 bios works like this. Of cource, if I specified IDE HDD first (not IDE CD-ROM), it can boot up. Am I somthing wrong? [libvirt setting] disk type='block' device='disk' driver name='qemu' type='raw' cache='none'/ source dev='/dev/sdb5'/ target dev='hda' bus='ide'/ address type='drive' controller='0' bus='1' unit='1'/ /disk disk type='file' device='cdrom' driver name='qemu' type='raw'/ target dev='hdc' bus='ide'/ readonly/ alias name='ide0-1-0'/ address type='drive' controller='0' bus='1' unit='0'/ /disk controller type='ide' index='0' alias name='ide0'/ address type='pci' domain='0x' bus='0x00' slot='0x01' function='0x1'/ /controller [qemu-kvm option by ps command] qemu 6685 1 21 09:42 ?00:00:03 /usr/libexec/qemu-kvm -S -M rhel6.0.0 -enable-kvm -m 2048 -smp 2,sockets=2,cores=1,threads=1 -name RHEL5.5-x86_64-disk1 -uuid b05c3fa6-a52d-1f15-8412-00a876a0c672 -nodefconfig -nodefaults -chardev socket,id=monitor,path=/var/lib/libvirt/qemu/RHEL5.5-x86_64-disk1.monitor,server,nowait -mon chardev=monitor,mode=control -rtc base=utc -boot d -drive file=/dev/sdb5,if=none,id=drive-ide0-1-1,format=raw,cache=none -device ide-drive,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 -drive if=none,media=cdrom,id=drive-ide0-1-0,readonly=on,format=raw -device
Re: [Qemu-devel] Boot order problem
On Mon, Jul 25, 2011 at 04:44:58PM +0900, Minoru Usui wrote: On Sun, 24 Jul 2011 09:30:49 +0300 Gleb Natapov g...@redhat.com wrote: [skip] I tested another one about boot order case on RHEL6.1, and I also faced another problem. VM has two virtio HDD. HDD1 is installed RHEL6.1, HDD2 is empty. I specified boot order to HDD1:1, HDD2:2, VM booted up from HDD1, but boot order HDD1:2, HDD2:1 case, VM couldn't boot up from HDD2. (It searched CD-ROM, NIC(gPXE), and finally stopped booting.) That's BIOS specification limitation. BIOS can't fall back from one HDD to another, so only HDD with lowest priority among all HDDs will be tried. It seems seabios searches only 1 device per device list(HDD, CD-ROM, NET, FLOPPY). Is it true? No, it searches only one HDD. Other devices do not have this limitation IIRC. boot order can specify per device, so shouldn't seabios search all device, even if it specifies multiple device per device list? -- Minoru Usui u...@mxm.nes.nec.co.jp -- Gleb.
Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
+static void piix4_pm_machine_ready(struct Notifier* n)
+{
+ PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
DO_UPCAST()? I assume we have it for a reason.
NIH is the reason we have it.
DO_UPCAST checks that the offset of the field is zero:
#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
char __attribute__((unused)) offset_must_be_zero[ \
-offsetof(type, field)]; \
container_of(dev, type, field);}))
#else
#define DO_UPCAST(type, field, dev) container_of(dev, type, field)
#endif
This isn't the case here, we really want container_of.
BTW, DO_UPCAST actually is used to do a _down_cast (base to derived). A
compile-time checked upcast (derived to base) could be done like this:
#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
char __attribute__((unused)) offset_must_be_zero[ \
-offsetof(type, field)]; \
char __attribute__((unused)) type_matches = \
type_check(type, __typeof__(dev));
(dev)-field);}))
#else
#define DO_UPCAST(type, field, dev) (dev)-field
#endif
Paolo
[Qemu-devel] ci-joint votre offre pour
La pose de panneaux photovoltaïques vous permet de devenir producteur d’énergie pour votre consommation personnelle et de revendre votre sur plus d’électricité à EDF* pendant 20 ans vous possédez une maison, un entrepôt, un hangar, un bâtiment industriel avec une toiture en pente. Aucune limite de surface: de 40, 100, 1000 m2 ou même 5 000 m2 tout est possible, les revenus, nets, de ces installations sont énormes, 800€, 2000€, 5000€, 15 000€ et bien plus facturés à EDF* chaque année. De plus vous pouvez bénéficier d’un crédit d’impôts. \*sous réserve d'acceptation du dossier et du raccordement par EDF*/ Contactez-nous au 0 2 4 0 5 7 0 1 1…3 Comment choisir la source d’énergie la plus économique pour votre confort. Climatisation réversible et chauffage offrent air frais ou chaleur selon la température. L'installation de climatisation réversible est rapide. Vous pouvez bénéficier d'un crédit d'impôt sur votre installation. Contactez-nous au 0 2 4 0 5 7 0 1 1…3 Pour ne plus recevoir notre newsletter contactez-nous annulation- prod @ sfr .fr
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Alex
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, 2011-07-25 at 11:32 +0200, Alexander Graf wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. It'll warn when you do silly things such as: struct some_struct *k; k = qemu_malloc(sizeof(k)); -- Sasha.
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Please add qemu capabilities, so we can reasonably detect what an unknown qemu binary supports and so we don't need to do endless parsing of the -help output and guesswork. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 11:37, Sasha Levin wrote: On Mon, 2011-07-25 at 11:32 +0200, Alexander Graf wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. It'll warn when you do silly things such as: struct some_struct *k; k = qemu_malloc(sizeof(k)); Hm - is there any way to get this without adding upper case C++'ish macros? Alex
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25 July 2011 10:32, Alexander Graf ag...@suse.de wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Yeah, while we're writing in C we should just stick to the C-like APIs, it's less confusing IMHO than wrapping it up in something else. I assume Anthony's new object model stuff will have a create me a new foo object API anyway, so QEMU_NEW() is possibly a bit of a namespace grab. -- PMM
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 12:43 PM, Alexander Graf wrote: Hm - is there any way to get this without adding upper case C++'ish macros? Switch to C++. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 12:48 PM, Peter Maydell wrote: On 25 July 2011 10:32, Alexander Grafag...@suse.de wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Yeah, while we're writing in C we should just stick to the C-like APIs, it's less confusing IMHO than wrapping it up in something else. That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I assume Anthony's new object model stuff will have a create me a new foo object API anyway, so QEMU_NEW() is possibly a bit of a namespace grab. Anthony's stuff is at a much higher level, hopefully he'll come back to the ground one day. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 11:52, Avi Kivity wrote: On 07/25/2011 12:48 PM, Peter Maydell wrote: On 25 July 2011 10:32, Alexander Grafag...@suse.de wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Yeah, while we're writing in C we should just stick to the C-like APIs, it's less confusing IMHO than wrapping it up in something else. That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? I sympathize with Peter on the rationale that keeping interfaces aligned with how C APIs usually look like helps making the code more readable. Alex
Re: [Qemu-devel] coroutines and block I/O considerations
On Mon, Jul 25, 2011 at 9:56 AM, Paolo Bonzini pbonz...@redhat.com wrote: On 07/19/2011 12:57 PM, Stefan Hajnoczi wrote: From what I understand committed on Windows means that physical pages have been allocated and pagefile space has been set aside: http://msdn.microsoft.com/en-us/library/ms810627.aspx Yes, memory that is reserved on Windows is just a contiguous part of the address space that is set aside, like MAP_NORESERVE under Linux. Memory that is committed is really allocated. The question is how can we get the same effect on Windows and does the current Fibers implementation not already work? Windows thread and fiber stacks have both a reserved and a committed part. The dwStackSize argument to CreateFiber indeed represents _committed_ stack size, so we're now committing 4 MB of stack per fiber. The maximum size that the stack can grow to is set to the (per-executable) default. If you want to specify both the reserved and committed stack sizes, you can do that with CreateFiberEx. http://msdn.microsoft.com/en-us/library/ms682406%28v=vs.85%29.aspx 4 MB is quite a lot of address space anyway to waste for a thread. A coroutine should not need that much, even on Linux. I think for Windows 64 KB of initial stack size and 1 MB of maximum size should do (for Linux it would 1 MB overall). I agree, let's make sure not to commit all this memory upfront. Stefan
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Alex
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@redhat.com wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity a...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); If you ever change the name of the type you have to search-replace these instances. The idomatic C way works well, I don't see a reason to use QEMU_NEW(). Stefan
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 01:04 PM, Alexander Graf wrote: On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC] ppc: qdev-ify CPU creation
On 24.07.2011, at 21:08, Hervé Poussineau wrote:
Alexander Graf a écrit :
On 21.12.2010, at 21:01, Andreas Färber wrote:
From: Hervé Poussineau hpous...@reactos.org
v1:
* Coding style fixes.
Signed-off-by: Hervé Poussineau hpous...@reactos.org
Cc: Alexander Graf ag...@suse.de
Signed-off-by: Andreas Färber andreas.faer...@web.de
---
Hello Alex,
Seeing the discussions about Leon3, is this the way to go for ppc? Is
ppc.[hc] right?
The unconditional use of 6xx looks suspicious to me, no?
Should we rename cpu_device_irq_request() to cpu_device_irq_request_6xx()?
Regards,
Andreas
hw/ppc.c| 75
+++
hw/ppc.h|2 +
target-ppc/cpu.h|1 +
target-ppc/helper.c | 21 +++---
4 files changed, 94 insertions(+), 5 deletions(-)
diff --git a/hw/ppc.c b/hw/ppc.c
index 968aec1..0927326 100644
--- a/hw/ppc.c
+++ b/hw/ppc.c
@@ -30,6 +30,8 @@
#include loader.h
#include kvm.h
#include kvm_ppc.h
+#include hw/qdev.h
+#include hw/sysbus.h
//#define PPC_DEBUG_IRQ
//#define PPC_DEBUG_TB
@@ -1286,3 +1288,76 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t
NVRAM_size,
return 0;
}
+
+DeviceState *cpu_ppc_create_simple(const char *cpu_model)
+{
+DeviceState *dev;
+
+dev = qdev_create(NULL, cpu-ppc);
+if (!dev) {
+return NULL;
+}
+qdev_prop_set_string(dev, model, qemu_strdup(cpu_model));
+if (qdev_init(dev) 0) {
+return NULL;
+}
+return dev;
+}
+
+typedef struct CPUPPC {
+SysBusDevice busdev;
I'm not sure we really want CPUs on the sysbus. They belong to their own CPU
bus. Basically, I think we should try to model our bus topology so that it
reflects the bus topology in the device tree 1:1. Then generating a device
tree from the bug information and some device specific callbacks would be
possible.
CPUs don't need a bus with specific capabilities, so I used the most simple
existing one, ie SysBus.
Yeah, that's nice and simple from the implementation POV, but I have this funny
idea that we might be able to generate a device tree from qemu's bus topology.
And at that point we'll have to have a separate CPU bus.
+char *model;
+CPUPPCState state;
+} CPUPPC;
+
+static void cpu_device_irq_request(void *opaque, int pin, int level)
+{
+CPUPPC* cpu = opaque;
+CPUPPCState* env = cpu-state;
+ppc6xx_set_irq(env, pin, level);
+}
+
+static int cpu_device_init(SysBusDevice *dev)
+{
+CPUPPC* cpu = FROM_SYSBUS(CPUPPC, dev);
+CPUPPCState* env = cpu-state;
+
+if (cpu_ppc_init_inplace(env, cpu-model) 0) {
+return -1;
+}
+
+if (env-flags POWERPC_FLAG_RTC_CLK) {
Where does this flag suddenly come from? Is this related to qdev'ification?
It's not related to qdev'ification. It is already set in CPU definitions
since a long time.
Hrm. So where is it usually read out then? Shouldn't that code go away?
+/* POWER / PowerPC 601 RTC clock frequency is 7.8125 MHz */
+cpu_ppc_tb_init(env, 7812500UL);
+} else {
+/* Set time-base frequency to 100 Mhz */
+cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
Usually we have a TB frequency of 400Mhz in our board/devtrees hardcoded in
the TCG case. How about a qdev property that the creator could just modify
to its needs? We won't need the special 601 flag then either - just move
that into the PREP code.
This code has been extracted from ppc_prep.c ; a qdev property is also fine.
Yes, please :)
+}
+
+qdev_init_gpio_in(dev-qdev, cpu_device_irq_request, PPC6xx_INPUT_NB);
+return 0;
+}
+
+static void cpu_device_reset(DeviceState *d)
+{
+CPUPPC *s = FROM_SYSBUS(CPUPPC, sysbus_from_qdev(d));
+cpu_reset(s-state);
+}
+
+static SysBusDeviceInfo cpu_device_info = {
+.qdev.name = cpu-ppc,
+.qdev.size = sizeof(CPUPPC),
+.qdev.reset = cpu_device_reset,
+.init = cpu_device_init,
+.qdev.props = (Property[]) {
+DEFINE_PROP_STRING(model, CPUPPC, model),
+DEFINE_PROP_END_OF_LIST(),
+},
+};
+
+static void ppc_register_devices(void)
+{
+sysbus_register_withprop(cpu_device_info);
+}
+
+device_init(ppc_register_devices)
diff --git a/hw/ppc.h b/hw/ppc.h
index 34f54cf..ae8dd97 100644
--- a/hw/ppc.h
+++ b/hw/ppc.h
@@ -37,6 +37,8 @@ void ppce500_irq_init (CPUState *env);
void ppc6xx_irq_init (CPUState *env);
void ppc970_irq_init (CPUState *env);
+DeviceState *cpu_ppc_create_simple(const char *cpu_model);
+
/* PPC machines for OpenBIOS */
enum {
ARCH_PREP = 0,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index deb8d7c..0f56d45 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -721,6 +721,7 @@ struct mmu_ctx_t {
/*/
CPUPPCState *cpu_ppc_init (const
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 01:06 PM, Stefan Hajnoczi wrote: char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); If you ever change the name of the type you have to search-replace these instances. The idomatic C way works well, I don't see a reason to use QEMU_NEW(). It works as long as you don't make any mistakes: f = qemu_malloc(sizeof(*g)); f = qemu_malloc(sizeof(f)); qemu_malloc() returns a void pointer, these are poisonous. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile: fix out-of-tree builds
On Thu, Jul 21, 2011 at 5:41 AM, Alexandre Raymond cerb...@gmail.com wrote: This patch fixes a minor bugs which prevented QEMU from being built out of tree. Signed-off-by: Alexandre Raymond cerb...@gmail.com --- Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I don't normally use --source-path but it still seems broken to me after applying your patch? $ cd /tmp; mkdir out; cd out $ ~/qemu/configure --source-path=$HOME/qemu $ make GEN config-all-devices.mak cat: i386-softmmu/config-devices.mak: No such file or directory cat: x86_64-softmmu/config-devices.mak: No such file or directory cat: alpha-softmmu/config-devices.mak: No such file or directory Stefan
Re: [Qemu-devel] [Qemu-trivial] [PATCH] vhost build fix for i386
On Mon, Jul 11, 2011 at 02:57:43PM +0200, Wolfgang Mauerer wrote: vhost.c uses __sync_fetch_and_and(), which is only available for -march=i486 and above (see https://bugzilla.redhat.com/show_bug.cgi?id=624279). Signed-off-by: Wolfgang Mauerer wolfgang.maue...@siemens.com --- configure | 23 +++ 1 files changed, 23 insertions(+), 0 deletions(-) Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 12:09, Avi Kivity wrote: On 07/25/2011 01:04 PM, Alexander Graf wrote: On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). Right. But then we really do need a check in checkpatch.pl to see if someone's using qemu_new for simple structs. I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth. Blue, any disagreement on adding qemu_new() as a macro? Something used that often in upper case would seriously disturb the reading flow :) Alex
Re: [Qemu-devel] [Qemu-trivial] [PATCH] sh4: Fix potential crash in debug code
On Wed, Jul 20, 2011 at 08:56:35PM +0200, Stefan Weil wrote: cppcheck reports this error: qemu/hw/sh_intc.c:390: error: Possible null pointer dereference: s - otherwise it is redundant to check if s is null at line 385 If s were NULL, the printf() statement would crash. Setting braces fixes this bug. Signed-off-by: Stefan Weil w...@mail.berlios.de --- hw/sh_intc.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) Aurelien Jarno is listed as active maintainer for this code. Patches should go through him. Stefan
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@redhat.com wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity a...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); Maybe we should allow this and make it the usual pattern: f = qemu_new(typeof(*f)); It's gcc specific, but we already don't care about portability to other compilers in more places. On the other hand, how many bugs did we have recently that were caused by a wrong sizeof for qemu_malloc? As far as I can say, there's no real reason to do it. I think it's the same kind of discussion as with forbidding qemu_malloc(0) (except that this time it just won't improve things much instead of being really stupid). Kevin
Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] Makefile: Minor cscope fixups
On Wed, Jul 20, 2011 at 11:12:15PM -0400, Alexandre Raymond wrote: Changes since v1: -Use SRC_PATH instead of PWD -Create cscope symbols for assembly files in addition to .c/.h files. -Create cscope database with full path instead of relative path so cscope can be used with CSCOPE_DB in any directory. Signed-off-by: Alexandre Raymond cerb...@gmail.com --- Makefile |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 2011-07-25 11:41, Richard W.M. Jones wrote: On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Yes, pc is the right default. Other arch have other defaults. Please add qemu capabilities, so we can reasonably detect what an unknown qemu binary supports and so we don't need to do endless parsing of the -help output and guesswork. This syntax was not yet released (but will be with 0.15, so I was pushing this). Therefore, nothing was officially broken by this patch. I'm sorry if you may have released any libguestfs with the transient syntax, but my patches were waiting quite a while for being merged since the introduction of -machine. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, Jul 25, 2011 at 11:25 AM, Kevin Wolf kw...@redhat.com wrote: Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@redhat.com wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity a...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); Maybe we should allow this and make it the usual pattern: f = qemu_new(typeof(*f)); It's gcc specific, but we already don't care about portability to other compilers in more places. On the other hand, how many bugs did we have recently that were caused by a wrong sizeof for qemu_malloc? As far as I can say, there's no real reason to do it. I think it's the same kind of discussion as with forbidding qemu_malloc(0) (except that this time it just won't improve things much instead of being really stupid). Totally agree. In theory you can add stuff on top to prevent known bad uses but in practice it's not worth obfuscating the code. Stefan
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On Mon, Jul 25, 2011 at 12:33:01PM +0200, Jan Kiszka wrote: On 2011-07-25 11:41, Richard W.M. Jones wrote: On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Yes, pc is the right default. Other arch have other defaults. So what you're saying is we have to parse qemu -machine \? output by looking for the string '(default)'? eg: $ ./arm-softmmu/qemu-system-arm -machine \?|fgrep '(default)' integratorcp ARM Integrator/CP (ARM926EJ-S) (default) $ ./i386-softmmu/qemu -machine \?|fgrep '(default)' pc-0.14Standard PC (default) Please add qemu capabilities, so we can reasonably detect what an unknown qemu binary supports and so we don't need to do endless parsing of the -help output and guesswork. This syntax was not yet released (but will be with 0.15, so I was pushing this). Therefore, nothing was officially broken by this patch. I'm sorry if you may have released any libguestfs with the transient syntax, but my patches were waiting quite a while for being merged since the introduction of -machine. That's an excuse, not a practical solution. We have to be able to work with any qemu. eg. the qemu in current Fedora Rawhide which supports only -machine accel=, or qemu in other distros which are also branched from arbitrary git releases, or qemu that people compile themselves. Parsing -help output and guesswork isn't scalable, and this is not exactly the first time that people have complained about this. (Yes, libvirt and libguestfs do allow callers to mechanically query their respective APIs for capabilities.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On Mon, 25 Jul 2011, Alexander Graf wrote: On 25.07.2011, at 12:09, Avi Kivity wrote: On 07/25/2011 01:04 PM, Alexander Graf wrote: On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). Right. But then we really do need a check in checkpatch.pl to see if someone's using qemu_new for simple structs. I agree qemu_new() will be nicer, but that will have to wait until Blue is several light-days away from Earth. Blue, any disagreement on adding qemu_new() as a macro? Something used that often in upper case would seriously disturb the reading flow :) So do not use it then, macros should be uppercase. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Avi Kivity a...@redhat.com writes: On 07/25/2011 01:04 PM, Alexander Graf wrote: On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). Byte array: add the obvious type-safe allocator for a variable-sized array T[N], then use it with unsigned char for T. In fact, I find QEMU_NEW() pretty pointless without a buddy for arrays. Still not covered: allocating a struct with a variable-size array as final member. I guess a solution for that can be found if we care enough. [...]
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 2011-07-25 12:45, Richard W.M. Jones wrote: On Mon, Jul 25, 2011 at 12:33:01PM +0200, Jan Kiszka wrote: On 2011-07-25 11:41, Richard W.M. Jones wrote: On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Yes, pc is the right default. Other arch have other defaults. So what you're saying is we have to parse qemu -machine \? output by looking for the string '(default)'? eg: $ ./arm-softmmu/qemu-system-arm -machine \?|fgrep '(default)' integratorcp ARM Integrator/CP (ARM926EJ-S) (default) $ ./i386-softmmu/qemu -machine \?|fgrep '(default)' pc-0.14Standard PC (default) I understand, this is clumsy. Will see if we can do better. Please add qemu capabilities, so we can reasonably detect what an unknown qemu binary supports and so we don't need to do endless parsing of the -help output and guesswork. This syntax was not yet released (but will be with 0.15, so I was pushing this). Therefore, nothing was officially broken by this patch. I'm sorry if you may have released any libguestfs with the transient syntax, but my patches were waiting quite a while for being merged since the introduction of -machine. That's an excuse, not a practical solution. We have to be able to work with any qemu. eg. the qemu in current Fedora Rawhide which supports only -machine accel=, or qemu in other distros which are also branched from arbitrary git releases, or qemu that people compile themselves. In principle, this is first of all a Rawhide problem. Upstream really can't babysit every distro doing crazy things with arbitrary devel snapshots. These patches were public, and the maintainers had a fair chance to realize that the interface was not yet set in stone. Parsing -help output and guesswork isn't scalable, and this is not exactly the first time that people have complained about this. I agree. That's why we try hard to release stable interfaces and then maintain them. (Yes, libvirt and libguestfs do allow callers to mechanically query their respective APIs for capabilities.) Maybe Anthony's (Liguori) rework of the QEMU config interfaces will provide a better reflections, haven't checked. But for now you need to stick with this model, specifically when you want to maintain all the distro forks. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
Kevin Wolf kw...@redhat.com writes: Am 25.07.2011 12:06, schrieb Stefan Hajnoczi: On Mon, Jul 25, 2011 at 9:51 AM, Avi Kivity a...@redhat.com wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Signed-off-by: Avi Kivity a...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type Does this mean we need to duplicate the type name for each allocation? struct foo *f; ... f = qemu_malloc(sizeof(*f)); Becomes: struct foo *f; ... f = QEMU_NEW(struct foo); Maybe we should allow this and make it the usual pattern: f = qemu_new(typeof(*f)); It's gcc specific, but we already don't care about portability to other compilers in more places. On the other hand, how many bugs did we have recently that were caused by a wrong sizeof for qemu_malloc? As far as I can say, there's no real reason to do it. I think it's the same kind of discussion as with forbidding qemu_malloc(0) (except that this time it just won't improve things much instead of being really stupid). Side-stepping the stupid OMG malloc(0) is weird, therefore we must make qemu_malloc(0) differently weird controversy would be useful all by itself.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 25.07.2011, at 12:59, Markus Armbruster wrote: Avi Kivity a...@redhat.com writes: On 07/25/2011 01:04 PM, Alexander Graf wrote: On 25.07.2011, at 12:02, Avi Kivity wrote: On 07/25/2011 12:56 PM, Alexander Graf wrote: That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I agree that it keeps you from accidently malloc'ing a struct of pointer size. But couldn't we also just add this to checkpatch.pl? Better APIs trump better patch review. Only if you enforce them. The only sensible thing for QEMU_NEW (despite the general rule of upper case macros, I'd actually prefer this one to be lower case though since it's so often used) would be to remove qemu_malloc, declare malloc() as unusable and convert all users of qemu_malloc() to qemu_new(). Some qemu_mallocs() will remain (allocating a byte array or something variable sized). Byte array: add the obvious type-safe allocator for a variable-sized array T[N], then use it with unsigned char for T. In fact, I find QEMU_NEW() pretty pointless without a buddy for arrays. #define QEMU_NEW_MULTI(type, len) ((type *)(qemu_mallocz(sizeof(type) * len))) char *arr = QEMU_NEW_MULTI(char, 1024); Still not covered: allocating a struct with a variable-size array as final member. I guess a solution for that can be found if we care enough. Yeah, but at the end of the day I'd assume most of us know C and can just open code this all, no? Alex
Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model
Am 25.07.2011 03:44, schrieb Anthony Liguori: Hi, This series is the rough beginnings of the QEMU Object Model. This is basically qdev generalized on steroids. This series includes the core infrastructure, a strawman Device type, and the beginnings of the conversion of CharDriverState. This is in a rougher state than I would like it to be but I wanted to get the concepts on the list as soon as possible. My plan is to drop the Device parts from future versions of the series and just focus on backends to start with. Please note that this series has an awful lot of ramifications. Most of our current command line options would become deprecated, the build system will change significantly, and a lot of our QMP functions will become deprecated. It seems like a lot of change, but hopefully this series illustrates how we can do it very incrementally with value being added at each stage of the conversion. I haven't looked in much detail at it yet, but it has still the same problem I was talking about last week: Patches 17-21 don't actually convert existing code, but they add new code. This means that we can't review only the changes, but have to review the whole code. It also makes conflicts with patches modifying the old version hard to even notice. On another note, I'm not so sure if your renaming is really helpful. It doesn't matter that much with qemu-char because someone thought having the function pointers in CharDriverState was a good idea, but if you're consistent, the rename would go like this in the block layer: BlockDriverState - BlockDriver BlockDriver - BlockDriverClass IMHO, that's not very helpful, but just going to create confusion. We could probably discuss other parts of the terminology, too, but let's save the bikeshedding for later. Kevin
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
Jan Kiszka jan.kis...@siemens.com writes: On 2011-07-25 12:45, Richard W.M. Jones wrote: On Mon, Jul 25, 2011 at 12:33:01PM +0200, Jan Kiszka wrote: On 2011-07-25 11:41, Richard W.M. Jones wrote: On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Yes, pc is the right default. Other arch have other defaults. So what you're saying is we have to parse qemu -machine \? output by looking for the string '(default)'? eg: $ ./arm-softmmu/qemu-system-arm -machine \?|fgrep '(default)' integratorcp ARM Integrator/CP (ARM926EJ-S) (default) $ ./i386-softmmu/qemu -machine \?|fgrep '(default)' pc-0.14Standard PC (default) I understand, this is clumsy. Will see if we can do better. Is there a technical reason why type isn't optional with -machine? [...]
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 25 July 2011 11:45, Richard W.M. Jones rjo...@redhat.com wrote: So what you're saying is we have to parse qemu -machine \? output by looking for the string '(default)'? eg: $ ./arm-softmmu/qemu-system-arm -machine \?|fgrep '(default)' integratorcp ARM Integrator/CP (ARM926EJ-S) (default) For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. -- PMM
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 2011-07-25 13:39, Markus Armbruster wrote: Jan Kiszka jan.kis...@siemens.com writes: On 2011-07-25 12:45, Richard W.M. Jones wrote: On Mon, Jul 25, 2011 at 12:33:01PM +0200, Jan Kiszka wrote: On 2011-07-25 11:41, Richard W.M. Jones wrote: On Sat, Jul 23, 2011 at 12:38:37PM +0200, Jan Kiszka wrote: From: Jan Kiszka jan.kis...@siemens.com -machine somehow suggests that it selects the machine, but it doesn't. Fix that before this command is set in stone. Actually, -machine should supersede -M and allow to introduce arbitrary per-machine options to the command line. That will change the internal realization again, but we will be able to keep the user interface stable. This breaks libguestfs which was doing: qemu -machine accel=kvm:tcg ... We are not passing any -M option at all. We don't particularly care about the machine type since we're not that performance sensitive and we don't need to serialize the machine state. I have checked, and this works: qemu -machine pc,accel=kvm:tcg ... pc is the default, right? What about for other architectures? Yes, pc is the right default. Other arch have other defaults. So what you're saying is we have to parse qemu -machine \? output by looking for the string '(default)'? eg: $ ./arm-softmmu/qemu-system-arm -machine \?|fgrep '(default)' integratorcp ARM Integrator/CP (ARM926EJ-S) (default) $ ./i386-softmmu/qemu -machine \?|fgrep '(default)' pc-0.14Standard PC (default) I understand, this is clumsy. Will see if we can do better. Is there a technical reason why type isn't optional with -machine? [...] Maybe it's just the assert(!permit_abbrev || list-implied_opt_name); in qemu_opts_parse, but I haven't looked at all details (and all other users) yet. From -machine POV, I see no reason that prevents defaulting to some machine type if [type=]XXX is missing in the command line. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 25 July 2011 12:48, Peter Maydell peter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. (Consider also the possibility of eventually having a single qemu binary that supports multiple architectures -- that would make a 'default machine' definitely a bit odd.) -- PMM
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 03:51 AM, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Just use g_new() and g_new0() Regards, Anthony Liguori Signed-off-by: Avi Kivitya...@redhat.com --- This is part of my memory API patchset, but doesn't really belong there. qemu-common.h |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index ba55719..66effa3 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -186,6 +186,9 @@ void qemu_free(void *ptr); char *qemu_strdup(const char *str); char *qemu_strndup(const char *str, size_t size); +#define QEMU_NEW(type) ((type *)(qemu_malloc(sizeof(type +#define QEMU_NEWZ(type) ((type *)(qemu_mallocz(sizeof(type + void qemu_mutex_lock_iothread(void); void qemu_mutex_unlock_iothread(void);
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 2011-07-25 14:05, Peter Maydell wrote: On 25 July 2011 12:48, Peter Maydell peter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. Then you may want to drop is_default = 1 from integratorcp and prepare the main loop to face a NULL machine. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile: fix out-of-tree builds
On 07/25/2011 05:15 AM, Stefan Hajnoczi wrote: On Thu, Jul 21, 2011 at 5:41 AM, Alexandre Raymondcerb...@gmail.com wrote: This patch fixes a minor bugs which prevented QEMU from being built out of tree. Signed-off-by: Alexandre Raymondcerb...@gmail.com --- Makefile |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I don't normally use --source-path but it still seems broken to me after applying your patch? $ cd /tmp; mkdir out; cd out $ ~/qemu/configure --source-path=$HOME/qemu $ make GEN config-all-devices.mak cat: i386-softmmu/config-devices.mak: No such file or directory cat: x86_64-softmmu/config-devices.mak: No such file or directory cat: alpha-softmmu/config-devices.mak: No such file or directory Stefan Works okay for me with and without the patch if I do a `make distclean` in $HOME/qemu beforehand. Not sure what the trigger is for the breakage Alexandre is trying to address.
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 03:11 PM, Anthony Liguori wrote: On 07/25/2011 03:51 AM, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Just use g_new() and g_new0() These bypass qemu_malloc(). Are we okay with that? I suppose so, since many library functions can allocate memory and bypass qemu_malloc()? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 06:11 AM, Alexander Graf wrote: #define QEMU_NEW_MULTI(type, len) ((type *)(qemu_mallocz(sizeof(type) * len))) char *arr = QEMU_NEW_MULTI(char, 1024); Still not covered: allocating a struct with a variable-size array as final member. I guess a solution for that can be found if we care enough. Yeah, but at the end of the day I'd assume most of us know C and can just open code this all, no? While it's always fun to reinvent things, glib has already solved all of this and we're already dependent on it in the build: http://developer.gnome.org/glib/stable/glib-Memory-Allocation.html It also has fancy ways to hook memory allocation for debugging. Regards, Anthony Liguori Alex
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 07:18 AM, Avi Kivity wrote: On 07/25/2011 03:11 PM, Anthony Liguori wrote: On 07/25/2011 03:51 AM, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Just use g_new() and g_new0() These bypass qemu_malloc(). Are we okay with that? Yes. We can just make qemu_malloc use g_malloc. I suppose so, since many library functions can allocate memory and bypass qemu_malloc()? Right. Regards, Anthony Liguori
[Qemu-devel] [PATCH] Fix last sector write on sd card
When writing the last sector of an SD card using WRITE_MULTIPLE_BLOCK
QEmu throws an error saying that we've run off the end, and leaves
itself in the wrong state.
Tested on ARM Vexpress model.
Signed-off-by: Dr. David Alan Gilbert david.gilb...@linaro.org
---
Don't throw address error on last block, and leave in correct state.
diff --git a/hw/sd.c b/hw/sd.c
index cedfb20..219a0dd 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -1450,14 +1450,8 @@ void sd_write_data(SDState *sd, uint8_t value)
break;
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
-sd-data[sd-data_offset ++] = value;
-if (sd-data_offset = sd-blk_len) {
-/* TODO: Check CRC before committing */
-sd-state = sd_programming_state;
-BLK_WRITE_BLOCK(sd-data_start, sd-data_offset);
-sd-blk_written ++;
-sd-data_start += sd-blk_len;
-sd-data_offset = 0;
+if (sd-data_offset == 0) {
+/* Start of the block - lets check the address is valid */
if (sd-data_start + sd-blk_len sd-size) {
sd-card_status |= ADDRESS_ERROR;
break;
@@ -1466,6 +1460,15 @@ void sd_write_data(SDState *sd, uint8_t value)
sd-card_status |= WP_VIOLATION;
break;
}
+}
+sd-data[sd-data_offset++] = value;
+if (sd-data_offset = sd-blk_len) {
+/* TODO: Check CRC before committing */
+sd-state = sd_programming_state;
+BLK_WRITE_BLOCK(sd-data_start, sd-data_offset);
+sd-blk_written++;
+sd-data_start += sd-blk_len;
+sd-data_offset = 0;
sd-csd[14] |= 0x40;
/* Bzzztt Operation complete. */
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 25 July 2011 13:18, Jan Kiszka jan.kis...@siemens.com wrote: On 2011-07-25 14:05, Peter Maydell wrote: In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. Then you may want to drop is_default = 1 from integratorcp and prepare the main loop to face a NULL machine. We can't change the default machine for -M, that would break backwards compatibility. All we can do is avoid having a notion of default machine in new command line syntax. -- PMM
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 07/25/2011 05:59 AM, Jan Kiszka wrote: On 2011-07-25 12:45, Richard W.M. Jones wrote: That's an excuse, not a practical solution. We have to be able to work with any qemu. eg. the qemu in current Fedora Rawhide which supports only -machine accel=, or qemu in other distros which are also branched from arbitrary git releases, or qemu that people compile themselves. In principle, this is first of all a Rawhide problem. Upstream really can't babysit every distro doing crazy things with arbitrary devel snapshots. These patches were public, and the maintainers had a fair chance to realize that the interface was not yet set in stone. Parsing -help output and guesswork isn't scalable, and this is not exactly the first time that people have complained about this. I agree. That's why we try hard to release stable interfaces and then maintain them. (Yes, libvirt and libguestfs do allow callers to mechanically query their respective APIs for capabilities.) Maybe Anthony's (Liguori) rework of the QEMU config interfaces will provide a better reflections, haven't checked. But for now you need to stick with this model, specifically when you want to maintain all the distro forks. Yes, it will, but it doesn't fix this particular. Problem. Until we do a release, we reserve the right to change the syntax of newly introduced command line options. I don't think any level of introspection changes this. Regards, Anthony Liguori Jan
Re: [Qemu-devel] [PATCH v1 1/1] Submit the codes for QEMU disk I/O limits.
On Mon, Jul 25, 2011 at 8:08 AM, Zhi Yong Wu zwu.ker...@gmail.com wrote:
On Fri, Jul 22, 2011 at 6:54 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Jul 22, 2011 at 10:20 AM, Zhi Yong Wu wu...@linux.vnet.ibm.com
wrote:
+ elapsed_time = (real_time - bs-slice_start[is_write]) / 10.0;
+ fprintf(stderr, real_time = %ld, slice_start = %ld, elapsed_time =
%g\n, real_time, bs-slice_start[is_write], elapsed_time);
+
+ bytes_limit = bps_limit * slice_time;
+ bytes_disp = bs-io_disps-bytes[is_write];
+ if (bs-io_limits-bps[2]) {
+ bytes_disp += bs-io_disps-bytes[!is_write];
+ }
+
+ bytes_res = (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
+ fprintf(stderr, bytes_limit = %g bytes_disp = %g, bytes_res = %g,
elapsed_time = %g\n, bytes_limit, bytes_disp, bytes_res, elapsed_time);
+
+ if (bytes_disp + bytes_res = bytes_limit) {
+ if (wait) {
+ *wait = 0;
+ }
+
+ fprintf(stderr, bytes_disp + bytes_res = bytes_limit\n);
+ return false;
+ }
+
+ /* Calc approx time to dispatch */
+ wait_time = (bytes_disp + bytes_res - bytes_limit) / bps_limit;
+ if (!wait_time) {
+ wait_time = 1;
+ }
+
+ wait_time = wait_time + (slice_time - elapsed_time);
+ if (wait) {
+ *wait = wait_time * 10 + 1;
+ }
+
+ return true;
+}
After a slice expires all bytes/iops dispatched data is forgotten,
even if there are still requests queued. This means that requests
issued by the guest immediately after a new 100 ms period will be
issued but existing queued requests will still be waiting.
And since the queued requests don't get their next chance until later,
it's possible that they will be requeued because the requests that the
guest snuck in have brought us to the limit again.
In order to solve this problem, we need to extend the current slice if
Extend the current slice? like in-kernel block throttling algorithm?
Our algorithm seems not to adopt it currently.
I'm not sure if extending the slice is necessary as long as new
requests are queued while previous requests are still queued. But
extending slices is one way to deal with requests that span across
multiple slices. See below.
there are still requests pending. To prevent extensions from growing
the slice forever (and keeping too much old data around), it should be
alright to cull data from 2 slices ago. The simplest way of doing
that is to subtract the bps/iops limits from the bytes/iops
dispatched.
You mean that the largest value of current_slice_time is not more than
2 slice_time?
Yes. If no single request is larger than the I/O limit, then the
timer value for a queued request should always be within the next
slice. Therefore everything before last slice should be completed
already and we don't need to keep that history around.
@@ -2129,6 +2341,19 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState
*bs, int64_t sector_num,
if (bdrv_check_request(bs, sector_num, nb_sectors))
return NULL;
+ /* throttling disk read I/O */
+ if (bs-io_limits != NULL) {
+ if (bdrv_exceed_io_limits(bs, nb_sectors, false, wait_time)) {
+ ret = qemu_block_queue_enqueue(bs-block_queue, bs,
bdrv_aio_readv,
+ sector_num, qiov, nb_sectors, cb, opaque);
5 space indent, should be 4.
+ fprintf(stderr, bdrv_aio_readv: wait_time = %ld, timer value =
%ld\n, wait_time, wait_time + qemu_get_clock_ns(vm_clock));
+ qemu_mod_timer(bs-block_timer, wait_time +
qemu_get_clock_ns(vm_clock));
Imagine 3 requests that need to be queued: A, B, and C. Since the
timer is unconditionally set each time a request is queued, the timer
will be set to C's wait_time. However A or B's wait_time might be
earlier and we will miss that deadline!
Yeah, exactly there is this issue.
We really need a priority queue here. QEMU's timers solve the same
problem with a sorted list, which might actually be faster for short
lists where a fancy data structure has too much overhead.
You mean the block requests should be handled in FIFO way in order?
If the block queue is not empty, should this coming request be
enqueued at first? right?
Yes. If the limit was previously exceeded, enqueue new requests immediately:
/* If a limit was exceeded, immediately queue this request */
if (!QTAILQ_EMPTY(queue-requests)) {
if (limits-bps[IO_LIMIT_TOTAL]) {
/* queue any rd/wr request */
} else if (limits-bps[is_write] another_request_is_queued[is_write]) {
/* queue if the rd/wr-specific limit was previously exceeded */
}
...same for iops...
}
This way new requests cannot skip ahead of queued requests due to the
lost history when a new slice starts.
+BlockDriverAIOCB *qemu_block_queue_enqueue(BlockQueue *queue,
+ BlockDriverState *bs,
+ BlockRequestHandler
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 2011-07-25 14:22, Peter Maydell wrote: On 25 July 2011 13:18, Jan Kiszka jan.kis...@siemens.com wrote: On 2011-07-25 14:05, Peter Maydell wrote: In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. Then you may want to drop is_default = 1 from integratorcp and prepare the main loop to face a NULL machine. We can't change the default machine for -M, that would break backwards compatibility. All we can do is avoid having a notion of default machine in new command line syntax. The new syntax can't change is that as we cannot tell apart the omitting of -M from that of -machine. Both will have the semantic use default machine. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 01/55] blockdev: Make eject fail for non-removable drives even with -f
Looks good, Reviewed-by: Christoph Hellwig h...@lst.de
Re: [Qemu-devel] [PATCH 02/55] block: Reset device model callbacks on detach
On Wed, Jul 20, 2011 at 06:23:36PM +0200, Markus Armbruster wrote: BlockDriverState members change_cb and change_opaque are initially null. The device model may set them, with bdrv_set_change_cb(). If the device model gets detached (hot unplug), they're left dangling. Only safe because device hot unplug automatically destroys the BlockDriverState. But that's a questionable feature, best not to rely on it. Looks good, Reviewed-by: Christoph Hellwig h...@lst.de
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 04:52 AM, Avi Kivity wrote: On 07/25/2011 12:48 PM, Peter Maydell wrote: On 25 July 2011 10:32, Alexander Grafag...@suse.de wrote: On 25.07.2011, at 10:51, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. What does this buy you over type *x = qemu_malloc(sizeof(type)); ? I find the non-C++ version easier to read even. Yeah, while we're writing in C we should just stick to the C-like APIs, it's less confusing IMHO than wrapping it up in something else. That argument can be used to block any change. You'll get used to it in time. The question is, is the new interface better or not. I assume Anthony's new object model stuff will have a create me a new foo object API anyway, so QEMU_NEW() is possibly a bit of a namespace grab. Anthony's stuff is at a much higher level, hopefully he'll come back to the ground one day. The point of introducing glib was to address things like this. We need to start making heavier use of what it provides. Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH 04/55] block: Generalize change_cb() to BlockDevOps
On Wed, Jul 20, 2011 at 06:23:38PM +0200, Markus Armbruster wrote: So we can more easily add device model callbacks. Signed-off-by: Markus Armbruster arm...@redhat.com Looks good, Reviewed-by: Christoph Hellwig h...@lst.de
Re: [Qemu-devel] [PATCH 05/55] block: Split change_cb() into change_media_cb(), resize_cb()
On Wed, Jul 20, 2011 at 06:23:39PM +0200, Markus Armbruster wrote: Multiplexing callbacks complicates matters needlessly. Looks good, Reviewed-by: Christoph Hellwig h...@lst.de
Re: [Qemu-devel] [PATCH 06/55] block/raw-win32: Drop disabled code for removable host devices
On Wed, Jul 20, 2011 at 06:23:40PM +0200, Markus Armbruster wrote: It's been disabled since the start (commit 19cb3738, Aug 2006), and has been untouched except for spelling fixes and such. I don't feel like dragging it along any further. Signed-off-by: Markus Armbruster arm...@redhat.com Ok, let's nuke it.
Re: [Qemu-devel] [PATCH 10/55] ide: Update command code definitions as per ACS-2 Table B.2
On Wed, Jul 20, 2011 at 06:23:44PM +0200, Markus Armbruster wrote: Drop WIN_SRST, it has same value as WIN_DEVICE_RESET. CFA_IDLEIMMEDIATE isn't specific to CFATA. ACS-2 shows it as a defined command in ATA-1, -2 and -3. Rename to WIN_IDLEIMMEDIATE2. Turn unused macros into comments. I don't think that part is overly helpful. In general you'd want to keep all defines for protocol headers, just to make it easier to actually use them later on.
Re: [Qemu-devel] [PATCH 11/55] ide: Clean up case label indentation in ide_exec_cmd()
Looks good, Reviewed-by: Christoph Hellwig h...@lst.de
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command block_set for dynamic block params change
On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+ret = bdrv_open(bs, bs-filename, bdrv_flags, drv);
+if (ret 0) {
+/* Reopen failed. Try to open with original flags */
+error_report(Opening file with changed flags...);
+qerror_report(QERR_REOPEN_FILE_FAILED, bs-filename);
If the next open fails too then there will be two qerror_report() calls.
This causes a warning and the new qerror is dropped. Please consolidate
the error reporting so there is only one error before returning from
this function.
ok
+if (bdrv_is_inserted(bs)) {
+/* Reopen file with changed set of flags */
+return bdrv_reopen(bs, bdrv_flags);
+} else {
+/* Save hostcache change for future use */
+bs-open_flags = bdrv_flags;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Intention here is to use the changed hostcache setting during device
insertion and there after. To apply this to 'change' command (during
insertion of a device), will need to make the following code changes
in do_change_block.
+
+bdrv_flags = bs-open_flags;
+bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
if (bdrv_open(bs, filename, bdrv_flags, drv) 0) {
qerror_report(QERR_OPEN_FILE_FAILED, filename);
Need to track bs-open_flags a bit more to see what other changes
needed to ensure usage of changed hostcache setting until user
initiates next hostcache change explicitly for that drive.
Please suggest... should we be saving the hostcache change for
future use or just inhibit user from changing hostcache setting
for an empty drive?
+/* If input not in param=value format, display error */
+driver = qemu_opt_get(opts, driver);
+if (driver != NULL) {
+error_report(Invalid parameter %s, driver);
error_report() only works for HMP. Please use qerror_report() so both
HMP and QMP see the error. Same issue further down.
ok..will change error_report to qerror_report.
Will make further code changes into patchset version 5
Re: [Qemu-devel] [PATCH] Introduce QEMU_NEW()
On 07/25/2011 03:21 PM, Anthony Liguori wrote: On 07/25/2011 07:18 AM, Avi Kivity wrote: On 07/25/2011 03:11 PM, Anthony Liguori wrote: On 07/25/2011 03:51 AM, Avi Kivity wrote: qemu_malloc() is type-unsafe as it returns a void pointer. Introduce QEMU_NEW() (and QEMU_NEWZ()), which return the correct type. Just use g_new() and g_new0() These bypass qemu_malloc(). Are we okay with that? Yes. We can just make qemu_malloc use g_malloc. Excellent. Patch withdrawn. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile: fix out-of-tree builds
On Mon, Jul 25, 2011 at 1:16 PM, Michael Roth mdr...@linux.vnet.ibm.com wrote: On 07/25/2011 05:15 AM, Stefan Hajnoczi wrote: On Thu, Jul 21, 2011 at 5:41 AM, Alexandre Raymondcerb...@gmail.com wrote: This patch fixes a minor bugs which prevented QEMU from being built out of tree. Signed-off-by: Alexandre Raymondcerb...@gmail.com --- Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I don't normally use --source-path but it still seems broken to me after applying your patch? $ cd /tmp; mkdir out; cd out $ ~/qemu/configure --source-path=$HOME/qemu $ make GEN config-all-devices.mak cat: i386-softmmu/config-devices.mak: No such file or directory cat: x86_64-softmmu/config-devices.mak: No such file or directory cat: alpha-softmmu/config-devices.mak: No such file or directory Stefan Works okay for me with and without the patch if I do a `make distclean` in $HOME/qemu beforehand. Not sure what the trigger is for the breakage Alexandre is trying to address. You are right that make distclean in the source directory solves the issue. Intuitively I expect ./configure to re-wire things, make distclean should not be necessary. Alexandre: Can you describe the case where you hit a build issue in more detail? Stefan
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 25.07.2011, at 14:05, Peter Maydell wrote: On 25 July 2011 12:48, Peter Maydell peter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. That's not exactly true. For PPC, everyone so far expects a Mac to pop up. For S390x, we only have a single machine implemented. I think having a default makes it easier for users to run something you give to them - if it runs on the default target. (Consider also the possibility of eventually having a single qemu binary that supports multiple architectures -- that would make a 'default machine' definitely a bit odd.) It would be a different machine depending on the emulated target. Alex
Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model
On 07/25/2011 06:21 AM, Kevin Wolf wrote: Am 25.07.2011 03:44, schrieb Anthony Liguori: Hi, This series is the rough beginnings of the QEMU Object Model. This is basically qdev generalized on steroids. This series includes the core infrastructure, a strawman Device type, and the beginnings of the conversion of CharDriverState. This is in a rougher state than I would like it to be but I wanted to get the concepts on the list as soon as possible. My plan is to drop the Device parts from future versions of the series and just focus on backends to start with. Please note that this series has an awful lot of ramifications. Most of our current command line options would become deprecated, the build system will change significantly, and a lot of our QMP functions will become deprecated. It seems like a lot of change, but hopefully this series illustrates how we can do it very incrementally with value being added at each stage of the conversion. I haven't looked in much detail at it yet, but it has still the same problem I was talking about last week: Patches 17-21 don't actually convert existing code, but they add new code. Actually, it's mostly existing code. In terms of incremental conversion, the most straight forward way is to adds the new version side-by-side with the old version and then remove the old version. Converting in device in place requires some gymnastics. If you think it's absolutely critical, I could try to do it but I'm not sure I agree it's the thing to do. This means that we can't review only the changes, but have to review the whole code. It also makes conflicts with patches modifying the old version hard to even notice. On another note, I'm not so sure if your renaming is really helpful. It doesn't matter that much with qemu-char because someone thought having the function pointers in CharDriverState was a good idea, but if you're consistent, the rename would go like this in the block layer: BlockDriverState - BlockDriver BlockDriver - BlockDriverClass I think we do need to introduce consistent naming conventions. If those conventions are FooState and Foo, that could be okay, but the code base today is absolutely not consistent on it. I think Foo and FooClass is better because Foo is the most common usage of the type and it's less characters to type. Regards, Anthony Liguori IMHO, that's not very helpful, but just going to create confusion. We could probably discuss other parts of the terminology, too, but let's save the bikeshedding for later. Kevin
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 07/25/2011 07:44 AM, Alexander Graf wrote: On 25.07.2011, at 14:05, Peter Maydell wrote: On 25 July 2011 12:48, Peter Maydellpeter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. That's not exactly true. For PPC, everyone so far expects a Mac to pop up. Except if you're running on an IBM Power box, then you definitely expect a pseries guest to pop up. We really need to enable the default config file (yes, we have a default config file) can express the default machine. Regards, Anthony Liguori
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On Mon, Jul 25, 2011 at 07:47:51AM -0500, Anthony Liguori wrote: On 07/25/2011 07:44 AM, Alexander Graf wrote: On 25.07.2011, at 14:05, Peter Maydell wrote: On 25 July 2011 12:48, Peter Maydellpeter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. That's not exactly true. For PPC, everyone so far expects a Mac to pop up. Except if you're running on an IBM Power box, then you definitely expect a pseries guest to pop up. We really need to enable the default config file (yes, we have a default config file) can express the default machine. +1 to this. I was going to say there's missing information here, ie. if I had a Debian/arm kernel know, what machine should I use, but it looks like a config file would provide this missing information. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command block_set for dynamic block params change
On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery supri...@in.ibm.com wrote:
On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+ if (bdrv_is_inserted(bs)) {
+ /* Reopen file with changed set of flags */
+ return bdrv_reopen(bs, bdrv_flags);
+ } else {
+ /* Save hostcache change for future use */
+ bs-open_flags = bdrv_flags;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Intention here is to use the changed hostcache setting during device
insertion and there after. To apply this to 'change' command (during
insertion of a device), will need to make the following code changes
in do_change_block.
+
+ bdrv_flags = bs-open_flags;
+ bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
if (bdrv_open(bs, filename, bdrv_flags, drv) 0) {
qerror_report(QERR_OPEN_FILE_FAILED, filename);
Need to track bs-open_flags a bit more to see what other changes
needed to ensure usage of changed hostcache setting until user
initiates next hostcache change explicitly for that drive.
Please suggest... should we be saving the hostcache change for
future use or just inhibit user from changing hostcache setting
for an empty drive?
The 'change' command does not support any cache= options today. It
always opens the new image with cache=writethrough semantics.
This seems like a bug in 'change' to me. We should preserve cache=
settings across change or at least provide a way to specify them as
arguments.
Perhaps your existing code is fine. When 'change' is fixed or
replaced then 'block_set' will work across 'change' too.
Kevin: Thoughts?
Stefan
Re: [Qemu-devel] [RESEND][PATCH v3] Generalize -machine command line option
On 25.07.2011, at 14:49, Richard W.M. Jones wrote: On Mon, Jul 25, 2011 at 07:47:51AM -0500, Anthony Liguori wrote: On 07/25/2011 07:44 AM, Alexander Graf wrote: On 25.07.2011, at 14:05, Peter Maydell wrote: On 25 July 2011 12:48, Peter Maydellpeter.mayd...@linaro.org wrote: For ARM you absolutely should not be relying on the default machine type (not least because it's an incredibly ancient dev board which nobody uses any more). An ARM kernel is generally fairly specific to the hardware platform being emulated, so you should know which machine you're intending to run on and specify it explicitly. In fact having thought about it a bit I'm going to go further and say that the whole idea of a default machine is a rather x86-centric idea -- most architectures don't really have a single machine type that's used by just about everybody, always has been, and isn't likely to become obsolete in the future. So if we're reworking the command line API to supersede -M then we shouldn't have a default at all. That's not exactly true. For PPC, everyone so far expects a Mac to pop up. Except if you're running on an IBM Power box, then you definitely expect a pseries guest to pop up. We really need to enable the default config file (yes, we have a default config file) can express the default machine. +1 to this. I was going to say there's missing information here, ie. if I had a Debian/arm kernel know, what machine should I use, but it looks like a config file would provide this missing information. Well, what we really want is an image file format that also pulls along machine config files. Or just have 2 files for now - an image and a machine description config. VMs simply are more than just their hard disks ;). Alex
Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model
Am 25.07.2011 14:45, schrieb Anthony Liguori: On 07/25/2011 06:21 AM, Kevin Wolf wrote: Am 25.07.2011 03:44, schrieb Anthony Liguori: Hi, This series is the rough beginnings of the QEMU Object Model. This is basically qdev generalized on steroids. This series includes the core infrastructure, a strawman Device type, and the beginnings of the conversion of CharDriverState. This is in a rougher state than I would like it to be but I wanted to get the concepts on the list as soon as possible. My plan is to drop the Device parts from future versions of the series and just focus on backends to start with. Please note that this series has an awful lot of ramifications. Most of our current command line options would become deprecated, the build system will change significantly, and a lot of our QMP functions will become deprecated. It seems like a lot of change, but hopefully this series illustrates how we can do it very incrementally with value being added at each stage of the conversion. I haven't looked in much detail at it yet, but it has still the same problem I was talking about last week: Patches 17-21 don't actually convert existing code, but they add new code. Actually, it's mostly existing code. In terms of incremental conversion, the most straight forward way is to adds the new version side-by-side with the old version and then remove the old version. Converting in device in place requires some gymnastics. If you think it's absolutely critical, I could try to do it but I'm not sure I agree it's the thing to do. Okay, if it isn't possible with reasonable effort, I guess we'll have to bite the bullet and give it a very careful manual review immediately before the merge. By the way, I see that you create new directories. You probably have an idea of what the directory structure will look like after the whole conversion is completed. Can you share this idea with us? This means that we can't review only the changes, but have to review the whole code. It also makes conflicts with patches modifying the old version hard to even notice. On another note, I'm not so sure if your renaming is really helpful. It doesn't matter that much with qemu-char because someone thought having the function pointers in CharDriverState was a good idea, but if you're consistent, the rename would go like this in the block layer: BlockDriverState - BlockDriver BlockDriver - BlockDriverClass I think we do need to introduce consistent naming conventions. If those conventions are FooState and Foo, that could be okay, but the code base today is absolutely not consistent on it. I think Foo and FooClass is better because Foo is the most common usage of the type and it's less characters to type. Maybe. But then, CharDriver is a really bad names for an instance. There is only one driver, which made it a good name for the class until now. Maybe CharBackend and CharBackendClass (or CharBackendDriver) would be a more sensible replacement that follows your pattern. Kevin
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. - PCI devices do not respect PAM (if a PCI device maps a region while PAM maps the region to RAM, the request will be honored) I assume you mean SMM shadowing, right? PAM doesn't cover an area that's ever forwarded to the PCI bus. This patch fixes things by introducing a pci address space, and using memory region aliases to represent PAM regions, SMRAM, and PCI holes. The memory hierarchy looks something like system_memory | +--- low memory alias (0-0xe000) According to the spec, PCI memory doesn't start at e00... but rather at the top of RAM. In fact, this is what the spec says: The address range from the top of main DRAM to 4 Gbytes (top of physical memory space supported by the 440FX PCIset) is normally mapped to PCI. The PMC forwards all accesses within this address range to PCI. There are two sub-ranges within this address range defined as APIC Configuration Space and High BIOS Address Range. So the right thing to do is to forward all accesses from low_memory_memsize ... 4GB to the PCI bus. Regards, Anthony Liguori
Re: [Qemu-devel] [RFC][PATCH 0/21] QEMU Object Model
On 07/25/2011 08:08 AM, Kevin Wolf wrote: Am 25.07.2011 14:45, schrieb Anthony Liguori: Okay, if it isn't possible with reasonable effort, I guess we'll have to bite the bullet and give it a very careful manual review immediately before the merge. By the way, I see that you create new directories. You probably have an idea of what the directory structure will look like after the whole conversion is completed. Can you share this idea with us? Just the logic extension of what we have already: block/ chrdrv/ net/ qom/ qapi/ devices/ \ pc/ | pci/ | scsi/ | etc. I think Foo and FooClass is better because Foo is the most common usage of the type and it's less characters to type. Maybe. But then, CharDriver is a really bad names for an instance. There is only one driver, which made it a good name for the class until now. Maybe CharBackend and CharBackendClass (or CharBackendDriver) would be a more sensible replacement that follows your pattern. Good suggestion. I've been thinking that there's like to be a need for a generic Backend base class too so that would work well from a naming convention perspective. Regards, Anthony Liguori Kevin
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. - PCI devices do not respect PAM (if a PCI device maps a region while PAM maps the region to RAM, the request will be honored) I assume you mean SMM shadowing, right? PAM doesn't cover an area that's ever forwarded to the PCI bus. No, PAM. And all of the PAM address space is forwarded to the PCI bus (the ROMs are satisfied by PIIX which is a PCI device). Here at least the spec is clear, see 3.2.18. This patch fixes things by introducing a pci address space, and using memory region aliases to represent PAM regions, SMRAM, and PCI holes. The memory hierarchy looks something like system_memory | +--- low memory alias (0-0xe000) According to the spec, PCI memory doesn't start at e00... but rather at the top of RAM. In fact, this is what the spec says: The address range from the top of main DRAM to 4 Gbytes (top of physical memory space supported by the 440FX PCIset) is normally mapped to PCI. The PMC forwards all accesses within this address range to PCI. There are two sub-ranges within this address range defined as APIC Configuration Space and High BIOS Address Range. So the right thing to do is to forward all accesses from low_memory_memsize ... 4GB to the PCI bus. We could do that. However what happens if we implement memory hotplug? Maybe we should implement memory hotplug on a newer chipset anyway. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11.TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset:69h Default Value: 02h Attribute: Read/Write -- Gleb.
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 04:17 PM, Gleb Natapov wrote: On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11.TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset:69h Default Value: 02h Attribute: Read/Write What's it doing in PIIX3? Is it the same TOM? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command block_set for dynamic block params change
Am 25.07.2011 14:50, schrieb Stefan Hajnoczi:
On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery supri...@in.ibm.com wrote:
On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+if (bdrv_is_inserted(bs)) {
+/* Reopen file with changed set of flags */
+return bdrv_reopen(bs, bdrv_flags);
+} else {
+/* Save hostcache change for future use */
+bs-open_flags = bdrv_flags;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Intention here is to use the changed hostcache setting during device
insertion and there after. To apply this to 'change' command (during
insertion of a device), will need to make the following code changes
in do_change_block.
+
+bdrv_flags = bs-open_flags;
+bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
if (bdrv_open(bs, filename, bdrv_flags, drv) 0) {
qerror_report(QERR_OPEN_FILE_FAILED, filename);
Need to track bs-open_flags a bit more to see what other changes
needed to ensure usage of changed hostcache setting until user
initiates next hostcache change explicitly for that drive.
Please suggest... should we be saving the hostcache change for
future use or just inhibit user from changing hostcache setting
for an empty drive?
The 'change' command does not support any cache= options today. It
always opens the new image with cache=writethrough semantics.
This seems like a bug in 'change' to me. We should preserve cache=
settings across change or at least provide a way to specify them as
arguments.
Perhaps your existing code is fine. When 'change' is fixed or
replaced then 'block_set' will work across 'change' too.
Kevin: Thoughts?
I'm not sure if I would say it's a bug. Probably preserving would be
more useful in most cases, but using the default cache mode doesn't
appear completely wrong either. If you like to change it, I'm not
opposed to it.
Kevin
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On Mon, Jul 25, 2011 at 04:28:12PM +0300, Avi Kivity wrote: On 07/25/2011 04:17 PM, Gleb Natapov wrote: On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11.TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset:69h Default Value: 02h Attribute: Read/Write What's it doing in PIIX3? Is it the same TOM? Good question. Looks like it is not: This register enables the forwarding of ISA or DMA memory cycles to the PCI Bus and sets the top of main memory accessible by ISA or DMA devices. In addition, this register controls the forwarding of ISA or DMA accesses to the lower BIOS region (E–Eh) and the 512–640-Kbyte main memory region (8– 9h). The Top Of Memory configuration register must be set by the BIOS. -- Gleb.
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 04:28 PM, Avi Kivity wrote: On 07/25/2011 04:17 PM, Gleb Natapov wrote: On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11.TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset:69h Default Value: 02h Attribute: Read/Write What's it doing in PIIX3? Is it the same TOM? That's the ISA TOM (15MB hole and friends). -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 08:28 AM, Avi Kivity wrote: On 07/25/2011 04:17 PM, Gleb Natapov wrote: On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11. TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset: 69h Default Value: 02h Attribute: Read/Write What's it doing in PIIX3? Is it the same TOM? This is a programmable register used to indicate the TOM for the purposes of forwarding ISA DMA transactions. It is unrelated to the I440FX notion of top of memory. Regards, Anthony Liguori
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On Mon, Jul 25, 2011 at 04:31:27PM +0300, Avi Kivity wrote: On 07/25/2011 04:28 PM, Avi Kivity wrote: On 07/25/2011 04:17 PM, Gleb Natapov wrote: On Mon, Jul 25, 2011 at 04:14:45PM +0300, Avi Kivity wrote: On 07/25/2011 04:07 PM, Anthony Liguori wrote: On 07/20/2011 11:50 AM, Avi Kivity wrote: The current implementation of PAM and the PCI holes is broken in several ways: - PCI BARs are not restricted to the PCI hole (a BAR may hide memory) Technically, a BAR can be mapped to any non-RAM memory location. I understood TOM (Top Of Memory) to be fixed - can't find a register for it - but maybe I misread the spec. PIIX3 spec: 2.2.11.TOM—TOP OF MEMORY REGISTER (Function 0) Address Offset:69h Default Value: 02h Attribute: Read/Write What's it doing in PIIX3? Is it the same TOM? That's the ISA TOM (15MB hole and friends). Correct. What about: 3.2.19.DRB[0:7] DRAM ROW BOUNDARY REGISTERS from 440fx spec? -- Gleb.
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 04:35 PM, Gleb Natapov wrote: That's the ISA TOM (15MB hole and friends). Correct. What about: 3.2.19.DRB[0:7] DRAM ROW BOUNDARY REGISTERS from 440fx spec? Maybe. But we can't use that, since it ignores address line 31. (440fx supports only 1GB RAM, and we're ignoring that) -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On 07/25/2011 08:38 AM, Avi Kivity wrote: On 07/25/2011 04:35 PM, Gleb Natapov wrote: That's the ISA TOM (15MB hole and friends). Correct. What about: 3.2.19. DRB[0:7] DRAM ROW BOUNDARY REGISTERS from 440fx spec? Maybe. But we can't use that, since it ignores address line 31. (440fx supports only 1GB RAM, and we're ignoring that) What are we trying to do? Can't we just register highest RAM address under 4G to 4G as PCI memory and call it a day? Do we really need a guest visible register to do this? Regards, Anthony Liguori
Re: [Qemu-devel] [RFC v5 86/86] 440fx: fix PAM, PCI holes
On Mon, Jul 25, 2011 at 08:47:31AM -0500, Anthony Liguori wrote: On 07/25/2011 08:38 AM, Avi Kivity wrote: On 07/25/2011 04:35 PM, Gleb Natapov wrote: That's the ISA TOM (15MB hole and friends). Correct. What about: 3.2.19. DRB[0:7] DRAM ROW BOUNDARY REGISTERS from 440fx spec? Maybe. But we can't use that, since it ignores address line 31. (440fx supports only 1GB RAM, and we're ignoring that) What are we trying to do? I just tried to answer Avi's question about TOM register. Can't we just register highest RAM address under 4G to 4G as PCI memory and call it a day? It is good idea to emulate existing functionality as close as possible, but if existing functionality does not satisfy us (like in our case) then yes, we can. Do we really need a guest visible register to do this? Regards, Anthony Liguori -- Gleb.
[Qemu-devel] 垃圾邮件通知信
Title: Spam Quarantine Notification Spam Quarantine Notification 下列邮件已被您的管理员作为可疑垃圾邮件阻止。 自您上一次收到垃圾邮件隔离区通知以来，您的电子邮件隔离区内已有 1 封新邮件。如果下列邮件是垃圾邮件，您不需要采取任何措施。邮件将于 7 天后自动从隔离区中移除。 如果下列任何邮件不是垃圾邮件，请单击“放行”链接以将其发送到您的收件箱。要查看所有隔离邮件，请查看 您的电子邮件隔离区。 已隔离的电子邮件 发件人 主题 日期 放行 杨璐 [垃圾邮件] cjsc...@cjsc.com.cn敬请关注广东省高层次人才洽谈会hih 24 Jul 2011 查看所有隔离的邮件(1) 注意: 此邮件是由仅用于通知的系统发送的。请不要 回复如果以上链接无效，请将以下 URL 复制并粘贴到 Web 浏览器中: http://quarantine.cjsc.com.cn:82/Search?h=4bdaba1c4191fb427a7e1cc7efeba63b=qemu-devel%40nongnu.org
Re: [Qemu-devel] [Qemu-trivial] [PATCH] Makefile: fix out-of-tree builds
On 07/25/2011 07:43 AM, Stefan Hajnoczi wrote: On Mon, Jul 25, 2011 at 1:16 PM, Michael Rothmdr...@linux.vnet.ibm.com wrote: On 07/25/2011 05:15 AM, Stefan Hajnoczi wrote: On Thu, Jul 21, 2011 at 5:41 AM, Alexandre Raymondcerb...@gmail.com wrote: This patch fixes a minor bugs which prevented QEMU from being built out of tree. Signed-off-by: Alexandre Raymondcerb...@gmail.com --- Makefile |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I don't normally use --source-path but it still seems broken to me after applying your patch? $ cd /tmp; mkdir out; cd out $ ~/qemu/configure --source-path=$HOME/qemu $ make GEN config-all-devices.mak cat: i386-softmmu/config-devices.mak: No such file or directory cat: x86_64-softmmu/config-devices.mak: No such file or directory cat: alpha-softmmu/config-devices.mak: No such file or directory Stefan Works okay for me with and without the patch if I do a `make distclean` in $HOME/qemu beforehand. Not sure what the trigger is for the breakage Alexandre is trying to address. You are right that make distclean in the source directory solves the issue. Intuitively I expect ./configure to re-wire things, make distclean should not be necessary. Alexandre: Can you describe the case where you hit a build issue in more detail? Stefan The root problem seems to be that by including $(SRC_DIR) in VPATH (via $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)), $(SRC_DIR) ends up being searched for source files as well as target dependencies. So any crud left in there can still satisfy dependencies when building outside $SRC_PATH. I'm not sure there's a simple way around this except to prefix all source files with $(SRC_PATH) and remove $(SRC_PATH) from VPATH...I'm not even sure that would work though.. Perhaps just a friendly error message if we detect the $(SRC_PATH) directory needs a distclean? Once you know that's the magic fix it's not terribly inconvenientalternatively we could automatically do the distclean in $SRC_PATH but that might be considered overstepping our bounds. Consequently, it seems like this patch would be a noop...default-configs should never exist in an external build directory, so $(SRC_PATH)/default-configs and default-configs end up being equivalent when make eventually find it in $(SRC_PATH).
