1. Patchew
  2. QEMU
  3. [PATCH v1 00/22] Fix incorrect hash results on AST2700
  4. View patch

[PATCH v1 09/22] hw/misc/aspeed_hace: Ensure HASH_IRQ is always set to prevent firmware hang

Jamin Lin via posted 22 patches 10 months, 3 weeks ago
There is a newer version of this series
[PATCH v1 09/22] hw/misc/aspeed_hace: Ensure HASH_IRQ is always set to prevent firmware hang
Posted by Jamin Lin via 10 months, 3 weeks ago 
Currently, if the program encounters an unsupported algorithm, it does not set
the HASH_IRQ bit in the status register and send an interrupt to indicate
command completion. As a result, the FW gets stuck waiting for a completion
signal from the HACE module.

Additionally, in do_hash_operation, if an error occurs within the conditional
statement, the HASH_IRQ bit is not set in the status register. This causes the
firmware to continuously send HASH commands, as it is unaware that the HACE
model has completed processing the command.

To fix this, the HASH_IRQ bit in the status register must always be set to
ensure that the firmware receives an interrupt from the HACE module, preventing
it from getting stuck or repeatedly sending HASH commands.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/misc/aspeed_hace.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 8f333fc97e..d4f653670e 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -311,12 +311,6 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
                             iov[i - 1].iov_len, false,
                             iov[i - 1].iov_len);
     }
-
-    /*
-     * Set status bits to indicate completion. Testing shows hardware sets
-     * these irrespective of HASH_IRQ_EN.
-     */
-    s->regs[R_STATUS] |= HASH_IRQ;
 }
 
 static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
@@ -400,10 +394,16 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
                 qemu_log_mask(LOG_GUEST_ERROR,
                         "%s: Invalid hash algorithm selection 0x%"PRIx64"\n",
                         __func__, data & ahc->hash_mask);
-                break;
+        } else {
+            do_hash_operation(s, algo, data & HASH_SG_EN,
+                    ((data & HASH_HMAC_MASK) == HASH_DIGEST_ACCUM));
         }
-        do_hash_operation(s, algo, data & HASH_SG_EN,
-                ((data & HASH_HMAC_MASK) == HASH_DIGEST_ACCUM));
+
+        /*
+         * Set status bits to indicate completion. Testing shows hardware sets
+         * these irrespective of HASH_IRQ_EN.
+         */
+        s->regs[R_STATUS] |= HASH_IRQ;
 
         if (data & HASH_IRQ_EN) {
             qemu_irq_raise(s->irq);
-- 
2.43.0
Re: [PATCH v1 09/22] hw/misc/aspeed_hace: Ensure HASH_IRQ is always set to prevent firmware hang
Posted by Cédric Le Goater 10 months, 1 week ago 
On 3/21/25 10:26, Jamin Lin wrote:
> Currently, if the program encounters an unsupported algorithm, it does not set
> the HASH_IRQ bit in the status register and send an interrupt to indicate
> command completion. As a result, the FW gets stuck waiting for a completion
> signal from the HACE module.
> 
> Additionally, in do_hash_operation, if an error occurs within the conditional
> statement, the HASH_IRQ bit is not set in the status register. This causes the
> firmware to continuously send HASH commands, as it is unaware that the HACE
> model has completed processing the command.
> 
> To fix this, the HASH_IRQ bit in the status register must always be set to
> ensure that the firmware receives an interrupt from the HACE module, preventing
> it from getting stuck or repeatedly sending HASH commands.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

Should we add a 'Fixes' trailer ?

> ---
>   hw/misc/aspeed_hace.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 8f333fc97e..d4f653670e 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -311,12 +311,6 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                               iov[i - 1].iov_len, false,
>                               iov[i - 1].iov_len);
>       }
> -
> -    /*
> -     * Set status bits to indicate completion. Testing shows hardware sets
> -     * these irrespective of HASH_IRQ_EN.
> -     */
> -    s->regs[R_STATUS] |= HASH_IRQ;
>   }
>   
>   static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned int size)
> @@ -400,10 +394,16 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                   qemu_log_mask(LOG_GUEST_ERROR,
>                           "%s: Invalid hash algorithm selection 0x%"PRIx64"\n",
>                           __func__, data & ahc->hash_mask);
> -                break;
> +        } else {
> +            do_hash_operation(s, algo, data & HASH_SG_EN,
> +                    ((data & HASH_HMAC_MASK) == HASH_DIGEST_ACCUM));
>           }
> -        do_hash_operation(s, algo, data & HASH_SG_EN,
> -                ((data & HASH_HMAC_MASK) == HASH_DIGEST_ACCUM));
> +
> +        /*
> +         * Set status bits to indicate completion. Testing shows hardware sets
> +         * these irrespective of HASH_IRQ_EN.

is that still true on the AST2700 SoC ?

> +         */
> +        s->regs[R_STATUS] |= HASH_IRQ;
>   >           if (data & HASH_IRQ_EN) {
>               qemu_irq_raise(s->irq);



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.
RE: [PATCH v1 09/22] hw/misc/aspeed_hace: Ensure HASH_IRQ is always set to prevent firmware hang
Posted by Jamin Lin 9 months, 1 week ago 
Hi Cédric

> Subject: Re: [PATCH v1 09/22] hw/misc/aspeed_hace: Ensure HASH_IRQ is
> always set to prevent firmware hang
> 
> On 3/21/25 10:26, Jamin Lin wrote:
> > Currently, if the program encounters an unsupported algorithm, it does
> > not set the HASH_IRQ bit in the status register and send an interrupt
> > to indicate command completion. As a result, the FW gets stuck waiting
> > for a completion signal from the HACE module.
> >
> > Additionally, in do_hash_operation, if an error occurs within the
> > conditional statement, the HASH_IRQ bit is not set in the status
> > register. This causes the firmware to continuously send HASH commands,
> > as it is unaware that the HACE model has completed processing the
> command.
> >
> > To fix this, the HASH_IRQ bit in the status register must always be
> > set to ensure that the firmware receives an interrupt from the HACE
> > module, preventing it from getting stuck or repeatedly sending HASH
> commands.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> 
> Should we add a 'Fixes' trailer ?
Thanks for review and suggestion.
Will add
Jamin

> 
> > ---
> >   hw/misc/aspeed_hace.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > 8f333fc97e..d4f653670e 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -311,12 +311,6 @@ static void do_hash_operation(AspeedHACEState *s,
> int algo, bool sg_mode,
> >                               iov[i - 1].iov_len, false,
> >                               iov[i - 1].iov_len);
> >       }
> > -
> > -    /*
> > -     * Set status bits to indicate completion. Testing shows hardware sets
> > -     * these irrespective of HASH_IRQ_EN.
> > -     */
> > -    s->regs[R_STATUS] |= HASH_IRQ;
> >   }
> >
> >   static uint64_t aspeed_hace_read(void *opaque, hwaddr addr, unsigned
> > int size) @@ -400,10 +394,16 @@ static void aspeed_hace_write(void
> *opaque, hwaddr addr, uint64_t data,
> >                   qemu_log_mask(LOG_GUEST_ERROR,
> >                           "%s: Invalid hash algorithm selection
> 0x%"PRIx64"\n",
> >                           __func__, data & ahc->hash_mask);
> > -                break;
> > +        } else {
> > +            do_hash_operation(s, algo, data & HASH_SG_EN,
> > +                    ((data & HASH_HMAC_MASK) ==
> HASH_DIGEST_ACCUM));
> >           }
> > -        do_hash_operation(s, algo, data & HASH_SG_EN,
> > -                ((data & HASH_HMAC_MASK) ==
> HASH_DIGEST_ACCUM));
> > +
> > +        /*
> > +         * Set status bits to indicate completion. Testing shows hardware
> sets
> > +         * these irrespective of HASH_IRQ_EN.
> 
> is that still true on the AST2700 SoC ?
> 
> > +         */
> > +        s->regs[R_STATUS] |= HASH_IRQ;
> >   >           if (data & HASH_IRQ_EN) {
> >               qemu_irq_raise(s->irq);
> 
> 
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> Thanks,
> 
> C.
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.