Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support

[Rob Landley]

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.