Re: [PATCH 2/9] hw/ide/core: set ERR_STAT in unsupported command completion

From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: John Snow <jsnow@redhat.com>
Cc: Niklas Cassel <nks@flawful.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Damien Le Moal <dlemoal@kernel.org>
Subject: Re: [PATCH 2/9] hw/ide/core: set ERR_STAT in unsupported command completion
Date: Thu, 1 Jun 2023 13:28:38 +0000	[thread overview]
Message-ID: <ZHidAgMaVTZq23lB@x1-carbon> (raw)
In-Reply-To: <CAFn=p-adzHqAMvdYjiMvXpv1-Z3SE78LUErOD5v0yW+6ZZbumw@mail.gmail.com>

On Wed, May 17, 2023 at 05:12:57PM -0400, John Snow wrote:
> On Fri, Apr 28, 2023 at 9:22 AM Niklas Cassel <nks@flawful.org> wrote:
> >
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > Currently, the first time sending an unsupported command
> > (e.g. READ LOG DMA EXT) will not have ERR_STAT set in the completion.
> > Sending the unsupported command again, will correctly have ERR_STAT set.
> >
> > When ide_cmd_permitted() returns false, it calls ide_abort_command().
> > ide_abort_command() first calls ide_transfer_stop(), which will call
> > ide_transfer_halt() and ide_cmd_done(), after that ide_abort_command()
> > sets ERR_STAT in status.
> >
> > ide_cmd_done() for AHCI will call ahci_write_fis_d2h() which writes the
> > current status in the FIS, and raises an IRQ. (The status here will not
> > have ERR_STAT set!).
> >
> > Thus, we cannot call ide_transfer_stop() before setting ERR_STAT, as
> > ide_transfer_stop() will result in the FIS being written and an IRQ
> > being raised.
> >
> > The reason why it works the second time, is that ERR_STAT will still
> > be set from the previous command, so when writing the FIS, the
> > completion will correctly have ERR_STAT set.
> >
> > Set ERR_STAT before writing the FIS (calling cmd_done), so that we will
> > raise an error IRQ correctly when receiving an unsupported command.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  hw/ide/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 45d14a25e9..c144d1155d 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -531,9 +531,9 @@ BlockAIOCB *ide_issue_trim(
> >
> >  void ide_abort_command(IDEState *s)
> >  {
> > -    ide_transfer_stop(s);
> >      s->status = READY_STAT | ERR_STAT;
> >      s->error = ABRT_ERR;
> > +    ide_transfer_stop(s);
> >  }
> >
> >  static void ide_set_retry(IDEState *s)
> > --
> > 2.40.0
> >
> 
> Seems OK at a glance. Does this change the behavior of
> ide_transfer_stop at all? I guess we've been using this order of
> operations since 2008 at least. I didn't know C then.

Hello John,

Not as far as I can see.

Kind regards,
Niklas