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: Sun, 6 Jan 2008 22:47:39 -0600
User-agent: Mutt/1.4.1i

On Sun, Jan 06, 2008 at 03:22:15AM -0600, Rob Landley wrote:
> > > > > @@ -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.

I should also said that unless done after as shown by the patch, reading the
parameter will be imposible, because the buffer used for the IDE commands is
reused for input/output as shown by lines 1295 to 1300 at the beginning of
ide_atapi_cmd :

    const uint8_t *packet;
    uint8_t *buf;
    int max_len;

    packet = s->io_buffer;
    buf = s->io_buffer;

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

good point, and I am affraid that it might be unsafe in those architectures
with strict alignment requirements, but sadly there is no mechanism available
(at least for the ide emulation) to emulate the hardware buffers in a safe
way.

could someone with most experience on this hint into the right direction to
approach this problem and that affects several other emulated commandas as
well?

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

yes

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

will try to make a simpler version then that could be used at least as an
intermediate step and that is less intrusive than this one, while avoiding so
may magic values.

Carlo




reply via email to

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