Re: [PATCH 2/9] hw/ide/core: set ERR_STAT in unsupported command completion
On Wed, May 17, 2023 at 05:12:57PM -0400, John Snow wrote:
> On Fri, Apr 28, 2023 at 9:22 AM Niklas Cassel wrote:
> >
> > From: Niklas Cassel
> >
> > 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
> > ---
> > 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
Re: [PATCH 2/9] hw/ide/core: set ERR_STAT in unsupported command completion
On Fri, Apr 28, 2023 at 9:22 AM Niklas Cassel wrote:
>
> From: Niklas Cassel
>
> 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
> ---
> 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.
ACK
[PATCH 2/9] hw/ide/core: set ERR_STAT in unsupported command completion
From: Niklas Cassel
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
---
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
