From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCotc-0006Tj-O2 for qemu-devel@nongnu.org; Wed, 08 Jul 2015 08:56:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCotX-0000yP-7g for qemu-devel@nongnu.org; Wed, 08 Jul 2015 08:56:16 -0400 Date: Wed, 8 Jul 2015 13:56:03 +0100 From: Stefan Hajnoczi Message-ID: <20150708125603.GB20502@stefanha-thinkpad.redhat.com> References: <1436219392-31915-1-git-send-email-jsnow@redhat.com> <1436219392-31915-3-git-send-email-jsnow@redhat.com> <20150707084929.GB25892@stefanha-thinkpad.redhat.com> <559C0916.3060301@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kORqDWCi7qDJ0mEj" Content-Disposition: inline In-Reply-To: <559C0916.3060301@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] ahci: fix signature generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: pbonzini@redhat.com, hare@suse.de, qemu-block@nongnu.org, qemu-devel@nongnu.org --kORqDWCi7qDJ0mEj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 07, 2015 at 01:15:02PM -0400, John Snow wrote: >=20 >=20 > On 07/07/2015 04:49 AM, Stefan Hajnoczi wrote: > > On Mon, Jul 06, 2015 at 05:49:52PM -0400, John Snow wrote: > >> The initial register device-to-host FIS no longer needs to specially > >> set certain fields, as these can be handled generically by setting tho= se > >> fields explicitly with the signatures we want at port reset time. > >> > >> (1) Signatures are decomposed into their four component registers and > >> set upon (AHCI) port reset. > >> (2) the signature cache register is no longer set manually per-each > >> device type, but instead just once during ahci_init_d2h. > >> > >> Signed-off-by: John Snow > >> --- > >> hw/ide/ahci.c | 33 ++++++++++++++++++++------------- > >> 1 file changed, 20 insertions(+), 13 deletions(-) > >=20 > > I see two code paths that call ahci_init_d2h(). Either > > ahci_reset_port() does it (if a block device is attached) or it's called > > when the guest writes to the PORT_CMD register. > >=20 > > I'm not sure the latter works. The signature doesn't seem to be set > > anywhere. > >=20 > > Any ideas? =2E.. > So on initial boot, we call ahci_init_d2h and set pr->sig, then call > ahci_write_fis_d2h. However, since the FIS RX engine (PxFRE) is off, we > don't actually generate the FIS because there's nowhere to store it. My question is about the ide_state->blk =3D=3D NULL case: ahci_reset_port() is contradictory: static void ahci_reset_port(AHCIState *s, int port) { ... ide_state =3D &s->dev[port].port.ifs[0]; if (!ide_state->blk) { return; } ... s->dev[port].port_state =3D STATE_RUN; if (!ide_state->blk) { <-- deadcode? pr->sig =3D 0; ide_state->status =3D SEEK_STAT | WRERR_STAT; } Does code after the first "if (!ide_state->blk)" in ahci_reset_port() ever execute in a drive hotplug scenario? If it doesn't execute then sig is never filled in. Your patch does not include a regression but either something is broken here or I don't understand the code. --kORqDWCi7qDJ0mEj Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVnR3jAAoJEJykq7OBq3PIe8cH/RJeLm9oI6gPeWiwYM6KZ2fK 1Wzg+uuPgeWQ9ZoucY/iZbTFacyMFECguyXX4FX0q2VNo626NZV7NNgenAQGoTLy tIh9bKy2SXUspvmxL3YWXmmcg6quqXcrxVvYXThTDzY8H7i06iHrIMX+Osv4BQDJ ECwsDFUaAWMuBWi9TkVsaIToWkrdp1UJ+Dp7okhMSSiri9XQHQDQWXSw1DXSKH53 m0npjh67OB9kAsgFxDnyryN0sw/QMncBZvQQ241P/3UG6uooOjhlOtk3bREDacsK hhRo5WXOfZsU8YZM5gTlEpOw29UTLHy6mDHbNgpYPahCqlVbqtKQ/p/teA3pcHs= =Na2M -----END PGP SIGNATURE----- --kORqDWCi7qDJ0mEj--