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

[Cédric Le Goater]

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.


> Test information
> 1. QEMU version: 
> https://github.com/qemu/qemu/commit/d6430c17d7113d3c38480dc34e59d00b0504e2f7
> 2. This patch series depends on the following patch series
>     
> 20250113064455.1660564-1-jamin_lin@aspeedtech.com/">https://patchwork.kernel.org/project/qemu-devel/cover/20250113064455.1660564-1-jamin_lin@aspeedtech.com/
>     
> https://patchwork.kernel.org/project/qemu-devel/cover/20241114094839.4128404-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/ast2700-default-obmc.tar.gz

We need a functional test case for the new A1 SoC please.



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.
 
> 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 ?

> 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 ?


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(-)