RE: [PATCH v1 00/18] Support AST2700 A1

[Jamin Lin]

Hi Cedric, 

> From: Cédric Le Goater <clg@kaod.org>
> Sent: Friday, January 31, 2025 3:34 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 00/18] Support AST2700 A1
> 
> Hello Jamin,
> 
> On 1/21/25 08:04, Jamin Lin wrote:
> > v1:
> >   1. Refactor INTC model to support both INTC0 and INTC1.
> >   2. Support AST2700 A1.
> >   3. Create ast2700a0-evb machine.
> >
> > With the patch applied, QEMU now supports two machines for running
> AST2700 SoCs:
> > ast2700a0-evb: Designed for AST2700 A0
> > ast2700-evb: Designed for AST2700 A1
> 
> Let's do that progressively, please add a new ast2700a1-evb machine too and a
> machine alias on ast2700a0-evb. Then we will change the alias on the newer
> SoC.
> 
Will add
> 
> >
> > Test information
> > 1. QEMU version:
> >
> https://github.com/qemu/qemu/commit/d6430c17d7113d3c38480dc34e59d0
> 0b05
> > 04e2f7 2. This patch series depends on the following patch series
> >
> https://patchwork.kernel.org/project/qemu-devel/cover/20250113064455.166
> 0564-1-jamin_lin@aspeedtech.com/
> >
> > https://patchwork.kernel.org/project/qemu-devel/cover/20241114094839.4
> > 128404-1-jamin_lin@aspeedtech.com/
> > 3. ASPEED SDK v09.05 pre-built image
> >     https://github.com/AspeedTech-BMC/openbmc/releases/tag/v09.05
> >     ast2700-default-obmc.tar.gz (A1)
> >
> >
> https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.05/ast
> > 2700-default-obmc.tar.gz
> 
> We need a functional test case for the new A1 SoC please.
> 
Will add
> 
> 
> Also, could please split the patchset in 3 and resend :
> 
> 1. INTC rename/prereqs/cleanups
> 
>    hw/intc/aspeed: Rename INTC to INTC0
>    hw/intc/aspeed: Support different memory region ops
>    hw/intc/aspeed: Introduce a new aspeed_2700_intc0_ops for INTC0
>    hw/intc/aspeed: Support setting different memory and register size
>    hw/intc/aspeed: Introduce helper functions for enable and status registers
>    hw/intc/aspeed: Add ID to trace events for better debugging
>    hw/arm/aspeed: Rename IRQ table and machine name for AST2700 A0
> 
> 2. AST2700A1 support
> 
>    hw/misc/aspeed_scu: Add Support for AST2700/AST2750 A1 Silicon
> Revisions
>    hw/intc/aspeed: Introduce AspeedINTCIRQ structure to save the irq index
> and register address
>    hw/intc/aspeed: Introduce IRQ handler function to reduce code duplication
>    hw/intc/aspeed: Add Support for Multi-Output IRQ Handling
>    hw/intc/aspeed: Add Support for AST2700 INTC1 Controller
>    hw/arm/aspeed_ast27x0: Support two levels of INTC controllers for
> AST2700 A1
>    hw/arm/aspeed: Add SoC and Machine Support for AST2700 A1
> 
> 3. HACE support
> 
>    hw/misc/aspeed_hace: Fix coding style
>    hw/misc/aspeed_hace: Add AST2700 support
>    hw/arm/aspeed_ast27x0: Add HACE support for AST2700
>    hw/misc/aspeed_hace: (DROP) Fix boot issue in the Crypto Manager Self
> Test(WORKAROUND)
> 
> It seems that 1+3 could be reviewed and merged quickly. 2 is definitely more
> complex.
> 
Will re-send 3 patch series for v2
 
> > Known Issue:
> > The HACE crypto and hash engine is enable by default since AST2700 A1.
> > However, aspeed_hace.c(HACE model) currently does not support the
> CRYPTO command.
> 
> Do we need support from the QEMU crypto subsystem or do we only lack an
> implementation for the model ?
> 

We currently lack an implementation for the HACE model.
While it supports the HASH module, the CRYPT module is not yet implemented 
(aspeed_hace.c).
Additionally, I am investigating which "QCrypto common API" can be used to 
support AES (ECB, CBC, CFB, CTR, GCM, etc.) and DES.
https://github.com/qemu/qemu/tree/master/crypto 

Do you have any suggestion?

> > To boot AST2700 A1, I have created a Patch 18 which temporarily
> > resolves the issue by sending an interrupt to notify the firmware that
> > the cryptographic command has completed. It is a temporary workaround
> > to resolve the boot issue in the Crypto Manager SelfTest.
> 
> could you test the silicon revision in the model for this workaround ?
> 

I have tested our pre-built image v09.05 and it worked well. You can download 
it from:
https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.05/ast2700-default-obmc.tar.gz
 

```
U-Boot 2023.10-v00.05.05 (Jan 22 2025 - 07:38:51 +0000)

SOC: AST2700-A1
```

Thanks for suggestion and review.
Jamin

> 
> Thanks,
> 
> C.
> 
> 
> 
> > As a result, you will encounter the following kernel warning due to
> > the Crypto Manager test failure. If you don't want to see these kernel
> > warning, please add the following settings in your kernel config.
> >
> > ```
> > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y
> > ```
> >
> > ```
> > alg: skcipher: aspeed-ctr-tdes encryption test failed (wrong result) on test
> vector 0, cfg="in-place (one sglist)"
> > [    5.035966] alg: self-tests for ctr(des3_ede) using aspeed-ctr-tdes 
> > failed
> (rc=-22)
> > [    5.036139] ------------[ cut here ]------------
> > [    5.037188] alg: self-tests for ctr(des3_ede) using aspeed-ctr-tdes 
> > failed
> (rc=-22)
> > [    5.037312] WARNING: CPU: 2 PID: 109 at /crypto/testmgr.c:5936
> alg_test+0x42c/0x548
> > [    5.038049] Modules linked in:
> > [    5.038302] CPU: 2 PID: 109 Comm: cryptomgr_test Tainted: G
> W          6.6.52-v00.06.04-gf52a0cf7c475 #1
> > [    5.038787] Hardware name: AST2700-EVB (DT)
> > [    5.038988] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> > [    5.039315] pc : alg_test+0x42c/0x548
> > [    5.039505] lr : alg_test+0x42c/0x548
> > [    5.039697] sp : ffffffc0825e3d50
> > [    5.039862] x29: ffffffc0825e3df0 x28: 0000000000000004 x27:
> 0000000000000000
> > [    5.040226] x26: ffffffc080bcada0 x25: ffffffc081dac9d0 x24:
> 0000000000000004
> > [    5.040700] x23: 0000000000001285 x22: ffffff8003ded280 x21:
> ffffff8003ded200
> > [    5.041458] x20: 00000000ffffffff x19: 00000000ffffffea x18: 
> > ffffffffffffffff
> > [    5.041915] x17: 282064656c696166 x16: 20736564742d7274 x15:
> 00000000fffffffe
> > [    5.042287] x14: 0000000000000000 x13: ffffffc081ba555c x12:
> 65742d666c657320
> > [    5.042684] x11: 00000000fffeffff x10: ffffffc0818ff048 x9 :
> ffffffc0800a36e4
> > [    5.043077] x8 : 000000000017ffe8 x7 : c0000000fffeffff x6 :
> 000000000057ffa8
> > [    5.043461] x5 : 000000000000ffff x4 : 0000000000000000 x3 :
> 0000000000000000
> > [    5.043751] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> ffffff800415e740
> > [    5.045544] Call trace:
> > [    5.045693]  alg_test+0x42c/0x548
> > [    5.045878]  cryptomgr_test+0x28/0x48
> > [    5.046052]  kthread+0x114/0x120
> > [    5.046226]  ret_from_fork+0x10/0x20
> > [    5.046413] ---[ end trace 0000000000000000 ]---
> > [    5.071510] alg: skcipher: aspeed-ctr-des encryption test failed (wrong
> result) on test vector 0, cfg="in-place (one sglist)"
> > [    5.072145] alg: self-tests for ctr(des) using aspeed-ctr-des failed
> (rc=-22)
> > ```
> >
> >
> > Jamin Lin (18):
> >    hw/intc/aspeed: Rename INTC to INTC0
> >    hw/intc/aspeed: Support different memory region ops
> >    hw/intc/aspeed: Introduce a new aspeed_2700_intc0_ops for INTC0
> >    hw/intc/aspeed: Support setting different memory and register size
> >    hw/intc/aspeed: Introduce helper functions for enable and status
> >      registers
> >    hw/intc/aspeed: Introduce AspeedINTCIRQ structure to save the irq
> >      index and register address
> >    hw/intc/aspeed: Introduce IRQ handler function to reduce code
> >      duplication
> >    hw/intc/aspeed: Add Support for Multi-Output IRQ Handling
> >    hw/intc/aspeed: Add ID to trace events for better debugging
> >    hw/intc/aspeed: Add Support for AST2700 INTC1 Controller
> >    hw/misc/aspeed_scu: Add Support for AST2700/AST2750 A1 Silicon
> >      Revisions
> >    hw/arm/aspeed_ast27x0: Support two levels of INTC controllers for
> >      AST2700 A1
> >    hw/arm/aspeed: Rename IRQ table and machine name for AST2700 A0
> >    hw/arm/aspeed: Add SoC and Machine Support for AST2700 A1
> >    hw/misc/aspeed_hace: Fix coding style
> >    hw/misc/aspeed_hace: Add AST2700 support
> >    hw/arm/aspeed_ast27x0: Add HACE support for AST2700
> >    hw/misc/aspeed_hace: (DROP) Fix boot issue in the Crypto Manager Self
> >      Test(WORKAROUND)
> >
> >   hw/arm/aspeed.c               |  26 +-
> >   hw/arm/aspeed_ast27x0.c       | 229 +++++++++--
> >   hw/intc/aspeed_intc.c         | 689
> +++++++++++++++++++++++++---------
> >   hw/intc/trace-events          |  25 +-
> >   hw/misc/aspeed_hace.c         |  44 ++-
> >   hw/misc/aspeed_scu.c          |   3 +
> >   include/hw/arm/aspeed_soc.h   |   5 +-
> >   include/hw/intc/aspeed_intc.h |  35 +-
> >   include/hw/misc/aspeed_hace.h |   1 +
> >   include/hw/misc/aspeed_scu.h  |   2 +
> >   10 files changed, 817 insertions(+), 242 deletions(-)
> >