I re-downloaded the GNU/Solaris preview CD, linked from here: http://lwn.net/Articles/256737/
And fired it up: qemu -cdrom solaris-preview.iso -boot d -m 256 Note that it won't boot with the default 128 megs, because Solaris is a pig. Without the patch at the start of this thread, the GNU/Solaris boot hangs. With this patch, it boots just fine. On Friday 04 January 2008 21:36:41 Carlo Marcelo Arenas Belon 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: > > > > 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. Ok. > > > > @@ -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. Makes sense. > > > > + 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 (*) Is dereferencing a word value at an odd address alignment safe on all the potential host platforms? (I dunno if qemu runs on an arm host, nor if the ube16_to_cpu value is already dealing with this. Just curious.) > > > > - 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, ..). Ok. > > > > - 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. I take it buf[7]=0 is what real hardware returns when there's no disk in the drive? > > > > - 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. Explicit bug fix. Check. > > > > 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? > > I think it is clearer this way, and matches better the wording of the > specification which says : > > "The CurrentP bit when set to one, shall indicate that this profile is > currently active. If no mediums is present, no profile should be active" Not sure how that part addresses the question, but you address it in the next hunk. > > > > + 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? > > It is clearer on why the size of the response includes the profile > definitions plus the 2 headers, remember that buf[11] is now a constant > because we are defining only 2 profiles by hand, but the aim was to clean > the code and make it extensible (and I hoped self explanatory) even if it > makes the computer do some math or is not as compact as the original one, > after all this code doesn't need to be optimized for speed and, well I > trust compilers ;) I.E. if the value gets set in a more complicated way, it propagates naturally to all the places it needs to go. *shrug* I suppose I can see trying to have fewer instances of each magic constant... > > > > + 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? > > as you saw there were several changes that were required overall to the > previous implementation and that I described probably better in the > original post as can be seen in : > > http://lists.gnu.org/archive/html/qemu-devel/2007-11/msg00852.html I hadn't read the original patch, but after testing the GNU/Solaris preview CD I'm fairly happy with the patch. > > 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. > > yes I'd tested and has been distributed with the Gentoo unofficial packages > of kvm I maintain for more than a month and committed into KVM for the last > 2 releases. The patch works for me. > > I can test this patch and see if it boots my indiana ISO, assuming I can > > find said ISO... > > good luck, and thanks I did, and GNU/Solaris successfully booted to a login prompt with your patch. (It didn't give me the promised Gnome desktop but stayed in text mode, and when I guessed "root" it wanted a password that the documentation doesn't mention, but that's Sun's fault, not qemu's.) > Carlo > > (*) MMC6 DRAFT availeble at > http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf Rob -- "One of my most productive days was throwing away 1000 lines of code." - Ken Thompson.