qemu-devel
[Top][All Lists]
Advanced

[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: Carlo Marcelo Arenas Belon
Subject: Re: [Qemu-devel] [RESEND] [PATCH] ide: fix GET_CONFIGURATION DVD-ROM support
Date: Fri, 4 Jan 2008 21:36:41 -0600
User-agent: Mutt/1.4.1i

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:
> > 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.

thanks

> > >      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.)

this is just cosmetic.

in 2 of the RESENDs I did it was actually a second optional patch on a series
of 2.

this RESEND had it all together just because I though I had probably a better
chance of having it reviewed if it was 1 mail instead of 3 (including the
patch series description) and since I got mostly no feedback until now.

the real fix is on the "GET CONFIGURATION" implementation which is done below.

> > > @@ -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.

this way the first part of the code reads the input parameters and initialized
the buffer it will use later to generate a response in the logical order.

> > > +            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 (*)

> > >              bdrv_get_geometry(s->bs, &total_sectors);
> 
> Calculating stuff to use later rather than modifying buf[].  Ok.

I just kept this from the original patch, but it could be extracted instead
from s->nb_sectors as well, but though it would be probably easier if I just
kept the unrelated changes to a minimum.

> > > -            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, ..).

> > > -            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.

> > > -            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.

for the type of query issued the response flags returned were meant to be
persistent and current and that corresponds to the bit 1 and 0 of byte 2
respectively.

> > > -            buf[11] = 0x08; /* size of profile list */
> 
> Set to the same value later, just a comment change.  Ok.

yes this was part of the refactoring of this code to be clearer on the
intention and follow the documentation in an organized way.

> > > +            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.

this header is described in Table 86 for R10/1836-D Revision 1

> > > buf[13] = 0x10; /* DVD-ROM profile */ 
> 
> This is new.  This means "drive is DVD capable", not "DVD is in drive", 
> correct?

correct, "DVD is in drive" is set by the next byte which checks if the profile
is active or not by looking at the "Current Profile" in buf[7] and matching it
with the profile identification in this byte.

> > > -            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...

see below

> 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?

yes, the SPEC requires redundant information by having the "Current Profile"
indicate which kind of profile the device is, and also by indicating in the
list of profiles that the Profile actually in use has its "active" bit
enabled.

the "Profile Description" data is described in Table 90 of T10/1836-D Revision
1

> > >              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"

Of course as I said before, setting up profiles and marking which one is
active could be later be done through a macro or inline function instead just
by reproducing the logic above.

> > > +            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 ;)

> > > +            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

> 
> > > +            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?

yes

> 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.

> I can test this patch and see if it boots my indiana ISO, assuming I can find 
> said ISO...

good luck, and thanks

Carlo

(*) MMC6 DRAFT availeble at http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf




reply via email to

[Prev in Thread] Current Thread [Next in Thread]