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_...@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 <c...@redhat.com>
>
> Thanks,
>
> C.
>
