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

[Rob Landley]

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.