[Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
are from an array of the uint8_t type
Carlo
---
Index: block-vvfat.c
===
RCS file: /sources/qemu/qemu/block-vvfat.c,v
retrieving revision 1.16
diff -u -p -r1.16 block-vvfat.c
--- block-vvfat.c 24 Dec 2007 13:26:04 - 1.16
+++ block-vvfat.c 4 Jan 2008 07:57:20 -
@@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
int current_fd;
mapping_t* current_mapping;
unsigned char* cluster; /* points to current cluster */
-unsigned char* cluster_buffer; /* points to a buffer to hold temp data */
+uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
unsigned int current_cluster;
/* write support */
Index: block.c
===
RCS file: /sources/qemu/qemu/block.c,v
retrieving revision 1.53
diff -u -p -r1.53 block.c
--- block.c 24 Dec 2007 16:10:43 - 1.53
+++ block.c 4 Jan 2008 07:57:21 -
@@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
BlockDriver *drv = bs-drv;
int64_t i, total_sectors;
int n, j;
-unsigned char sector[512];
+uint8_t sector[512];
if (!drv)
return -ENOMEDIUM;
Re: [Qemu-devel] bug in qemu last win32 snapshot (12132007)
I didn't see any messages indicating that my patch was committed to CVS. Maybe I missed it. I someone could confirm that this is indeed the case, I can look at it Monday on a Windows system... - Original Message From: Sergey Bychkov [EMAIL PROTECTED] To: qemu-devel@nongnu.org Sent: Thursday, 3 January, 2008 9:42:18 PM Subject: Re: [Qemu-devel] bug in qemu last win32 snapshot (12132007) - Original Message - From: Sebastien WILLEMIJNS [EMAIL PROTECTED] To: qemu-devel@nongnu.org Sent: 2.01.2008 11:00 Subject: Re: [Qemu-devel] bug in qemu last win32 snapshot (12132007) ... but it is a new information to write inside win documentation because if i use qemu-0.9.0 windows stable build \\ is not mandatory... Looks like result of patch posted by Cd-MaN 30.12.2007 12:01 EEST Sergey Bychkow ICQ: 21014758 FTN: 2:450/118.55 __ Sent from Yahoo! Mail - a smarter inbox http://uk.mail.yahoo.com
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
Can someone please comment on the mergability of this patch? or in what needs
to be done to it so that it can be committed?
The patch is still current and the bug it fixes would otherwise prevent
OpenSolaris guests to be installed in qemu. the MMC-6 command it fixes (GET
CONFIGURATION) has been verified to behave as per the SPECs and compared to
behave just like real hardware does.
Carlo
On Wed, Dec 26, 2007 at 01:36:15AM -0600, Carlo Marcelo Arenas Belon wrote:
The following patch implements fixes to the CD-ROM IDE/ATAPI emulation
since ide.c revision 1.66 and that prevents installation of OpenSolaris
guests because of timeouts like :
WARNING: /[EMAIL PROTECTED],0/[EMAIL PROTECTED],1/[EMAIL PROTECTED] (ata1);
timeout: abort request, target=0 lun=0
WARNING: /[EMAIL PROTECTED],0/[EMAIL PROTECTED],1/[EMAIL PROTECTED] (ata1):
timeout: abort device, target=0 lun=0
WARNING: /[EMAIL PROTECTED],0/[EMAIL PROTECTED],1/[EMAIL PROTECTED] (ata1):
timeout: reset target, target=0 lun=0
WARNING: /[EMAIL PROTECTED],0/[EMAIL PROTECTED],1/[EMAIL PROTECTED] (ata1):
timeout: reset bus, target=0 lun=0
It has been tested in Gentoo Linux 2007.0 amd64 (64bit) and x86 (32bit) hosts
using several different versions of Linux, FreeBSD, OpenBSD, NetBSD,
DragonFlyBSD, OpenSolaris and Windows 2000 guests
The patch implements changes to the following IDE commands as described by :
* change the model name (used by INQUIRY and IDENTIFY DEVICE) to DVD
* recognize and honor Allocation Length parameter in GET CONFIGURATION
* only set current profile for the GET CONFIGURATION response if a profile
is current (CD or DVD loaded)
* calculate data length including all headers
* refactor code and add comments to help document references to all
implemented standards (ATAPI-4, SPC-3 and MMC-6)
Carlo
---
Index: hw/ide.c
===
RCS file: /sources/qemu/qemu/hw/ide.c,v
retrieving revision 1.79
diff -u -p -r1.79 ide.c
--- hw/ide.c 24 Dec 2007 14:33:24 - 1.79
+++ hw/ide.c 26 Dec 2007 07:11:01 -
@@ -1,5 +1,5 @@
/*
- * QEMU IDE disk and CD-ROM Emulator
+ * QEMU IDE disk and CD/DVD-ROM Emulator
*
* Copyright (c) 2003 Fabrice Bellard
* Copyright (c) 2006 Openedhand Ltd.
@@ -540,7 +540,7 @@ static void ide_atapi_identify(IDEState
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
-padstr((char *)(p + 27), QEMU CD-ROM, 40); /* model */
+padstr((char *)(p + 27), QEMU DVD-ROM, 40); /* model */
put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
#ifdef USE_DMA_CDROM
put_le16(p + 49, 1 9 | 1 8); /* DMA and LBA supported */
@@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
buf[6] = 0; /* reserved */
buf[7] = 0; /* reserved */
padstr8(buf + 8, 8, QEMU);
-padstr8(buf + 16, 16, QEMU CD-ROM);
+padstr8(buf + 16, 16, QEMU DVD-ROM);
padstr8(buf + 32, 4, QEMU_VERSION);
ide_atapi_cmd_reply(s, 36, max_len);
break;
case GPCMD_GET_CONFIGURATION:
{
+uint32_t len;
uint64_t total_sectors;
/* only feature 0 is supported */
@@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
ASC_INV_FIELD_IN_CMD_PACKET);
break;
}
-memset(buf, 0, 32);
+max_len = ube16_to_cpu(packet + 7);
bdrv_get_geometry(s-bs, total_sectors);
-buf[3] = 16;
-buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current
profile */
-buf[10] = 0x10 | 0x1;
-buf[11] = 0x08; /* size of profile list */
+memset(buf, 0, 32);
+if (total_sectors) {
+if (total_sectors 1433600) {
+buf[7] = 0x10; /* DVD-ROM */
+} else {
+buf[7] = 0x08; /* CD-ROM */
+}
+} else {
+buf[7] = 0x00; /* no current profile */
+}
+buf[10] = 0x02 | 0x01; /* persistent and current */
+buf[11] = 0x08; /* size of profile list = 4 bytes per profile */
buf[13] = 0x10; /* DVD-ROM profile */
-buf[14] = buf[7] == 0x10; /* (in)active */
+buf[14] = buf[13] == buf[7]; /* (in)active */
buf[17] = 0x08; /* CD-ROM profile */
-buf[18] = buf[7] == 0x08; /* (in)active */
-ide_atapi_cmd_reply(s, 32, 32);
+buf[18] = buf[17] == buf[7]; /* (in)active */
+len = 8 + 4 + buf[11]; /* headers + size of profile list */
+cpu_to_ube32(buf, len - 4); /* data length */
+ide_atapi_cmd_reply(s, len, max_len);
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: Carlo Marcelo Arenas Belon wrote: Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write are from an array of the uint8_t type Do we have a host where this actually makes a difference? I believe Perl makes sizeof(char) checks, so there likely is some platform where sizeof(char) 1. Andreas
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit : Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: Carlo Marcelo Arenas Belon wrote: Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write are from an array of the uint8_t type Do we have a host where this actually makes a difference? I believe Perl makes sizeof(char) checks, so there likely is some platform where sizeof(char) 1. The C standard says `When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.' Samuel
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Carlo Marcelo Arenas Belon wrote:
Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
are from an array of the uint8_t type
Do we have a host where this actually makes a difference?
Thiemo
---
Index: block-vvfat.c
===
RCS file: /sources/qemu/qemu/block-vvfat.c,v
retrieving revision 1.16
diff -u -p -r1.16 block-vvfat.c
--- block-vvfat.c 24 Dec 2007 13:26:04 - 1.16
+++ block-vvfat.c 4 Jan 2008 07:57:20 -
@@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
int current_fd;
mapping_t* current_mapping;
unsigned char* cluster; /* points to current cluster */
-unsigned char* cluster_buffer; /* points to a buffer to hold temp data */
+uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
unsigned int current_cluster;
/* write support */
Index: block.c
===
RCS file: /sources/qemu/qemu/block.c,v
retrieving revision 1.53
diff -u -p -r1.53 block.c
--- block.c 24 Dec 2007 16:10:43 - 1.53
+++ block.c 4 Jan 2008 07:57:21 -
@@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
BlockDriver *drv = bs-drv;
int64_t i, total_sectors;
int n, j;
-unsigned char sector[512];
+uint8_t sector[512];
if (!drv)
return -ENOMEDIUM;
Re: [Qemu-devel] bug in qemu last win32 snapshot (12132007)
On Thu, 3 Jan 2008 23:45:11 -0800 (PST), Balazs Attila-Mihaly (Cd-MaN) [EMAIL PROTECTED] said: Looks like result of patch posted by Cd-MaN 30.12.2007 12:01 EEST I do not know how to compile QEMU windows ;) --- Sébastien WILLEMIJNS
Re: [Qemu-devel] performance monitor
On Friday 04 January 2008 09:49:22 Rob Landley wrote: On Thursday 03 January 2008 15:38:02 Clemens Kolbitsch wrote: Does anyone have an idea on how I can measure performance in qemu to a somewhat accurate level? hwclock --show time1 tar xvjf linux-2.6.23.tar.bz2 cd linux-2.6.23 make allnoconfig make cd .. hwclock --show time2 Do that on host and client, and you've got a ratio of the performance of qemu to your host that should be good to within a few percent. I have modified qemu (the memory handling) and the linux kernel and want to find out the penalty this introduced... does anyone have any comments / ideas on this? If it's something big, you can compare the result in minutes and seconds. That's probably the best you're going to do. (Although really you want hwclock --show before and after, and then do the math. That tunnels out to the host system to get its idea of the time, which doesn't get thrown off by timer interrupt delivery (as a signal) getting deferred by the host system's scheduler. Of course the fact that hwclock _takes_ a second or so to read the clock is a bit of a downer, but anything that takes less than a minute or so to run isn't going to give you a very accurate time because the performance of qemu isn't constant, and your results are going to skew all over the place. Especially for small things, the performance varies from run to run. Start by imagining qemu as having the mother of all page fault latencies. The cost of faulting code into the L2 cache includes dynamic recompilation, which is expensive. Worse, when the dynamic recompilation buffer fills up it blanks the whole thing, and recompiles every new page it hits one at a time until the buffer fills up again. (What is it these days, 16 megs of translated code before it resets?) No LRU or anything, no cache management at _all_, just when the bucket fills up, dump it and start over. (Well, that's what it did back around the last stable release anyway. It has been almost a year since then, so maybe it's changed. I've been busy with other things and not really keeping track of changes that didn't affect what I could and couldn't get to run.) So anyway, depending on what code you run in what order, the performance can _differ_ from one run to the next due to when the cache gets blanked and stuff gets retranslated. By a lot. There's no obvious way to predict this or control it. And the software clock inside your emulated system can lie to you about it if timer interrupts get deferred. All this should pretty much average out if you do something big with lots of execs (like build a linux kernel from source). But if you do something small expect serious butterfly effects. Expect microbenchmarks to swing around wildly. Quick analogy: you know the performance difference faulting your executable in from disk vs running it out of cache? Imagine a daemon that makes random intermittent calls to echo 1 /proc/sys/vm/drop_caches, and now try to do a sane benchmark. No matter what you use to measure, what you're measuring isn't going to be consistent from one run to the next. Performance should be better (and more stable) with kqemu or kvm. Maybe that you can benchmark sanely, I wouldn't know. Ask somebody else. :) P.S. Take the above with a large grain of salt, I'm not close to an expert in this area... :-) Ok. What you've said pretty much covers how I've made up my mind in the last couple of hours trying to think about the problem *g* Guess I'll have to be happy counting TLB misses and page faults, adding up executed instructions (in user/kernel mode) per process and doing some timing stuff... then running the examples a lot of times, making an average of all numbers and finally just ignoring them since I *know* that they are bogus ;-) No, seriously... I understand the problem, but I think the above is the best I can do since I'm really only interested in the effekt it has on QEMU for the moment :-) Thanks again for your ideas!!
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Andreas Färber wrote: Am 04.01.2008 um 15:00 schrieb Samuel Thibault: Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit : Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: Carlo Marcelo Arenas Belon wrote: Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write are from an array of the uint8_t type Do we have a host where this actually makes a difference? I believe Perl makes sizeof(char) checks, so there likely is some platform where sizeof(char) 1. The C standard says `When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1.' AFAIR this is C99 ... The standard maybe. But Win64 violates the C standards, too. ;) According to our department's ANSI C course, the only consistent rule is sizeof(char) = sizeof(short) = sizeof(int) = sizeof(long) without any concrete numbers. ... and this is C89. Thiemo
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
In message: [EMAIL PROTECTED] Andreas_Färber [EMAIL PROTECTED] writes: : : Am 04.01.2008 um 15:00 schrieb Samuel Thibault: : : Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit : : : Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: : : Carlo Marcelo Arenas Belon wrote: : Trivial fix that ensures that all buffers used for bdrv_read or : bdrv_write : are from an array of the uint8_t type : : Do we have a host where this actually makes a difference? : : I believe Perl makes sizeof(char) checks, so there likely is some : platform where sizeof(char) 1. : : The C standard says : : `When applied to an operand that has type char, unsigned char, or : signed : char, (or a qualified version thereof) the result is 1.' : : The standard maybe. But Win64 violates the C standards, too. ;) : : According to our department's ANSI C course, the only consistent rule is : sizeof(char) = sizeof(short) = sizeof(int) = sizeof(long) : without any concrete numbers. : : I'm not saying it should be changed or not in QEMU, just saying it may : not be completely out-of-the-world. The problem is that sizeof(char) has to be 1, or a number of things will down right fail. The following code will fail if it doesn't: char *a = abc; char *b = malloc(strlen(a) + 1); strcpy(b, a); If sizeof(char) is really 2, then sizeof(abc) is going to be 6, not 3 that strlen returns and the strcpy will smash into memory it doesn't own. And *NOBODY*, not even ignorant students, adds a '* sizeof(char)' to the strlen. Such a compiler would break just about every program out there of any significant size. Now, there's a wchar_t which is the type of the characters in a L string: abcL can be 4, 8, 12, 16 or more bytes in size, but that's different, and not what's being discussed. Warner
[Qemu-devel] qemu/target-mips helper.c
CVSROOT:/sources/qemu Module name:qemu Changes by: Thiemo Seufer ths 08/01/04 17:52:57 Modified files: target-mips: helper.c Log message: Handle some more exception types. CVSWeb URLs: http://cvs.savannah.gnu.org/viewcvs/qemu/target-mips/helper.c?cvsroot=qemur1=1.62r2=1.63
Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c
On Friday 04 January 2008, Markus Hitter wrote: Am 03.01.2008 um 15:02 schrieb Paul Brook: Having to check every return value is extremely tedious and (as you've proved) easy to miss. Checking every return value is a measure for programming reliable code. Never failing is even more reliable. If the allocation fails we don't have any viable alternatives, so we may as well stop right there. Stop != segfault? Yes. What about a meaningful exit message? Out of memory is a fairly comprehensive description of the problem. In fact I'd say it's much more informative than random widget the user doesn't know or care about failed to initialize. Paul
Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c
Am 03.01.2008 um 15:02 schrieb Paul Brook: Having to check every return value is extremely tedious and (as you've proved) easy to miss. Checking every return value is a measure for programming reliable code. If the allocation fails we don't have any viable alternatives, so we may as well stop right there. Stop != segfault? What about a meaningful exit message? What if the code doesn't segfault but runs havok? Markus - - - - - - - - - - - - - - - - - - - Dipl. Ing. Markus Hitter http://www.jump-ing.de/
Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c
Paul Brook wrote: What about a meaningful exit message? Out of memory is a fairly comprehensive description of the problem. In fact I'd say it's much more informative than random widget the user doesn't know or care about failed to initialize. If the user requested a target parameter that is beyond the capabilities of the host system, a meaningful error message can be generated that instructs the user on how to possibly correct the problem.
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
Andreas Färber [EMAIL PROTECTED] writes: Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: Carlo Marcelo Arenas Belon wrote: Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write are from an array of the uint8_t type Do we have a host where this actually makes a difference? I believe Perl makes sizeof(char) checks, so there likely is some platform where sizeof(char) 1. Not in C. sizeof(char) == 1 by definition. Also CHAR_BIT = 8, so uint8_t can never be wider than char. Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
[Qemu-devel] qemu Makefile.target hw/mips_malta.c hw/pflash_...
CVSROOT:/sources/qemu Module name:qemu Changes by: Thiemo Seufer ths 08/01/04 19:11:32 Modified files: . : Makefile.target hw : mips_malta.c pflash_cfi01.c Log message: Malta flash support. CVSWeb URLs: http://cvs.savannah.gnu.org/viewcvs/qemu/Makefile.target?cvsroot=qemur1=1.236r2=1.237 http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mips_malta.c?cvsroot=qemur1=1.53r2=1.54 http://cvs.savannah.gnu.org/viewcvs/qemu/hw/pflash_cfi01.c?cvsroot=qemur1=1.4r2=1.5
Re: [Qemu-devel] qemu cpu-all.h exec.c
On 1/3/08, Paul Brook [EMAIL PROTECTED] wrote: As I said earlier, the only correct way to handle memory accesses is to be able to consider a memory range and its associated I/O callbacks as an object which can be installed _and_ removed. It implies that there is a priority system close to what you described. It is essential to correct long standing PCI bugs for example. This should be feasible, though raises a few questions. Does this mean another API for stacked registration, or should stacking happen automatically with current API? A new function is needed for removal. What could be the API for setting priorities? How would multiple layers be enabled for multiple devices at same location? How can a higher level handler pass the request to lower one? Do we need a status return for access handler? I don't think passing through requests to the next handler is an interesting use case. Just consider a device to handle all accesses within its defined region. If an overlapping region is accessed then at best you're into highly machine dependent behavior. The only interesting case I can think of is x86 where a PCI region may be overlayed on top of RAM. A single level of priority (ram/rom vs. everything else) is probably sufficient for practical purposes. The most important thing is that when one of the mappings is removed, subsequent accesses to the previously overlapped region hit the remaining device. The difference between passive stacking and active should be minimal and not visible to the devices. A few use cases: Partial width device unassigned ROM RAM unassigned SBus controller EBus controller Device unassigned Other direction (for future expansion): Device DMA controller SBus controller IOMMU RAM unassigned I think these are different things: - Registering multiple devices within the same address space. - Mapping access from one address sapce to annother. Currently qemu does neither. The former is what Fabrice is talking about. Right, but if we have this active stacking, address translation could be a possible future extension of this mode. The latter depends how general you want the solution to be. One possibility is for the device DMA+registration routines map everything onto CPU address space. Interesting idea, do you mean that all individual bus address spaces could exist in system view in the same large address space outside the target CPU address space? Then some of the translations could become simple offset operations.
[Qemu-devel] gen_op* function definitions
I'm trying to figure out how the translation blocks are generated and I'm having a bit of difficulty. I'm trying to find and modify a particular instruction rep ins*, which I've found and it looks like it's being broken down into simpler instructions in the translation process. I've followed it all the way down to the most basic instructions, the first of which is gen_op_movl_A0_reg[EDI](), which translates to gen_op_movl_A0_EDI(). This is where I'm stuck, I can't find the definition for this function, or any of the gen_op* instructions for that matter anywhere in the qemu source. I must be missing something, can someone point me in the right direction to find the definitions for the gen_op* functions. Thanks, Ryan
Re: [Qemu-devel] gen_op* function definitions
On 1/4/08, Ryan W Smith [EMAIL PROTECTED] wrote: I'm trying to figure out how the translation blocks are generated and I'm having a bit of difficulty. I'm trying to find and modify a particular instruction rep ins*, which I've found and it looks like it's being broken down into simpler instructions in the translation process. I've followed it all the way down to the most basic instructions, the first of which is gen_op_movl_A0_reg[EDI](), which translates to gen_op_movl_A0_EDI(). This is where I'm stuck, I can't find the definition for this function, or any of the gen_op* instructions for that matter anywhere in the qemu source. I must be missing something, can someone point me in the right direction to find the definitions for the gen_op* functions. op.c is compiled and the resulting object file op.o is processed by dyngen program, producing gen-op.h, opc.h, and op.h. These define the gen_op* versions of the functions, originally op_something in op.c.
Re: [Qemu-devel] qemu cpu-all.h exec.c
The latter depends how general you want the solution to be. One possibility is for the device DMA+registration routines map everything onto CPU address space. Interesting idea, do you mean that all individual bus address spaces could exist in system view in the same large address space outside the target CPU address space? Then some of the translations could become simple offset operations. No, I was basically assuming that all cpu-device mappings are linear offsets. This means you need almost no changes to the current CPU access code. You can also arrange for all device DMA requests to be translated into CPU physical addresses (VIA IOMMU, or whatever), then treat them the same as if they were CPU accesses. However on second thoughts this probably isn't such a clever idea. There are some potentially interesting cases it can't handle. I'll see if I can come up with an actual proposal. My current theory is that we should be able to combine the bus mappings with the TLB fill, which should help mitigate the overhead. Paul
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
On Friday 04 January 2008 07:41:29 Andreas Färber wrote: Am 04.01.2008 um 14:20 schrieb Thiemo Seufer: Carlo Marcelo Arenas Belon wrote: Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write are from an array of the uint8_t type Do we have a host where this actually makes a difference? I believe Perl makes sizeof(char) checks, so there likely is some platform where sizeof(char) 1. There's a difference between what some now-obsolete HP minicomputer once did in 1987 and an interesting system with nonzero potential deployments. A system with sizeof(char)!=1 does not fall in the second category. In fact, on Unix, short, int, and long all have defined sizes too. Standard: http://www.unix.org/whitepapers/64bit.html Rationale document: http://www.unix.org/version2/whatsnew/lp64_wp.html Infrastructure in search of a user always bit rots. Wait for somebody to complain, and _then_ add it, once a user has shown up who can test it (and detect its absence). Andreas Rob -- One of my most productive days was throwing away 1000 lines of code. - Ken Thompson.
Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c
On modern operating systems, allocations only return zero when you exhaust virtual memory. Returning nonzero doesn't mean you have enough memory, because it's given you a redundant copy on write mapping of the zero page and will fault in physical pages when you write to 'em, which has _no_ return value. Instead, the out of memory killer will shoot your program in the head in the middle of it's run Decent operating systems allow the system administrator gets to choose how optimistic memory allocation is. You're describing wildly-optimistic mode, which is often but not always the default. Paul
[Qemu-devel] [PATCH] sparc32: fix per cpu counter/timer
Sun4m SMP machines support a maximum of 4 CPUs. Linux
knows this and uses fixed size arrays for per-cpu counter/timers
and interrupt controllers. Sun4m uni-processor machines use
the slaveio chip which has a single per-cpu counter/timer
and interrupt controller. However it does not fully decode the
address so the same counter/timer or interrupt controller can
be accesses from multiple addresses.
This patch changes the per-cpu counter/timer to work the way
the real hardware works: 4 per-cpu counter/timers for SMP and
1 counter/timer for UP mapped at multiple addresses.
This patch also fixes a number of per-cpu user timer bugs:
limit bit set when limit reached, count saved and used when
written, limit bit reset on count write and system timer configuration
register updated properly for per-cpu user timer mode.
Sun4d currently uses the sun4m counter/timer code. They are
simular but not the same. This patch will break the broken
sun4d implementation further. The real fix is to create a proper
sun4d counter/timer implementation. Since the sun4d implementation
doesn't currently work anyway, this shouldn't be an issue.
Index: hw/sbi.c
===
RCS file: /sources/qemu/qemu/hw/sbi.c,v
retrieving revision 1.2
diff -p -u -r1.2 sbi.c
--- hw/sbi.c1 Jan 2008 17:06:38 - 1.2
+++ hw/sbi.c5 Jan 2008 00:43:27 -
@@ -34,15 +34,13 @@ do { printf(IRQ: fmt , ##args); } whi
#define DPRINTF(fmt, args...)
#endif
-#define MAX_CPUS 16
-
#define SBI_NREGS 16
typedef struct SBIState {
uint32_t regs[SBI_NREGS];
-uint32_t intreg_pending[MAX_CPUS];
-qemu_irq *cpu_irqs[MAX_CPUS];
-uint32_t pil_out[MAX_CPUS];
+uint32_t intreg_pending[MAX_SUN4D_CPUS];
+qemu_irq *cpu_irqs[MAX_SUN4D_CPUS];
+uint32_t pil_out[MAX_SUN4D_CPUS];
} SBIState;
#define SBI_SIZE (SBI_NREGS * 4)
@@ -107,7 +105,7 @@ static void sbi_save(QEMUFile *f, void *
SBIState *s = opaque;
unsigned int i;
-for (i = 0; i MAX_CPUS; i++) {
+for (i = 0; i MAX_SUN4D_CPUS; i++) {
qemu_put_be32s(f, s-intreg_pending[i]);
}
}
@@ -120,7 +118,7 @@ static int sbi_load(QEMUFile *f, void *o
if (version_id != 1)
return -EINVAL;
-for (i = 0; i MAX_CPUS; i++) {
+for (i = 0; i MAX_SUN4D_CPUS; i++) {
qemu_get_be32s(f, s-intreg_pending[i]);
}
sbi_check_interrupts(s);
@@ -133,7 +131,7 @@ static void sbi_reset(void *opaque)
SBIState *s = opaque;
unsigned int i;
-for (i = 0; i MAX_CPUS; i++) {
+for (i = 0; i MAX_SUN4D_CPUS; i++) {
s-intreg_pending[i] = 0;
}
sbi_check_interrupts(s);
@@ -150,7 +148,7 @@ void *sbi_init(target_phys_addr_t addr,
if (!s)
return NULL;
-for (i = 0; i MAX_CPUS; i++) {
+for (i = 0; i MAX_SUN4D_CPUS; i++) {
s-cpu_irqs[i] = parent_irq[i];
}
@@ -160,7 +158,7 @@ void *sbi_init(target_phys_addr_t addr,
register_savevm(sbi, addr, 1, sbi_save, sbi_load, s);
qemu_register_reset(sbi_reset, s);
*irq = qemu_allocate_irqs(sbi_set_irq, s, 32);
-*cpu_irq = qemu_allocate_irqs(sbi_set_timer_irq_cpu, s, MAX_CPUS);
+*cpu_irq = qemu_allocate_irqs(sbi_set_timer_irq_cpu, s, MAX_SUN4D_CPUS);
sbi_reset(s);
return s;
Index: hw/slavio_intctl.c
===
RCS file: /sources/qemu/qemu/hw/slavio_intctl.c,v
retrieving revision 1.29
diff -p -u -r1.29 slavio_intctl.c
--- hw/slavio_intctl.c 1 Jan 2008 20:57:25 - 1.29
+++ hw/slavio_intctl.c 5 Jan 2008 00:43:27 -
@@ -46,21 +46,20 @@ do { printf(IRQ: fmt , ##args); } whi
*
*/
-#define MAX_CPUS 16
#define MAX_PILS 16
typedef struct SLAVIO_INTCTLState {
-uint32_t intreg_pending[MAX_CPUS];
+uint32_t intreg_pending[MAX_SUN4M_CPUS];
uint32_t intregm_pending;
uint32_t intregm_disabled;
uint32_t target_cpu;
#ifdef DEBUG_IRQ_COUNT
uint64_t irq_count[32];
#endif
-qemu_irq *cpu_irqs[MAX_CPUS];
+qemu_irq *cpu_irqs[MAX_SUN4M_CPUS];
const uint32_t *intbit_to_level;
uint32_t cputimer_lbit, cputimer_mbit;
-uint32_t pil_out[MAX_CPUS];
+uint32_t pil_out[MAX_SUN4M_CPUS];
} SLAVIO_INTCTLState;
#define INTCTL_MAXADDR 0xf
@@ -84,7 +83,7 @@ static uint32_t slavio_intctl_mem_readl(
uint32_t saddr, ret;
int cpu;
-cpu = (addr (MAX_CPUS - 1) * TARGET_PAGE_SIZE) 12;
+cpu = (addr (MAX_SUN4M_CPUS - 1) * TARGET_PAGE_SIZE) 12;
saddr = (addr INTCTL_MAXADDR) 2;
switch (saddr) {
case 0:
@@ -105,7 +104,7 @@ static void slavio_intctl_mem_writel(voi
uint32_t saddr;
int cpu;
-cpu = (addr (MAX_CPUS - 1) * TARGET_PAGE_SIZE) 12;
+cpu = (addr (MAX_SUN4M_CPUS - 1) * TARGET_PAGE_SIZE) 12;
saddr = (addr INTCTL_MAXADDR) 2;
DPRINTF(write cpu %d reg 0x TARGET_FMT_plx = %x\n, cpu, addr, val);
switch (saddr) {
@@ -190,7 +189,7 @@ static void
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote:
Can someone please comment on the mergability of this patch? or in what
needs to be done to it so that it can be committed?
The patch is still current and the bug it fixes would otherwise prevent
OpenSolaris guests to be installed in qemu. the MMC-6 command it fixes
(GET CONFIGURATION) has been verified to behave as per the SPECs and
compared to behave just like real hardware does.
Carlo
Well, I'm no expert but the patch is small enough that I can read it.
padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */
-padstr((char *)(p + 27), QEMU CD-ROM, 40); /* model */
+padstr((char *)(p + 27), QEMU DVD-ROM, 40); /* model */
put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM)
*/ #ifdef USE_DMA_CDROM
put_le16(p + 49, 1 9 | 1 8); /* DMA and LBA supported */
@@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s)
buf[6] = 0; /* reserved */
buf[7] = 0; /* reserved */
padstr8(buf + 8, 8, QEMU);
-padstr8(buf + 16, 16, QEMU CD-ROM);
+padstr8(buf + 16, 16, QEMU DVD-ROM);
padstr8(buf + 32, 4, QEMU_VERSION);
I take it Solaris is actually checking these strings? Or is this a cosmetic
change? (Not a _bad_ change, I'm just curious.)
@@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s)
ASC_INV_FIELD_IN_CMD_PACKET);
break;
}
-memset(buf, 0, 32);
This moved a few lines down, but that just seems to be churn.
+max_len = ube16_to_cpu(packet + 7);
Why are you doing math on a value _before_ you adjust its endianness from a
potentially foreign format into the CPU native one? I take it that gives you
a pointer from which a 16 byte value is fetched?
bdrv_get_geometry(s-bs, total_sectors);
Calculating stuff to use later rather than modifying buf[]. Ok.
-buf[3] = 16;
The new code doesn't directly set buf[3] anymore, leaving it 0 from the
memset. I take it this is now handled by the cpu_to_ube32() targeting 4
bytes of buf[] much later?
-buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current
profile */
This becomes 3-state now. You added a state 0 when there's no cdrom in the
drive. The less intrusive change would be keeping the above line and adding
a second line: if (!total_sectors) buf[7] = 0; Not actually any larger,
codewise, and a less intrusive change.
Where does the constant come from, anyway?
-buf[10] = 0x10 | 0x1;
This turns into 0x02 | 0x01 further down. Why the change?
-buf[11] = 0x08; /* size of profile list */
Set to the same value later, just a comment change. Ok.
+memset(buf, 0, 32);
+if (total_sectors) {
+if (total_sectors 1433600) {
+buf[7] = 0x10; /* DVD-ROM */
+} else {
+buf[7] = 0x08; /* CD-ROM */
+}
+} else {
+buf[7] = 0x00; /* no current profile */
+}
+buf[10] = 0x02 | 0x01; /* persistent and current */
+buf[11] = 0x08; /* size of profile list = 4 bytes per
profile */
Commented on already, above.
buf[13] = 0x10; /* DVD-ROM profile */
This is new. This means drive is DVD capable, not DVD is in drive,
correct?
-buf[14] = buf[7] == 0x10; /* (in)active */
+buf[14] = buf[13] == buf[7]; /* (in)active */
Um... Ok? This change is a NOP? buf[13] is always 0x10 due to the previous
line, so you're always comparing with 0x10...
The result seems to indicate a dvd is in the drive, in a yes/no fashion
rather than looking for 0x10 in position 7. I'll assume the spec requires
this?
buf[17] = 0x08; /* CD-ROM profile */
-buf[18] = buf[7] == 0x08; /* (in)active */
-ide_atapi_cmd_reply(s, 32, 32);
+buf[18] = buf[17] == buf[7]; /* (in)active */
Again, the added line is not actually a change. What's the difference?
+len = 8 + 4 + buf[11]; /* headers + size of profile list */
Once again, buf[11] is a constant (0x08) that you just set above. So this is
actually a constant value, but unless the constant propagation is good enough
to track individual array members, you're forcing the machine to do
unnecessary math. Is there a reason for this?
+cpu_to_ube32(buf, len - 4); /* data length */
Here's what replaced the assignment to buf[3] above. This might be the meat
of the patch?
+ide_atapi_cmd_reply(s, len, max_len);
And this moved down from the call with (s, 32, 32) two hunks back. You just
calculated len (much unless I missed something must _always_ be 20), but
max_len was calculated above as:
max_len = ube16_to_cpu(packet + 7);
So max_len
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote: On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote: -buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current profile */ Where does the constant come from, anyway? 1433600? Seems it's the number of 512 KiB blocks in a 700 MiB CD image (700 * 1024 * 2). -- Stuart Brady
Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter
On Fri, Jan 04, 2008 at 01:20:39PM +, Thiemo Seufer wrote:
Carlo Marcelo Arenas Belon wrote:
Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
are from an array of the uint8_t type
Do we have a host where this actually makes a difference?
No that I know of (even if I'd heard horror stories about some bizarre old
architecure where sizeof(char) was 3 which I hope I never ever find),
and sorry for stirring this long thread by not being clear about my objective,
and not coming to clarify it before as I had no access until now to my email.
as you said this patch makes no difference in the logic at all in any
architecture that qemu uses (which is why I said it was a trivial fix)
but is IMHO a more correct version of the code because it is consistently
using the same type everywhere instead of different types with different names
in different places, even if they have the same storage requirements; after
all there is a reason why bdrv_read and bdrv_write where defined as using
(uint8_t *) for their buffer parameter since version 1.1 of the vl.h file.
I came to look for it when a merge conflict in kvm flipped the second
invocation from block.c into an unsigned char *sector[512] instead as you
can see by the proposed fix here :
http://article.gmane.org/gmane.comp.emulators.kvm.devel/11510
Carlo
---
Index: block-vvfat.c
===
RCS file: /sources/qemu/qemu/block-vvfat.c,v
retrieving revision 1.16
diff -u -p -r1.16 block-vvfat.c
--- block-vvfat.c 24 Dec 2007 13:26:04 - 1.16
+++ block-vvfat.c 4 Jan 2008 07:57:20 -
@@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
int current_fd;
mapping_t* current_mapping;
unsigned char* cluster; /* points to current cluster */
-unsigned char* cluster_buffer; /* points to a buffer to hold temp data
*/
+uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
unsigned int current_cluster;
/* write support */
Index: block.c
===
RCS file: /sources/qemu/qemu/block.c,v
retrieving revision 1.53
diff -u -p -r1.53 block.c
--- block.c 24 Dec 2007 16:10:43 - 1.53
+++ block.c 4 Jan 2008 07:57:21 -
@@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
BlockDriver *drv = bs-drv;
int64_t i, total_sectors;
int n, j;
-unsigned char sector[512];
+uint8_t sector[512];
if (!drv)
return -ENOMEDIUM;
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Sat, Jan 05, 2008 at 01:02:30AM +, Stuart Brady wrote: 1433600? Seems it's the number of 512 KiB blocks in a 700 MiB CD image (700 * 1024 * 2). Sorry, I mean 512 *byte* blocks. -- Stuart Brady
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Sat, Jan 05, 2008 at 01:02:30AM +, Stuart Brady wrote: On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote: On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote: -buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current profile */ Where does the constant come from, anyway? 1433600? Seems it's the number of 512 KiB blocks in a 700 MiB CD image (700 * 1024 * 2). yes, that magic constant corresponds to the maximum expected size for a CD-ROM and is used to detect if the profile used should be for DVD-ROM or not. It came from the original implementation patch for GET CONFIGURATION committed in revision 1.66 of ide.c by Filip Navarra as Partial IDE DVD emulation http://cvs.savannah.nongnu.org/viewvc/qemu/hw/ide.c?root=qemur1=1.65r2=1.66 Carlo
Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c
On Friday 04 January 2008 19:07:58 Paul Brook wrote: On modern operating systems, allocations only return zero when you exhaust virtual memory. Returning nonzero doesn't mean you have enough memory, because it's given you a redundant copy on write mapping of the zero page and will fault in physical pages when you write to 'em, which has _no_ return value. Instead, the out of memory killer will shoot your program in the head in the middle of it's run Decent operating systems allow the system administrator gets to choose how optimistic memory allocation is. You're describing wildly-optimistic mode, which is often but not always the default. True, but if you completely disable overcommit then fork() a large process and exec() a small one, you haven't got enough memory even though you're not really _using_ any to do so. You can disable overcommit and give the system an egregious amount of swap space, but then your pathological case is the system going into swap thrashing la-la land and essentially freezing (advancing at 0.1% of its normal rate, if that, for _hours_) instead of killing some runaway processes (or rebooting) and recovering. Not necessarily and improvement, especially if you're the one with the pager. It is alas, not a simple problem to get right. fork() and exec() being separate system calls isn't always an improvement over a combined one. (Espeically since exec() needs a file, not a file handle. You can't re-exec your current process unless you can find and reopen it, you can't exec() from a pipe... And then there's nommu vfork(), always fun...) Paul Rob -- One of my most productive days was throwing away 1000 lines of code. - Ken Thompson.
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote: On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote: Can someone please comment on the mergability of this patch? or in what needs to be done to it so that it can be committed? The patch is still current and the bug it fixes would otherwise prevent OpenSolaris guests to be installed in qemu. the MMC-6 command it fixes (GET CONFIGURATION) has been verified to behave as per the SPECs and compared to behave just like real hardware does. Carlo Well, I'm no expert but the patch is small enough that I can read it. thanks padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware version */ -padstr((char *)(p + 27), QEMU CD-ROM, 40); /* model */ +padstr((char *)(p + 27), QEMU DVD-ROM, 40); /* model */ put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */ #ifdef USE_DMA_CDROM put_le16(p + 49, 1 9 | 1 8); /* DMA and LBA supported */ @@ -1634,12 +1634,13 @@ static void ide_atapi_cmd(IDEState *s) buf[6] = 0; /* reserved */ buf[7] = 0; /* reserved */ padstr8(buf + 8, 8, QEMU); -padstr8(buf + 16, 16, QEMU CD-ROM); +padstr8(buf + 16, 16, QEMU DVD-ROM); padstr8(buf + 32, 4, QEMU_VERSION); I take it Solaris is actually checking these strings? Or is this a cosmetic change? (Not a _bad_ change, I'm just curious.) this is just cosmetic. in 2 of the RESENDs I did it was actually a second optional patch on a series of 2. this RESEND had it all together just because I though I had probably a better chance of having it reviewed if it was 1 mail instead of 3 (including the patch series description) and since I got mostly no feedback until now. the real fix is on the GET CONFIGURATION implementation which is done below. @@ -1648,17 +1649,27 @@ static void ide_atapi_cmd(IDEState *s) ASC_INV_FIELD_IN_CMD_PACKET); break; } -memset(buf, 0, 32); This moved a few lines down, but that just seems to be churn. it is structured in a way that could be later structured away into a function when/if more profiles are added. this way the first part of the code reads the input parameters and initialized the buffer it will use later to generate a response in the logical order. +max_len = ube16_to_cpu(packet + 7); Why are you doing math on a value _before_ you adjust its endianness from a potentially foreign format into the CPU native one? I take it that gives you a pointer from which a 16 byte value is fetched? yes, this adds not to the value but the pointer where the packet is stored and that contains on byte 7 (MSB) and 8 (LSB) the value for the Allocation Length parameter as described in Table 275 of T10/1836-D Revision 1 (*) bdrv_get_geometry(s-bs, total_sectors); Calculating stuff to use later rather than modifying buf[]. Ok. I just kept this from the original patch, but it could be extracted instead from s-nb_sectors as well, but though it would be probably easier if I just kept the unrelated changes to a minimum. -buf[3] = 16; The new code doesn't directly set buf[3] anymore, leaving it 0 from the memset. I take it this is now handled by the cpu_to_ube32() targeting 4 bytes of buf[] much later? yes, the response as described in Table 277 of T10/1836-D Revision 1 contains a 4 byte Data Length field (LSB is byte 3) and I am calculating it at the end instead of hardcoding it at the beginning, so that this code could adapt if later extended to add more profiles (CD-RW, HD-DVD, ..). -buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current profile */ This becomes 3-state now. You added a state 0 when there's no cdrom in the drive. The less intrusive change would be keeping the above line and adding a second line: if (!total_sectors) buf[7] = 0; Not actually any larger, codewise, and a less intrusive change. I refactored this code so that it is hopefully more clear in the intended effect, which is to set the default profile to either of the available profiles depending on the kind of media that was available, and as it is done by real hardware. Again, even if the refactoring was more of an intrusive change, it also allows for more flexibility to expand this code to support more profiles in a cleaner way. -buf[10] = 0x10 | 0x1; This turns into 0x02 | 0x01 further down. Why the change? the original implementation got the bits to be enabled in the flags wrong, 0x10 is part of the Version Field and is meant to be 0 as detailed in table 87 of T10/1836-D Revision 1. for the type of query issued the response flags returned were meant to be persistent and current and that corresponds to the bit 1 and 0 of byte 2 respectively. -buf[11] = 0x08; /* size of profile list */
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
On Friday 04 January 2008 19:02:30 Stuart Brady wrote: On Fri, Jan 04, 2008 at 06:25:25PM -0600, Rob Landley wrote: On Friday 04 January 2008 04:02:07 Carlo Marcelo Arenas Belon wrote: -buf[7] = total_sectors = 1433600 ? 0x08 : 0x10; /* current profile */ Where does the constant come from, anyway? 1433600? Seems it's the number of 512 KiB blocks in a 700 MiB CD image (700 * 1024 * 2). Except that according to http://en.wikipedia.org/wiki/CD-ROM it's actually 703 and 1/8 binary megabytes (360,000 sectors *2048 bytes), which would be 144. And we all know wikipedia's never wrong: http://www.theonion.com/content/node/50902 Rob -- One of my most productive days was throwing away 1000 lines of code. - Ken Thompson.
