From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z72x7-0007bf-V1 for qemu-devel@nongnu.org; Mon, 22 Jun 2015 10:44:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z72x6-0000M6-Pf for qemu-devel@nongnu.org; Mon, 22 Jun 2015 10:44:01 -0400 Message-ID: <55881F28.5050801@redhat.com> Date: Mon, 22 Jun 2015 10:43:52 -0400 From: John Snow MIME-Version: 1.0 References: <1434765047-29333-1-git-send-email-jsnow@redhat.com> <1434765047-29333-9-git-send-email-jsnow@redhat.com> <20150622143840.GE7136@stefanha-thinkpad.redhat.com> In-Reply-To: <20150622143840.GE7136@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd 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:38 AM, Stefan Hajnoczi wrote: > On Fri, Jun 19, 2015 at 09:50:39PM -0400, John Snow wrote: >> The legacy ide command execution layer will clear any errors >> outstanding before execution, but the NCQ layer doesn't. Even on >> success, this register will remain clogged. >> >> Clear it out before each NCQ command so the guest can tell if >> the error code produced after completion is meaningful or not. >> >> Signed-off-by: John Snow --- hw/ide/ahci.c | 2 >> ++ 1 file changed, 2 insertions(+) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 6bded67..e63ba9b >> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1048,6 +1048,8 >> @@ static void process_ncq_command(AHCIState *s, int port, >> uint8_t *cmd_fis, ncq_tfs->lba, ncq_tfs->lba + >> ncq_tfs->sector_count - 1, ide_state->nb_sectors - 1); >> >> + ide_state->error = 0; > > I'm not sure it makes sense use ide_state at all in NCQ. > > ide_state is per-port and NCQ can issue multiple asynchronous > commands per port. If process_ncq_command() modifies ide_state, it > may do that while other commands are still pending or about to be > processed. > > This will clobber ide_state->error. > Good point. The real problem that we need to fix then is in the core IDE layer, which tends to set a "default error" and waits for ide_exec_cmd to clear it before execution. If we don't clear that bit somehow, we will get failed commands. I'll have to do a little research to see if it's safe to remove our startup error, since it might be part of an ATA signature handshake. Hmm. Maybe it's not actually a problem because any real OS should probably have issued ATA IDENTIFY (which will clear ERR) by the time they get to NCQ, so maybe we can scrap this, and I'll adjust the test accordingly to issue at least IDENTIFY before attempting NCQ. Thanks. --js