From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAMuN-0008Hp-LJ for qemu-devel@nongnu.org; Wed, 01 Jul 2015 14:38:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAMuM-0006IV-N8 for qemu-devel@nongnu.org; Wed, 01 Jul 2015 14:38:55 -0400 Message-ID: <559433B7.5050701@redhat.com> Date: Wed, 01 Jul 2015 14:38:47 -0400 From: John Snow MIME-Version: 1.0 References: <1435608966-12135-1-git-send-email-jsnow@redhat.com> <20150701101837.GE13991@stefanha-thinkpad.redhat.com> <55940C07.6020307@redhat.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [RFC] ide: fix bmdma underflow code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu block , qemu-devel , Stefan Hajnoczi On 07/01/2015 01:10 PM, Stefan Hajnoczi wrote: > On Wed, Jul 1, 2015 at 4:49 PM, John Snow 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. >