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

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



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*

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

> nsector = 2 sectors PRD length = 1 sector
> 
> After prepare_buf() io_buffer_size = 1 sector and sg.size = 1
> sector.
> 
> When ide_dma_cb() is called again: nsector = 2 sectors sg.size = 1
> sector
> 
> So this assertion fails?
> 
> The assertion can be dropped.
> 

Though I'm now realizing that what I really meant to write was

assert(n * 512 == s->sg.size);

Which asserts that the amount we're about to deduct from s->nsector is
equivalent to the amount we just transferred in the sglist, which is a
more precise assertion than the previous

- -        assert(s->io_buffer_size == s->sg.size);

> 
> Anyway, I think this change is good.  Previously, IDE DMA
> transferred the full PRDT and then completed the request.  Now it
> only transfers the capped PRDT.
> 

I also fixed the limit parameter type, I'll send the new series in a
few minutes, thanks!

- -- 
—js
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVlAwHAAoJEH3vgQaq/DkOTtIQAI4k4xVNh2wRFuqqC46PvYab
3j9Sjs/eBhX8dc+eGJ7JMte0Ucq1Lz+vXkMO5Ibd9i4xe2rqMtW9E27TCVJUjUdO
oeaV1hQjXivdgkDqef5z9o+QQU8xfnf6cq/2mWehE6xQcRbe8h8xQhuDuuANvQ2r
jbrwi9z0PUnV+I/Djnmyacg8uAeJCcABDxteU/KkSyqrY8Ql/hrnD4+uhYsMuXkG
2dokdHWdrTDAHt65NYCFeg3P4BI+FXgqm4DSoG9lu1uiSKOGmqnF4r87vhHsF1pt
NyKk0yHiM+r2hLen3/8oXv4xHCF/RyMFgIhnM9oVofKgUQpRNlQdnQPsr9ctBYBA
XUUe2AtJyxBEWwZ3WfpuK7KbVTzTVBOOX/uruNKT/lsuQITJGUaBvA2aBwy9CT5B
Y7Flqprr33oZ0FdgUv6F5p84cmROltMS5gcc7J/tj4WG6+snxoGKZbhX6o3sBnYz
Ou1KHYEWYySOkFzqvn3RdTMnWfM5icg+jvfHqlhHftcwifqQLJwWeHRq3l36uu1B
a4rhpKXWAtL1kYhvR9cYfOqIO4cf+bhS9urKOEnQyBQ3urmGG94T0d7duKUOj1Mp
FbpdWMiy0x5gbXiYu3Dw3SgIGhqPn4MK8o+K2TSTfBrRmW6Pgyd0TwXSG/sp0lnK
tQyGlKobV1A/2Z56vsaS
=l9B7
-----END PGP SIGNATURE-----