[PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test

Jamin Lin via posted 28 patches 12 months ago
There is a newer version of this series
[PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Jamin Lin via 12 months ago
Currently, it does not support the CRYPT command. Instead, it only sends an
interrupt to notify the firmware that the crypt command has completed.
It is a temporary workaround to resolve the boot issue in the Crypto Manager
Self Test.

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

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 86422cb3be..4d0999e7e9 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -59,6 +59,7 @@
 /* Other cmd bits */
 #define  HASH_IRQ_EN                    BIT(9)
 #define  HASH_SG_EN                     BIT(18)
+#define  CRYPT_IRQ_EN                   BIT(12)
 /* Scatter-gather data list */
 #define SG_LIST_LEN_SIZE                4
 #define SG_LIST_LEN_MASK                0x0FFFFFFF
@@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
                 qemu_irq_lower(s->irq);
             }
         }
+        if (data & CRYPT_IRQ) {
+            data &= ~CRYPT_IRQ;
+
+            if (s->regs[addr] & CRYPT_IRQ) {
+                qemu_irq_lower(s->irq);
+            }
+        }
         break;
     case R_HASH_SRC:
         data &= ahc->src_mask;
@@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
     case R_CRYPT_CMD:
         qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not implemented\n",
                        __func__);
+        s->regs[R_STATUS] |= CRYPT_IRQ;
+        if (data & CRYPT_IRQ_EN) {
+            qemu_irq_raise(s->irq);
+        }
         break;
     default:
         break;
-- 
2.34.1
Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Cédric Le Goater 11 months, 3 weeks ago
On 2/13/25 04:35, Jamin Lin wrote:
> Currently, it does not support the CRYPT command. Instead, it only sends an
> interrupt to notify the firmware that the crypt command has completed.
> It is a temporary workaround to resolve the boot issue in the Crypto Manager
> Self Test.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

Please add an AspeedHACEClass class attribute (bool) to handle
this workaround and a comment in the code mentioning the issue.


Thanks,

C.



> ---
>   hw/misc/aspeed_hace.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 86422cb3be..4d0999e7e9 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -59,6 +59,7 @@
>   /* Other cmd bits */
>   #define  HASH_IRQ_EN                    BIT(9)
>   #define  HASH_SG_EN                     BIT(18)
> +#define  CRYPT_IRQ_EN                   BIT(12)
>   /* Scatter-gather data list */
>   #define SG_LIST_LEN_SIZE                4
>   #define SG_LIST_LEN_MASK                0x0FFFFFFF
> @@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>                   qemu_irq_lower(s->irq);
>               }
>           }
> +        if (data & CRYPT_IRQ) {
> +            data &= ~CRYPT_IRQ;
> +
> +            if (s->regs[addr] & CRYPT_IRQ) {
> +                qemu_irq_lower(s->irq);
> +            }
> +        }
>           break;
>       case R_HASH_SRC:
>           data &= ahc->src_mask;
> @@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque, hwaddr addr, uint64_t data,
>       case R_CRYPT_CMD:
>           qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not implemented\n",
>                          __func__);
> +        s->regs[R_STATUS] |= CRYPT_IRQ;
> +        if (data & CRYPT_IRQ_EN) {
> +            qemu_irq_raise(s->irq);
> +        }
>           break;
>       default:
>           break;
RE: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Jamin Lin 11 months, 3 weeks ago
Hi Cedric,

> Subject: Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the
> Crypto Manager Self Test
> 
> On 2/13/25 04:35, Jamin Lin wrote:
> > Currently, it does not support the CRYPT command. Instead, it only
> > sends an interrupt to notify the firmware that the crypt command has
> completed.
> > It is a temporary workaround to resolve the boot issue in the Crypto
> > Manager Self Test.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> 
> Please add an AspeedHACEClass class attribute (bool) to handle this
> workaround and a comment in the code mentioning the issue.
> 
Thanks for review and suggestion.
I will add the use_crypt_workaround attribute to the AspeedHACEClass and introduce the use-crypt-workaround property.
Do you have any concerns, or do you have a preferred naming convention?
Thanks-Jamin

> 
> Thanks,
> 
> C.
> 
> 
> 
> > ---
> >   hw/misc/aspeed_hace.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > 86422cb3be..4d0999e7e9 100644
> > --- a/hw/misc/aspeed_hace.c
> > +++ b/hw/misc/aspeed_hace.c
> > @@ -59,6 +59,7 @@
> >   /* Other cmd bits */
> >   #define  HASH_IRQ_EN                    BIT(9)
> >   #define  HASH_SG_EN                     BIT(18)
> > +#define  CRYPT_IRQ_EN                   BIT(12)
> >   /* Scatter-gather data list */
> >   #define SG_LIST_LEN_SIZE                4
> >   #define SG_LIST_LEN_MASK                0x0FFFFFFF
> > @@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque,
> hwaddr addr, uint64_t data,
> >                   qemu_irq_lower(s->irq);
> >               }
> >           }
> > +        if (data & CRYPT_IRQ) {
> > +            data &= ~CRYPT_IRQ;
> > +
> > +            if (s->regs[addr] & CRYPT_IRQ) {
> > +                qemu_irq_lower(s->irq);
> > +            }
> > +        }
> >           break;
> >       case R_HASH_SRC:
> >           data &= ahc->src_mask;
> > @@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque,
> hwaddr addr, uint64_t data,
> >       case R_CRYPT_CMD:
> >           qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not
> implemented\n",
> >                          __func__);
> > +        s->regs[R_STATUS] |= CRYPT_IRQ;
> > +        if (data & CRYPT_IRQ_EN) {
> > +            qemu_irq_raise(s->irq);
> > +        }
> >           break;
> >       default:
> >           break;

Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Cédric Le Goater 11 months, 3 weeks ago
On 2/21/25 06:43, Jamin Lin wrote:
> Hi Cedric,
> 
>> Subject: Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the
>> Crypto Manager Self Test
>>
>> On 2/13/25 04:35, Jamin Lin wrote:
>>> Currently, it does not support the CRYPT command. Instead, it only
>>> sends an interrupt to notify the firmware that the crypt command has
>> completed.
>>> It is a temporary workaround to resolve the boot issue in the Crypto
>>> Manager Self Test.
>>>
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>
>> Please add an AspeedHACEClass class attribute (bool) to handle this
>> workaround and a comment in the code mentioning the issue.
>>
> Thanks for review and suggestion.
> I will add the use_crypt_workaround attribute to the AspeedHACEClass and introduce the use-crypt-workaround property.
> Do you have any concerns, or do you have a preferred naming convention?

May be 'raise_crypt_interrupt_workaround' to reflect what is being done ?

Thanks,

C.



> Thanks-Jamin
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> ---
>>>    hw/misc/aspeed_hace.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
>>> 86422cb3be..4d0999e7e9 100644
>>> --- a/hw/misc/aspeed_hace.c
>>> +++ b/hw/misc/aspeed_hace.c
>>> @@ -59,6 +59,7 @@
>>>    /* Other cmd bits */
>>>    #define  HASH_IRQ_EN                    BIT(9)
>>>    #define  HASH_SG_EN                     BIT(18)
>>> +#define  CRYPT_IRQ_EN                   BIT(12)
>>>    /* Scatter-gather data list */
>>>    #define SG_LIST_LEN_SIZE                4
>>>    #define SG_LIST_LEN_MASK                0x0FFFFFFF
>>> @@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque,
>> hwaddr addr, uint64_t data,
>>>                    qemu_irq_lower(s->irq);
>>>                }
>>>            }
>>> +        if (data & CRYPT_IRQ) {
>>> +            data &= ~CRYPT_IRQ;
>>> +
>>> +            if (s->regs[addr] & CRYPT_IRQ) {
>>> +                qemu_irq_lower(s->irq);
>>> +            }
>>> +        }
>>>            break;
>>>        case R_HASH_SRC:
>>>            data &= ahc->src_mask;
>>> @@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque,
>> hwaddr addr, uint64_t data,
>>>        case R_CRYPT_CMD:
>>>            qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not
>> implemented\n",
>>>                           __func__);
>>> +        s->regs[R_STATUS] |= CRYPT_IRQ;
>>> +        if (data & CRYPT_IRQ_EN) {
>>> +            qemu_irq_raise(s->irq);
>>> +        }
>>>            break;
>>>        default:
>>>            break;
>
RE: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Jamin Lin 11 months, 2 weeks ago
Hi Cedric,

> Subject: Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the
> Crypto Manager Self Test
> 
> On 2/21/25 06:43, Jamin Lin wrote:
> > Hi Cedric,
> >
> >> Subject: Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in
> >> the Crypto Manager Self Test
> >>
> >> On 2/13/25 04:35, Jamin Lin wrote:
> >>> Currently, it does not support the CRYPT command. Instead, it only
> >>> sends an interrupt to notify the firmware that the crypt command has
> >> completed.
> >>> It is a temporary workaround to resolve the boot issue in the Crypto
> >>> Manager Self Test.
> >>>
> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>
> >> Please add an AspeedHACEClass class attribute (bool) to handle this
> >> workaround and a comment in the code mentioning the issue.
> >>
> > Thanks for review and suggestion.
> > I will add the use_crypt_workaround attribute to the AspeedHACEClass and
> introduce the use-crypt-workaround property.
> > Do you have any concerns, or do you have a preferred naming convention?
> 
> May be 'raise_crypt_interrupt_workaround' to reflect what is being done ?
> 
Thanks for your suggestion.
Will add it.

Jamin

> Thanks,
> 
> C.
> 
> 
> 
> > Thanks-Jamin
> >
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>
> >>> ---
> >>>    hw/misc/aspeed_hace.c | 12 ++++++++++++
> >>>    1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> >>> 86422cb3be..4d0999e7e9 100644
> >>> --- a/hw/misc/aspeed_hace.c
> >>> +++ b/hw/misc/aspeed_hace.c
> >>> @@ -59,6 +59,7 @@
> >>>    /* Other cmd bits */
> >>>    #define  HASH_IRQ_EN                    BIT(9)
> >>>    #define  HASH_SG_EN                     BIT(18)
> >>> +#define  CRYPT_IRQ_EN                   BIT(12)
> >>>    /* Scatter-gather data list */
> >>>    #define SG_LIST_LEN_SIZE                4
> >>>    #define SG_LIST_LEN_MASK                0x0FFFFFFF
> >>> @@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque,
> >> hwaddr addr, uint64_t data,
> >>>                    qemu_irq_lower(s->irq);
> >>>                }
> >>>            }
> >>> +        if (data & CRYPT_IRQ) {
> >>> +            data &= ~CRYPT_IRQ;
> >>> +
> >>> +            if (s->regs[addr] & CRYPT_IRQ) {
> >>> +                qemu_irq_lower(s->irq);
> >>> +            }
> >>> +        }
> >>>            break;
> >>>        case R_HASH_SRC:
> >>>            data &= ahc->src_mask;
> >>> @@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque,
> >> hwaddr addr, uint64_t data,
> >>>        case R_CRYPT_CMD:
> >>>            qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not
> >> implemented\n",
> >>>                           __func__);
> >>> +        s->regs[R_STATUS] |= CRYPT_IRQ;
> >>> +        if (data & CRYPT_IRQ_EN) {
> >>> +            qemu_irq_raise(s->irq);
> >>> +        }
> >>>            break;
> >>>        default:
> >>>            break;
> >

RE: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in the Crypto Manager Self Test
Posted by Jamin Lin 11 months, 3 weeks ago
Hi Cedric, 

> 
> Hi Cedric,
> 
> > Subject: Re: [PATCH v3 21/28] hw/misc/aspeed_hace: Fix boot issue in
> > the Crypto Manager Self Test
> >
> > On 2/13/25 04:35, Jamin Lin wrote:
> > > Currently, it does not support the CRYPT command. Instead, it only
> > > sends an interrupt to notify the firmware that the crypt command has
> > completed.
> > > It is a temporary workaround to resolve the boot issue in the Crypto
> > > Manager Self Test.
> > >
> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >
> > Please add an AspeedHACEClass class attribute (bool) to handle this
> > workaround and a comment in the code mentioning the issue.
> >
> Thanks for review and suggestion.
> I will add the use_crypt_workaround attribute to the AspeedHACEClass and
> introduce the use-crypt-workaround property.
> Do you have any concerns, or do you have a preferred naming convention?


Update my comments.
I do not need to create a property. I can set use_crypt_workaround to true in aspeed_ast2700_hace_class_init
Thanks-Jamin

> Thanks-Jamin
> 
> >
> > Thanks,
> >
> > C.
> >
> >
> >
> > > ---
> > >   hw/misc/aspeed_hace.c | 12 ++++++++++++
> > >   1 file changed, 12 insertions(+)
> > >
> > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index
> > > 86422cb3be..4d0999e7e9 100644
> > > --- a/hw/misc/aspeed_hace.c
> > > +++ b/hw/misc/aspeed_hace.c
> > > @@ -59,6 +59,7 @@
> > >   /* Other cmd bits */
> > >   #define  HASH_IRQ_EN                    BIT(9)
> > >   #define  HASH_SG_EN                     BIT(18)
> > > +#define  CRYPT_IRQ_EN                   BIT(12)
> > >   /* Scatter-gather data list */
> > >   #define SG_LIST_LEN_SIZE                4
> > >   #define SG_LIST_LEN_MASK                0x0FFFFFFF
> > > @@ -343,6 +344,13 @@ static void aspeed_hace_write(void *opaque,
> > hwaddr addr, uint64_t data,
> > >                   qemu_irq_lower(s->irq);
> > >               }
> > >           }
> > > +        if (data & CRYPT_IRQ) {
> > > +            data &= ~CRYPT_IRQ;
> > > +
> > > +            if (s->regs[addr] & CRYPT_IRQ) {
> > > +                qemu_irq_lower(s->irq);
> > > +            }
> > > +        }
> > >           break;
> > >       case R_HASH_SRC:
> > >           data &= ahc->src_mask;
> > > @@ -388,6 +396,10 @@ static void aspeed_hace_write(void *opaque,
> > hwaddr addr, uint64_t data,
> > >       case R_CRYPT_CMD:
> > >           qemu_log_mask(LOG_UNIMP, "%s: Crypt commands not
> > implemented\n",
> > >                          __func__);
> > > +        s->regs[R_STATUS] |= CRYPT_IRQ;
> > > +        if (data & CRYPT_IRQ_EN) {
> > > +            qemu_irq_raise(s->irq);
> > > +        }
> > >           break;
> > >       default:
> > >           break;