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

[John Snow <jsnow@redhat.com>]

On 10/28/2014 08:03 PM, Paolo Bonzini wrote:
>
>
> On 10/29/2014 12:54 AM, John Snow wrote:
>>
>>
>> On 10/28/2014 09:51 AM, Stefan Hajnoczi wrote:
>>> On Wed, Oct 01, 2014 at 06:55:45PM -0400, John Snow wrote:
>>>> Based off of feedback from the RFC of the same name,
>>>> this series batches together a group of fixes that
>>>> improve the AHCI device to fix a number of bugs.
>>>>
>>>> A number of fixes included in the RFC that provide more
>>>> radical changes are omitted for now in favor of a smaller,
>>>> more easily reviewable set for QEMU 2.2.
>>>>
>>>> In summary:
>>>>
>>>> Patch #1 and #6 correct the format of FIS packet responses
>>>> that are available to the guest operating system upon interrupt.
>>>>
>>>> Patch #2 corrects an oversight where we do not inform the
>>>> guest how many bytes we've transferred. This is relied upon
>>>> for non-NCQ modes and in some early bootup and shutdown code.
>>>>
>>>> Patch #5 corrects cases with malformed scatter-gather lists that
>>>> may cause leaks, or cause QEMU to hang in an allocation loop.
>>>>
>>>> Patch #4 attempts to continue minimizing the divergence of the
>>>> multiple pathways through the AHCI device by re-using existing
>>>> callbacks.
>>>>
>>>> Taken together, these patches should allow non-ncq operation
>>>> for Windows hosts, as well as enable hibernation for Windows 7.
>>>>
>>>> Hibernation for Windows 8 and AHCI remains non-functional.
>>>>
>>>> John Snow (6):
>>>>     ahci: Correct PIO/D2H FIS responses
>>>>     ahci: Update byte count after DMA completion
>>>>     ide: repair PIO transfers for cases where nsector > 1
>>>>     ahci: unify sglist preparation
>>>>     ide: Correct handling of malformed/short PRDTs
>>>>     ahci: Fix SDB FIS Construction
>>>>
>>>>    dma-helpers.c     |   3 ++
>>>>    hw/ide/ahci.c     | 113
>>>> ++++++++++++++++++++++++++++++++----------------------
>>>>    hw/ide/ahci.h     |   8 ++++
>>>>    hw/ide/core.c     |  17 ++++++--
>>>>    hw/ide/internal.h |   2 +
>>>>    hw/ide/pci.c      |   5 ++-
>>>>    6 files changed, 97 insertions(+), 51 deletions(-)
>>>
>>> Dropped the broken assert that Paolo spotted.
>>>
>>> Thanks, applied to my block tree:
>>> https://github.com/stefanha/qemu/commits/block
>>>
>>> Stefan
>>
>> Drop the PIO patch for now (#3), I ran into a regression with it with
>> CDROM drives. I believe the others are still fine.
>
> 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.
>
> Paolo
>

Are you referencing the sglist underflow patch? (#5 instead of #3) There 
were cases in the code already where we /assumed/ that having any bytes 
implied we had at least a sector's worth. 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.