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

[Stefan Hajnoczi]

On Wed, Jul 1, 2015 at 4:49 PM, John Snow <address@hidden> 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.

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