From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53529) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9wsC-0004fb-GS for qemu-devel@nongnu.org; Tue, 30 Jun 2015 10:51:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9wsB-00055k-0n for qemu-devel@nongnu.org; Tue, 30 Jun 2015 10:50:56 -0400 Date: Tue, 30 Jun 2015 15:50:45 +0100 From: Stefan Hajnoczi Message-ID: <20150630145045.GD31899@stefanha-thinkpad.redhat.com> References: <1435018875-22527-1-git-send-email-jsnow@redhat.com> <1435018875-22527-15-git-send-email-jsnow@redhat.com> <20150626155949.GF31186@stefanha-thinkpad.redhat.com> <558D8C60.3040105@redhat.com> <20150629145115.GJ32151@stefanha-thinkpad.redhat.com> <55915F26.6070202@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rz+pwK2yUstbofK6" Content-Disposition: inline In-Reply-To: <55915F26.6070202@redhat.com> Subject: Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: kwolf@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org, pbonzini@redhat.com --rz+pwK2yUstbofK6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 29, 2015 at 11:07:18AM -0400, John Snow wrote: > On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote: > > On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote: > >> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote: > >>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: > >>>> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice > >>>> *ad, uint16_t len) pio_fis[9] =3D s->hob_lcyl; pio_fis[10] =3D > >>>> s->hob_hcyl; pio_fis[11] =3D 0; - pio_fis[12] =3D cmd_fis[12]; - > >>>> pio_fis[13] =3D cmd_fis[13]; + pio_fis[12] =3D s->nsector & 0xFF;= =20 > >>>> + pio_fis[13] =3D (s->nsector >> 8) & 0xFF; > >>> > >>> hw/ide/core.c decreases s->nsector until it reaches 0 and the > >>> request ends. > >>> > >>> Will the values reported back to the guest be correct if we use=20 > >>> s->nsector? > >>> > >> > >> See the commit message for justification of this one. Ultimately, it > >> doesn't matter what gets put in here (for data transfer commands) -- > >> but getting RID of the cmd_fis mapping is a strong positive. > >=20 > > Getting rid of cmd_fis mapping is good. > >=20 > > Putting s->nsector into the undefined fields makes the code confusing. > >=20 > > It is clearer to zero the bytes with a comment saying the value does not > > matter according to the spec. > >=20 >=20 > Well, it's not that it doesn't matter /ever/, it's more that for > standard IO routines it doesn't matter. (See the normative output spec > in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it > carries a diagnostic signature.) >=20 > What's really the case is that the FIS always dutifully copies out what > the SATA registers are (or should be.) >=20 > There are still a handful of commands that, if we choose to support > them, copying the nsector register would be the "correct thing" to do, > so I decided to copy that field here to serve as documentation and > support future command additions. >=20 > I would argue that if this field ever does the /wrong thing/, it would > be a fix in the S/ATA layer, and not a change to the FIS generator here. >=20 > I am inclined to leave it as-is, since for the current cases, nsector is > going to empty to zero anyway. I believe the behavior presented here is > correct. I'm trying to understand the guest-visible change in behavior. Guests might take different code paths from before. For example, I think that after this patch the CHECK POWER MODE command works correctly for the first time with AHCI. It sets ->nsector to 0xff. Anyway, I'm happy with assigning s->nsector. --rz+pwK2yUstbofK6 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVkqzFAAoJEJykq7OBq3PIzDkH+wSrSesQV5b8t43YuLWzNScd jN4Ji2yoSr9GKSyFHxpS49fGA/BgyRnK95vegRPZMLr9i4mLsJ1dvvb2+JdnqJHw RvjsBBPvXfjwaUn3varNeA+rRmTRvhOmPs1+LsW6HKX4g0ph7yDTPvGv2NPIBcO+ vNN3h9BF+22OLY1bmondnEYCRDBbLbD1xfva4cZHh0W9BTcVi2hvn+wG2pPpPmBt 1DMEPDOln1NGE6o8jmL6SMuUnbpd7c4nfDnSh+w7vYc+2YPlueSg6lGnWvJoauvb 8EhKv2KESQVUMLZXI26VmnfnWtcTnVruYW1bKXbhQsFR0qrHhZAgLxplNgUtAI0= =Quy2 -----END PGP SIGNATURE----- --rz+pwK2yUstbofK6--