Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[John Snow]

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 <address@hidden> --- 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