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.