[PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
unconditionally, regardless if the I bit is set in the FIS or not.

Thus, we should never raise a normal IRQ after having sent an error
IRQ.

NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index fcc5476e9e..7676e2d871 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -897,11 +897,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i)
     pr->tfdata = (ad->port.ifs[0].error << 8) |
         ad->port.ifs[0].status;
 
+    /* TFES IRQ is always raised if ERR_STAT is set, regardless of I bit. */
     if (d2h_fis[2] & ERR_STAT) {
         ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
-    }
-
-    if (d2h_fis_i) {
+    } else if (d2h_fis_i) {
         ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
     }
 
-- 
2.41.0

Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Philippe Mathieu-Daudé]

On 11/10/23 15:12, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
> we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
> unconditionally, regardless if the I bit is set in the FIS or not.
> 
> Thus, we should never raise a normal IRQ after having sent an error
> IRQ.
> 
> NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
> commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
> QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/ahci.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Michael Tokarev]

11.10.2023 16:12, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
> we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
> unconditionally, regardless if the I bit is set in the FIS or not.
> 
> Thus, we should never raise a normal IRQ after having sent an error
> IRQ.
> 
> NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
> commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
> QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").

Am I right that without commit 1281e340a "ahci: handle TFES irq correctly"
in seabios (which is included in this 784155cd update above), seabios
will hang at boot for 32s when qemu is booted with ahci drives?

I mean, is it the only implication of not updating seabios after
this patch?

Thanks!

/mjt

Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Niklas Cassel]

On Wed, Oct 11, 2023 at 05:44:28PM +0300, Michael Tokarev wrote:
> 11.10.2023 16:12, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
> > we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
> > unconditionally, regardless if the I bit is set in the FIS or not.
> > 
> > Thus, we should never raise a normal IRQ after having sent an error
> > IRQ.
> > 
> > NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
> > commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
> > QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").
> 
> Am I right that without commit 1281e340a "ahci: handle TFES irq correctly"
> in seabios (which is included in this 784155cd update above), seabios
> will hang at boot for 32s when qemu is booted with ahci drives?
> 
> I mean, is it the only implication of not updating seabios after
> this patch?

That is correct.

If you don't have the seabios patch, the seabios ATAPI command will timeout
after 30 seconds, after which QEMU will boot and behave like normal.


Kind regards,
Niklas

Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Niklas Cassel]

On Wed, Oct 11, 2023 at 03:12:20PM +0200, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
> we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
> unconditionally, regardless if the I bit is set in the FIS or not.
> 
> Thus, we should never raise a normal IRQ after having sent an error
> IRQ.
> 
> NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
> commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
> QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  hw/ide/ahci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index fcc5476e9e..7676e2d871 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -897,11 +897,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i)
>      pr->tfdata = (ad->port.ifs[0].error << 8) |
>          ad->port.ifs[0].status;
>  
> +    /* TFES IRQ is always raised if ERR_STAT is set, regardless of I bit. */
>      if (d2h_fis[2] & ERR_STAT) {
>          ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
> -    }
> -
> -    if (d2h_fis_i) {
> +    } else if (d2h_fis_i) {
>          ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
>      }
>  
> -- 
> 2.41.0
> 

Gentle ping :)


Kind regards,
Niklas

Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both

[Kevin Wolf]

Am 11.10.2023 um 15:12 hat Niklas Cassel geschrieben:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
> we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
> unconditionally, regardless if the I bit is set in the FIS or not.
> 
> Thus, we should never raise a normal IRQ after having sent an error
> IRQ.
> 
> NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
> commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
> QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot").
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Thanks, applied to the block branch.

Kevin