From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z72Or-0005Iu-PS for qemu-devel@nongnu.org; Mon, 22 Jun 2015 10:08:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z72Om-0002cJ-4p for qemu-devel@nongnu.org; Mon, 22 Jun 2015 10:08:37 -0400 Message-ID: <558816D6.3030400@redhat.com> Date: Mon, 22 Jun 2015 10:08:22 -0400 From: John Snow MIME-Version: 1.0 References: <1434765047-29333-1-git-send-email-jsnow@redhat.com> <1434765047-29333-5-git-send-email-jsnow@redhat.com> <20150622140619.GB7136@stefanha-thinkpad.redhat.com> In-Reply-To: <20150622140619.GB7136@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On 06/22/2015 10:06 AM, Stefan Hajnoczi wrote: > On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote: >> @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState >> *s, int port, uint8_t *cmd_fis, ((uint64_t)ncq_fis->lba2 << 16) >> | ((uint64_t)ncq_fis->lba1 << 8) | (uint64_t)ncq_fis->lba0; + >> ncq_tfs->tag = tag; >> >> - /* Note: We calculate the sector count, but don't currently >> rely on it. - * The total size of the DMA buffer tells us the >> transfer size instead. */ ncq_tfs->sector_count = >> ((uint16_t)ncq_fis->sector_count_high << 8) | >> ncq_fis->sector_count_low; + ahci_populate_sglist(ad, >> &ncq_tfs->sglist, 0); + size = ncq_tfs->sector_count * 512; > > ncq_tfs->sector_count is used with - 2 and - 1 below. What is the > semantics of this field and why is it okay to use it without > subtracting here? > Just a bonkers patch order. Patch #7 fixes the debug prints, which are wrong. The field is not "off by one" in the spec, sector_count == 1 means 1 sector. (sector_count == 0 means 64K sectors, though, like it does in regular ATA.)