[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM sup
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 |
User-agent: |
KMail/1.9.6 (enterprise 0.20070907.709405) |
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.
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Carlo Marcelo Arenas Belon, 2008/01/04
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support,
Rob Landley <=
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Stuart Brady, 2008/01/04
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Stuart Brady, 2008/01/04
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Carlo Marcelo Arenas Belon, 2008/01/04
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Rob Landley, 2008/01/04
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Stuart Brady, 2008/01/05
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Carlo Marcelo Arenas Belon, 2008/01/05
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Stuart Brady, 2008/01/06
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Andreas Färber, 2008/01/06
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Carlo Marcelo Arenas Belon, 2008/01/06
- Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support, Carlo Marcelo Arenas Belon, 2008/01/06