Hi Cédric, > Subject: Re: [PATCH v1 07/22] hw/misc/aspeed_hace: Add support for source, > digest, key buffer 64 bit addresses > > On 3/21/25 10:26, Jamin Lin wrote: > > According to the AST2700 design, the data source address is 64-bit, > > with R_HASH_SRC_HI storing bits [63:32] and R_HASH_SRC storing bits > [31:0]. > > Similarly, the digest address is 64-bit, with R_HASH_DEST_HI storing > > bits > > R_HASH_DIGEST_HIGH would be easier to understand. > Will update it.
> > [63:32] and R_HASH_DEST storing bits [31:0]. The HMAC key buffer > > address is also 64-bit, with R_HASH_KEY_BUFF_HI storing bits [63:32] > > and R_HASH_KEY_BUFF storing bits [31:0]. > > > > The AST2700 supports a maximum DRAM size of 8 GB, with a DRAM > > addressable range from 0x0_0000_0000 to 0x1_FFFF_FFFF. Since this > > range fits within 34 bits, only bits [33:0] are needed to store the > > DRAM offset. To optimize address storage, the high physical address > > bits [1:0] of the source, digest and key buffer addresses are stored as > dram_offset bits [33:32]. > > > > To achieve this, a src_hi_mask with a mask value of 0x3 is introduced, > > ensuring that src_addr_hi consists of bits [1:0]. The final src_addr > > is computed as (src_addr_hi[1:0] << 32) | src_addr[31:0], representing > > the DRAM offset within bits [33:0]. > > > > Similarly, a dest_hi_mask with a mask value of 0x3 is introduced to > > ensure that dest_addr_hi consists of bits [1:0]. The final dest_addr > > is calculated as (dest_addr_hi[1:0] << 32) | dest_addr[31:0], > > representing the DRAM offset within bits [33:0]. > > > > Additionally, a key_hi_mask with a mask value of 0x3 is introduced to > > ensure that key_buf_addr_hi consists of bits [1:0]. The final > > key_buf_addr is determined as (key_buf_addr_hi[1:0] << 32) | > > key_buf_addr[31:0], representing the DRAM offset within bits [33:0]. > > What does the datasheet say regarding the High Address registers ? > Are bits [1:0] RW and [31:2] RO ? > > > > This approach eliminates the need to reduce the high part of the DRAM > > physical address for DMA operations. Previously, this was calculated > > as (high physical address bits [7:0] - 4), since the DRAM start > > address is 0x4_00000000, making the high part address [7:0] - 4. > > I don't understand this part. Is there a difference between AST2700 > A0 and A1 ? > A0 and A1 designs are identical. For example: The HACE model sets its DRAM memory region to the SoC’s dram_mr memory region. The SoC’s dram_mr represents the DRAM region starting at address 0x4_0000_0000. https://github.com/qemu/qemu/blob/master/hw/arm/aspeed_ast27x0.c#L926 /* HACE */ object_property_set_link(OBJECT(&s->hace), "dram", OBJECT(s->dram_mr), &error_abort); When FW sets the source buffer address to 0x4_XXXX_XXXX, the HACE model must convert this to a DRAM offset using: offset = 0x4_XXXX_XXXX - 0x4_0000_0000 This ensures compatibility with QEMU memory access APIs like: address_space_write (&s->dram_as....) address_space_ldl_le(&s->dram_as...) There are two approaches to convert the source address into a DRAM offset 1. This method subtracts the DRAM base address explicitly. The SPI controller model (SMC/FMC) uses this approach. https://github.com/qemu/qemu/commit/ee48fef06c034ff245db9e553dcf0f1262f97bd2 https://github.com/qemu/qemu/commit/6330be8da44cf11e429197187e814299eff881cd Reference: https://github.com/qemu/qemu/blob/master/hw/arm/aspeed_ast27x0.c#L725C4-L728C43 /* FMC, The number of CS is set at the board level */ object_property_set_int(OBJECT(&s->fmc), "dram-base", sc->memmap[ASPEED_DEV_SDRAM], &error_abort); https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L948 if (aspeed_smc_has_dma64(asc)) { dma_dram_offset = dma_dram_addr - s->dram_base; } else { dma_dram_offset = dma_dram_addr; } 2. Using this solution in HACE model The AST2700 supports a maximum DRAM size of 8 GB, with a DRAM addressable range from 0x0_0000_0000 to 0x1_FFFF_FFFF. Since this range fits within 34 bits, only bits [33:0] are needed to store the DRAM offset. Therefore, I set the high mark address to 0x03. In this solution we don’t need to create a new property. I2c model use this solution, too https://github.com/qemu/qemu/commit/be8c15118a24491ccb2e7451f74f8efca1dd149c > > > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com> > > --- > > include/hw/misc/aspeed_hace.h | 5 ++++- > > hw/misc/aspeed_hace.c | 29 > +++++++++++++++++++++++++++++ > > 2 files changed, 33 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/misc/aspeed_hace.h > > b/include/hw/misc/aspeed_hace.h index b69a038d35..a4479bd383 100644 > > --- a/include/hw/misc/aspeed_hace.h > > +++ b/include/hw/misc/aspeed_hace.h > > @@ -22,7 +22,7 @@ > > > > OBJECT_DECLARE_TYPE(AspeedHACEState, AspeedHACEClass, > ASPEED_HACE) > > > > -#define ASPEED_HACE_NR_REGS (0x64 >> 2) > > +#define ASPEED_HACE_NR_REGS (0x9C >> 2) > > I think we need a class attribute. > > > #define ASPEED_HACE_MAX_SG 256 /* max number of entries */ > > > > struct AspeedHACEState { > > @@ -49,6 +49,9 @@ struct AspeedHACEClass { > > uint32_t key_mask; > > uint32_t hash_mask; > > bool raise_crypt_interrupt_workaround; > > + uint32_t src_hi_mask; > > + uint32_t dest_hi_mask; > > + uint32_t key_hi_mask; > > }; > > > > #endif /* ASPEED_HACE_H */ > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > > d06158dffd..51c6523fab 100644 > > --- a/hw/misc/aspeed_hace.c > > +++ b/hw/misc/aspeed_hace.c > > @@ -30,6 +30,9 @@ > > #define R_HASH_DEST (0x24 / 4) > > #define R_HASH_KEY_BUFF (0x28 / 4) > > #define R_HASH_SRC_LEN (0x2c / 4) > > +#define R_HASH_SRC_HI (0x90 / 4) > > +#define R_HASH_DEST_HI (0x94 / 4) > > +#define R_HASH_KEY_BUFF_HI (0x98 / 4) > > > > #define R_HASH_CMD (0x30 / 4) > > /* Hash algorithm selection */ > > @@ -393,6 +396,15 @@ static void aspeed_hace_write(void *opaque, > hwaddr addr, uint64_t data, > > } > > } > > break; > > + case R_HASH_SRC_HI: > > + data &= ahc->src_hi_mask; > > + break; > > + case R_HASH_DEST_HI: > > + data &= ahc->dest_hi_mask; > > + break; > > + case R_HASH_KEY_BUFF_HI: > > + data &= ahc->key_hi_mask; > > + break; > > This change exposes the high address registers to all SoCs which is > unfortunate. > > I would introduce a class attribute to limit the number of registers per SoC. > > You could also size the MMIO aperture with this new attribute and remove the > "Out-of-bounds" message at the beginning of the read/write memops. Will do. I introduce a new mem_size class attribute to set the different memory size for different SOCs in patch 10. https://patchwork.kernel.org/project/qemu-devel/patch/20250321092623.2097234-11-jamin_...@aspeedtech.com/ Solution 1: Should I use this mem_size class attribute to set it to register spaces instead of memory size and rename it to reg_szie? ahc->reg_size = 0x64 >> 2 for AST2500, AST2600, AST1030 ahc->reg_size = 0x9C >> 2 for AST2700 Solution2: Keep patch 10. And Introduce a new subregion for register size 2700 Container ----> mem_szie 0x100) Sub region ---> reg_size (0x9C >> 2) 2600 Container --> mem_size 0x10000 Sub region --> reg_size (0x64 >> 2) Could you please tell me which solution you prefer? Thanks-Jamin > Thanks, > > C. > > > > > default: > > break; > > } > > @@ -566,6 +578,23 @@ static void > aspeed_ast2700_hace_class_init(ObjectClass *klass, void *data) > > ahc->key_mask = 0x7FFFFFF8; > > ahc->hash_mask = 0x00147FFF; > > > > + /* > > + * The AST2700 supports a maximum DRAM size of 8 GB, with a > DRAM > > + * addressable range from 0x0_0000_0000 to 0x1_FFFF_FFFF. Since > this range > > + * fits within 34 bits, only bits [33:0] are needed to store the DRAM > > + * offset. To optimize address storage, the high physical address bits > > + * [1:0] of the source, digest and key buffer addresses are stored as > > + * dram_offset bits [33:32]. > > + * > > + * This approach eliminates the need to reduce the high part of the > DRAM > > + * physical address for DMA operations. Previously, this was calculated > as > > + * (high physical address bits [7:0] - 4), since the DRAM start address > is > > + * 0x4_00000000, making the high part address [7:0] - 4. > > + */ > > + ahc->src_hi_mask = 0x00000003; > > + ahc->dest_hi_mask = 0x00000003; > > + ahc->key_hi_mask = 0x00000003; > > + > > /* > > * Currently, it does not support the CRYPT command. Instead, it > only > > * sends an interrupt to notify the firmware that the crypt > > command
