Re: [Qemu-devel] [Qemu-block] [RFC] ide: fix bmdma underflow code

[John Snow <jsnow@redhat.com>]

On 07/01/2015 01:10 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 1, 2015 at 4:49 PM, John Snow <jsnow@redhat.com> wrote:
>> On 07/01/2015 06:18 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 29, 2015 at 04:16:06PM -0400, John Snow wrote:
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c index 8c271cc..6bcf07c
>>>> 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -716,8 +716,8
>>>> @@ static void ide_dma_cb(void *opaque, int ret)
>>>>
>>>> sector_num = ide_get_sector(s); if (n > 0) { -
>>>> assert(s->io_buffer_size == s->sg.size); -
>>>> dma_buf_commit(s, s->io_buffer_size); +        assert(s->nsector
>>>> * 512 == s->sg.size);
>>>
>>> The short PRDT case:
>>>
>>
>> ...is handled "first," lower down in this callback. The first call to
>> ide_dma_cb occurs before we've prepared any bytes or set io_buffer_size.
>>
>> We'll cruise past these post-hoc checks because we didn't transfer
>> anything. We'll hit the explicit "too short" check during that first
>> call and will cease processing there.
>>
>> If the (n > 0) conditionals fire off here, it's because we've already
>> transferred data and we KNOW the PRDTs weren't too short for at least
>> one iteration of the DMA loop*
> 
> The case I described with nsector = 2 and PRDT having only 1 sector
> isn't handled before we hit the assertion failure:
> 
> qemu-system-x86_64: hw/ide/core.c:719: ide_dma_cb: Assertion
> `s->nsector * 512 == s->sg.size' failed.
> 

GOTCHA, it's because the short PRDT check doesn't check to see if it's
enough to satisfy the entire command, just at least one sector.

I forgot. You're right.

>> (*note that at present, AHCI only ever makes one DMA callback loop,
>> because we fire off the entire transfer in one shot, though the loop
>> is tolerant to multiple loops. Maybe other HBAs utilize that, but AHCI
>> doesn't. Regardless, with this patch no qtests or iotests fail, so
>> everything seems peachy.)
> 
> I have sent an ide-test.c patch with the test case that triggers the
> assertion failure.  It's a slightly different code path from the other
> short PRDT test case so it's useful to add it.
> 
>> Though I'm now realizing that what I really meant to write was
>>
>> assert(n * 512 == s->sg.size);
> 
> Looks good.
>