Re: [PATCH 9/9] hw/ide/ahci: fix broken SError handling

From: John Snow <jsnow@redhat.com>
To: Niklas Cassel <nks@flawful.org>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org,
	 Damien Le Moal <dlemoal@kernel.org>,
	Niklas Cassel <niklas.cassel@wdc.com>
Subject: Re: [PATCH 9/9] hw/ide/ahci: fix broken SError handling
Date: Wed, 17 May 2023 17:26:17 -0400	[thread overview]
Message-ID: <CAFn=p-YXdRWF4gd1sP=ZJHv-fvqwckVQsuKD5aVgtEM90WYzkA@mail.gmail.com> (raw)
In-Reply-To: <20230428132124.670840-10-nks@flawful.org>

On Fri, Apr 28, 2023 at 9:23 AM Niklas Cassel <nks@flawful.org> wrote:
>
> From: Niklas Cassel <niklas.cassel@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.cassel@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
>