Re: [Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[Kevin Wolf]

Am 25.08.2015 um 02:25 hat John Snow geschrieben:
> 
> 
> On 07/13/2015 06:18 AM, Kevin Wolf wrote:
> > Am 11.07.2015 um 03:05 hat John Snow geschrieben:
> >> We're supposed to abort on transfers like this, unless we fill
> >> Word 125 of our IDENTIFY data with a default transfer size, which
> >> we don't currently do.
> >>
> >> This is an ATA error, not a SCSI/ATAPI one.
> >> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
> >>
> >> If we don't do this, QEMU will loop forever trying to transfer
> >> zero bytes, which isn't particularly useful.
> >>
> >> Signed-off-by: John Snow <address@hidden>
> > 
> > Of course, one could argue that for a clean separation between the ATA
> > core and SCSI/ATAPI, this check should really be done in cmd_packet and
> > ide_abort_command() could stay static. And in fact, I do think it's
> > worthwhile to try getting a better separation in the mid term.
> > 
> > However, for a 2.4 fix, this patch might be the right thing to do.
> > 
> > The interesting part is that the SCSI command is already partially
> > handled before the error is returned. The required assumption here is
> > that the guest doesn't actually see any intermediate state between
> > issuing the command and getting the abort, so it doesn't notice that we
> > already started the command successfully. I _think_ that assumption
> > holds true.
> > 
> > Reviewed-by: Kevin Wolf <address@hidden>
> > 
> 
> Ping --
> 
> Do you still feel it's necessary we perform the abort in the IDE layer?

I can hardly say that it's necessary, but it might be desirable.

> This requires us to peek into the CDB and determine if the CDB will (or
> will not) transfer any data.
> 
> Is that sane? Is that worse than exporting ide_abort_command into the
> ATAPI layer?

No, core.c looking at the CDB is definitely not sane.

> Pretty much either way, the ATA bits need to know about the SCSI packet,
> or the ATAPI bits need to invoke core ATA functionality.
> 
> It's sort of gross, since the spec unfortunately allows for this field
> to be zero if the command does not transfer data. :(

The only reasonable way to do this that I can think of offhand would be
to return this information from ide_atapi_cmd().

Of course, the current structure of the code doesn't make this easy. But
in theory it can be done if (and only if) no asynchronous operation is
involved and the command is completed while ide_atapi_cmd() is running.
As I said in my review above, we already make that assumption; if it
weren't true, our current behaviour would be wrong.

In order to actually implement things this way, we would need to first
restructure the commands to move completion to ide_atapi_cmd() instead
of duplicating it in each command; I already mentioned this as a
possible improvement of the code in our private email thread.

Kevin