Re: [Qemu-devel] [PATCH 06/16] ahci: record ncq failures

[John Snow <jsnow@redhat.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
>> Handle NCQ failures for cases where we want to halt the VM on IO
>> errors.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c
>> | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 + hw/ide/internal.h
>> |  1 + 3 files changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 71b5085..a838317
>> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -959,13 +959,25
>> @@ static void ncq_cb(void *opaque, int ret) return; }
>> 
>> +    ncq_tfs->halt = false;
> 
> Why does halt need to be cleared here?
> 

Might make more sense to clear it just on the beginning of every
command, in execute().

There's no strong reason here other than "If there's an error and it
should be set, it'll get reset again pretty soon." It's just a default
state.

I could move it from process to execute.

>> if (ret < 0) { -        ncq_err(ncq_tfs); +        bool is_read =
>> ncq_tfs->cmd == READ_FPDMA_QUEUED; +        BlockErrorAction
>> action = blk_get_error_action(ide_state->blk, +
>> is_read, -ret); +        if (action == BLOCK_ERROR_ACTION_STOP)
>> { +            ncq_tfs->halt = true; +
>> ide_state->bus->error_status = IDE_RETRY_HBA; +        } else if
>> (action == BLOCK_ERROR_ACTION_REPORT) { +
>> ncq_err(ncq_tfs); +        } +
>> blk_error_action(ide_state->blk, action, is_read, -ret); } else
>> { ide_state->status = READY_STAT | SEEK_STAT; }
>> 
>> -    ncq_finish(ncq_tfs); +    if (!ncq_tfs->halt) { +
>> ncq_finish(ncq_tfs); +    } }
>> 
>> static int is_ncq(uint8_t ata_cmd) @@ -1042,6 +1054,7 @@ static
>> void process_ncq_command(AHCIState *s, int port, uint8_t
>> *cmd_fis, }
>> 
>> ncq_tfs->used = 1; +    ncq_tfs->halt = false; ncq_tfs->drive =
>> ad; ncq_tfs->slot = slot; ncq_tfs->cmd = ncq_fis->command; diff
>> --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 33607d7..47a3122
>> 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -262,6 +262,7
>> @@ typedef struct NCQTransferState { uint8_t cmd; int slot; int
>> used; +    bool halt; } NCQTransferState;
>> 
>> struct AHCIDevice { diff --git a/hw/ide/internal.h
>> b/hw/ide/internal.h index 7a4a86d..5abee19 100644 ---
>> a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -499,6 +499,7 @@
>> struct IDEDevice { #define IDE_RETRY_READ  0x20 #define
>> IDE_RETRY_FLUSH 0x40 #define IDE_RETRY_TRIM 0x80 +#define
>> IDE_RETRY_HBA  0x100
> 
> Feel free to squash this patch together with the next one.  It is
> hard to review in isolation since IDE_RETRY_HBA and ->halt aren't
> used yet.
> 

Will do.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjZmbAAoJEH3vgQaq/DkOPUAP/26368m3XEWpWLPePLnBOvS+
tFuUnYyrWyWdq9p9lNBPcz+v+//CAdISQ8bRZX4OS+f3W+uVB04yJc+N5igiFJCe
9f8+lnLnVO6nmNPzeKPhfigBPF8fc5tnmk4P9bSYUcEmTkdPb+HD8tbNh4l9euWl
0+uqGUhn7Gj1G8aZSZ7UNv+6Ru7SBrygnn/GlMrqrVcbTSW2uj6JmpT1xvdk1iU8
mh+j76JIBYRn2dZf4ZIHJW51x5Zijvsi04Rc8EfRDqiGZ/7HK9EQwwoZ5vUImo/d
vuSufuxISAGc5ECeGpb7fFyGbfl7Y2R+l+qipUsYZ704wTGdnkBMAst0E/NK5y8U
IHCv/5IR9lLp1qaAL0DSmkuaz6xeiBktKmy0lRT1Yq38ZK2RMCzgRPHTbsPGfF/Z
XhRkz5lgqJAdzJJNlvUDNI0jAepiREsBP4Oqi7FMaKq7ix3haSCH0k21JIJbAls5
yRNP9hUNh0Q30E/09h4/ZYRhoQTbvzZS2tLJ8zG1lB0dnIK0uML6SjO2mKbsWpN6
hPIYku04BtqDtPlRcfsOQwJ04bHHKh46PSEWaC0xCsvMVhGzWhs4FqMCxsKOvSMC
rAEYzgEWaJdZUMC+qp4RS8aX2yJP/nmHivEEb2vI6196jsXdqrdmYLBNHcL+Q6kY
bDrMjWdP5CbDUu7E4pAY
=kgU+
-----END PGP SIGNATURE-----