On Fri, Apr 28, 2023 at 9:23 AM Niklas Cassel <n...@flawful.org> wrote:
>
> From: Niklas Cassel <niklas.cas...@wdc.com>
>
> When encountering an NCQ error, you should not write the NCQ tag to the
> SError register. This is completely wrong.
Mea culpa ... !
>
> The SError register has a clear definition, where each bit represents a
> different error, see PxSERR definition in AHCI 1.3.1.
>
> If we write a random value (like the NCQ tag) in SError, e.g. Linux will
> read SError, and will trigger arbitrary error handling depending on the
> NCQ tag that happened to be executing.
>
> In case of success, ncq_cb() will call ncq_finish().
> In case of error, ncq_cb() will call ncq_err() (which will clear
> ncq_tfs->used), and then call ncq_finish(), thus using ncq_tfs->used is
> sufficient to tell if finished should get set or not.
>
> Signed-off-by: Niklas Cassel <niklas.cas...@wdc.com>
The bulk of this series looks good, I think I'd be happy to take it,
the commit messages are extremely well written so if a regression
happens to surface, we'll be able to track down what went awry.
Want to rebase and resend it?
--js
> ---
> hw/ide/ahci.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 4950d3575e..09a14408e3 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1011,7 +1011,6 @@ static void ncq_err(NCQTransferState *ncq_tfs)
>
> ide_state->error = ABRT_ERR;
> ide_state->status = READY_STAT | ERR_STAT;
> - ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
> qemu_sglist_destroy(&ncq_tfs->sglist);
> ncq_tfs->used = 0;
> }
> @@ -1021,7 +1020,7 @@ static void ncq_finish(NCQTransferState *ncq_tfs)
> /* If we didn't error out, set our finished bit. Errored commands
> * do not get a bit set for the SDB FIS ACT register, nor do they
> * clear the outstanding bit in scr_act (PxSACT). */
> - if (!(ncq_tfs->drive->port_regs.scr_err & (1 << ncq_tfs->tag))) {
> + if (ncq_tfs->used) {
> ncq_tfs->drive->finished |= (1 << ncq_tfs->tag);
> }
>
> --
> 2.40.0
>
