1. Patchew
  2. QEMU
  3. View series

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

Niklas Cassel posted 1 patch 7 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231011131220.1992064-1-nks@flawful.org
Maintainers: John Snow <jsnow@redhat.com>
hw/ide/ahci.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both
Posted by Niklas Cassel 7 months, 1 week ago 
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
Posted by Kevin Wolf 6 months, 1 week ago 
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
Re: [PATCH] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both
Posted by Niklas Cassel 6 months, 3 weeks ago 
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
Posted by Michael Tokarev 7 months, 1 week ago 
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
Posted by Niklas Cassel 7 months, 1 week ago 
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
Posted by Philippe Mathieu-Daudé 7 months, 1 week ago 
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>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.