From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49667) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCr75-0000lg-KE for qemu-devel@nongnu.org; Wed, 08 Jul 2015 11:18:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCr71-00010z-AO for qemu-devel@nongnu.org; Wed, 08 Jul 2015 11:18:19 -0400 Message-ID: <559D3F2B.7030901@redhat.com> Date: Wed, 08 Jul 2015 11:18:03 -0400 From: John Snow MIME-Version: 1.0 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> <20150708125603.GB20502@stefanha-thinkpad.redhat.com> In-Reply-To: <20150708125603.GB20502@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] ahci: fix signature generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: pbonzini@redhat.com, hare@suse.de, qemu-block@nongnu.org, qemu-devel@nongnu.org On 07/08/2015 08:56 AM, Stefan Hajnoczi wrote: > On Tue, Jul 07, 2015 at 01:15:02PM -0400, John Snow wrote: >> >> >> 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 those >>>> 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(-) >>> >>> 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. >>> >>> I'm not sure the latter works. The signature doesn't seem to be set >>> anywhere. >>> >>> Any ideas? > ... >> 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 == NULL case: > > ahci_reset_port() is contradictory: > > static void ahci_reset_port(AHCIState *s, int port) > { > ... > ide_state = &s->dev[port].port.ifs[0]; > if (!ide_state->blk) { > return; > } > > ... > > s->dev[port].port_state = STATE_RUN; > if (!ide_state->blk) { <-- deadcode? > pr->sig = 0; > ide_state->status = 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. > I'm sorry, I misunderstood you... Haven't really played with the hotplugging much so I don't know it to work. I'll throw it on the list. Actually, since I need to start focusing on non-legacy devices, I'll start a wiki page of all the bugs and quirks I know about and that way if I forget to get back to it (or my plane disappears over the Bermuda Triangle) my understanding of existing problems will be documented. I'll stage patch #1 here (Reviewed by Kevin) for inclusion in 2.4, #2 is also a bug fix but it's more subtle and isn't known to break anything and could possibly benefit from a more comprehensive fix so I'll leave it for now. Thanks, --js