From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JAwr0-0004YU-GH for qemu-devel@nongnu.org; Fri, 04 Jan 2008 19:25:34 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JAwqx-0004Xk-01 for qemu-devel@nongnu.org; Fri, 04 Jan 2008 19:25:33 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JAwqw-0004Xh-Qz for qemu-devel@nongnu.org; Fri, 04 Jan 2008 19:25:30 -0500 Received: from static-71-162-243-5.phlapa.fios.verizon.net ([71.162.243.5] helo=grelber.thyrsus.com) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JAwqw-0004zR-Mg for qemu-devel@nongnu.org; Fri, 04 Jan 2008 19:25:30 -0500 From: Rob Landley Subject: Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support Date: Fri, 4 Jan 2008 18:25:25 -0600 References: <20071226073615.GB25052@tapir> <20080104100207.GC9968@tapir> In-Reply-To: <20080104100207.GC9968@tapir> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200801041825.26304.rob@landley.net> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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 is adjusted for endianness, but len isn't? Presumably because max_len came from the packet, but len is calculated? > > break; > > } > > default: Well, I had several questions but it seems vaguely sane. I assume you tested it and that solaris does indeed boot now. I encountered the same hang trying out the first GNU/Solaris bootable CD ("indiana") wandered by on LWN. I was curious about booting GNU/Solaris, since popularizing that name would probably annoy Richard Stallman. I can test this patch and see if it boots my indiana ISO, assuming I can find said ISO... Rob -- "One of my most productive days was throwing away 1000 lines of code." - Ken Thompson.