From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37343) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1zLl-0005xT-Fe for qemu-devel@nongnu.org; Tue, 01 Jul 2014 10:48:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1zLg-00016X-N1 for qemu-devel@nongnu.org; Tue, 01 Jul 2014 10:48:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24015) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1zLg-00015T-DO for qemu-devel@nongnu.org; Tue, 01 Jul 2014 10:47:56 -0400 Message-ID: <53B2CA19.2060101@redhat.com> Date: Tue, 01 Jul 2014 10:47:53 -0400 From: John Snow MIME-Version: 1.0 References: <1404213207-89115-1-git-send-email-reza.jelveh@tuhh.de> <20140701113612.GF4587@noname.str.redhat.com> <53B2A04B.70406@suse.de> In-Reply-To: <53B2A04B.70406@suse.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH_v2] ahci.c: mask unused flags when reading size PRDT DBC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Reza Jelveh On 07/01/2014 07:49 AM, Alexander Graf wrote: > > On 01.07.14 13:36, Kevin Wolf wrote: >> Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben: >>> From: Reza Jelveh >>> >>> The data byte count(DBC) read from the description information is >>> defined for >>> bits 21:00. Bits 30:22 are reserved and bit 31 is the Interrupt on >>> Completion >>> (I) flag. >>> >>> Completion interrupts are triggered after every transaction instead >>> of on >>> I-flag in QEMU. tbl_entry_size is a signed integer and improperly >>> reading the >>> DBC leads to a negative offset that causes sglist allocation to fail. >>> >>> Signed-off-by: Reza Jelveh >>> --- >>> This requires a custom ovmf image with sata controller for testing: >>> >>> http://reza.jelveh.me/assets/OVMF.fd.bz2 >>> >>> Signed-off-by: Reza Jelveh >> Reviewed-by: Kevin Wolf >> >> The spec also seems to require an even byte count, which we don't seem >> to check. Do we want to add this? (In a separate patch, of course.) > > We could just remove the lowest bit in the mask, no? ;) > > > Alex > Reviewed-by: John Snow Taking a look at the spec, AHCI 1.3 sec 4.2.3.3 p. 40; a value of 0x00 implies one byte, and 0x01 implies two bytes. Masking off the one bit would probably lead to an off-by-one somewhere. The spec does state that it requires the 0th bit to be set, so in a separate patch we should check to make sure, but the mask as-is is appropriate. --John