Re: [Qemu-devel] [PATCH 0/6] AHCI Device Fixes

[John Snow]

On 10/28/2014 08:27 PM, Paolo Bonzini wrote:
> > > Yeah, I was wondering if any commands could have <512 bytes response...
> > >    I sort of convinced myself that the answer was no for ATA commands, but
> > > stupidly forgot about packet (SCSI) commands.  Their results are
> > > obviously shorter than 512 bytes.
> >
> > Are you referencing the sglist underflow patch? (#5 instead of #3)
>
> Hmm, yeah.
>
> > There were cases in the code already where we /assumed/ that having any bytes
> > implied we had at least a sector's worth.
>
> Even for ATAPI?
>
>   If there are valid cases for
> > the sglist to have less than a sector's worth (SCSI) then I'll need to
> > touch that again as well and update all the assumptions in the IDE code
> > to look for numbytes instead of numsectors.
>
> cmd_inquiry can return 36 bytes.  That can be both PIO and DMA.  SeaBIOS
> only uses the DMA variant for AHCI.
>
> Paolo

OK. So the sglist underflow patch recognizes that functions in the DMA 
loop check to see if the "prepare buf" calls succeed or not by checking 
their return code, which was previously generated by something like:
return (size != 0);

Which I updated to be:
return (size / 512) != 0;

This was adjusted in two functions:
ahci_dma_prepare_buf
bmdma_prepare_buf

which are both usually accessed via the .prepare_buf member for IDE DMA OPS.

bmdma_prepare_buf is never accessed outside of this callback. AHCI's 
prepare buf is called only once, in ahci_start_transfer, which is the 
PIO pathway (so it includes the PACKET transfer mechanisms.) In 
ahci_start_transfer, we don't actually even check the return code, so we 
don't fail commands like cmd_inquiry, so this will succeed.

The only call to prepare_buf is otherwise in ide_dma_cb, which does not 
include PIO or PACKET pathways, so this command should always return 
full sectors.

So the code as-written does not actually prevent non-ATA commands from 
working as intended, however, it is a little messy because somebody 
might misunderstand what the return code from .prepare_buf really even 
means.

Here's what I propose:

(1) Update the prepare_buf callback (including the AHCI and BMDMA 
implementations) to return, simply, the number of bytes prepared. For 
AHCI, the largest this can ever be is something like

(2) Update uses of the callback or implementations to use this number 
directly to determine if the call succeeded, failed, or "succeeded 
enough" for our purposes.

(3) We can reserve the return code of -1 to imply catastrophic failure.



In the meantime:
Patches 1, 2, and 6 are fine and should be merged. I have also fixed 
patch 3, but I can submit that by itself a little later.

Patch 4 and 5 work best together but have the confusion documented 
above, and I can do better.