[PATCH v1 0/3] Fix RAM size detection failure on BE hosts

[PATCH v1 0/3] Fix RAM size detection failure on BE hosts

[Jamin Lin via]

v1:
 1. Fix RAM size detection failure on BE hosts
 2. INTC: Set impl.min_access_size to 4
          Fix coding style

Jamin Lin (3):
  hw/intc/aspeed: Set impl.min_access_size to 4
  hw/intc/aspeed Fix coding style
  hw/arm/aspeed_ast27x0: Fix RAM size detection failure on BE hosts

 hw/arm/aspeed_ast27x0.c | 21 ++++++++++++++++-----
 hw/intc/aspeed_intc.c   | 12 ++++++++++--
 2 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] hw/intc/aspeed: Set impl.min_access_size to 4

[Jamin Lin via]

This patch explicitly sets ".impl.min_access_size = 4" to match the
declared ".valid.min_access_size = 4", enforcing stricter access size
checking and preventing inconsistent partial accesses to the interrupt
controller registers.

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

diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 33fcbe729c..19f88853d8 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -737,6 +737,7 @@ static const MemoryRegionOps aspeed_intc_ops = {
     .read = aspeed_intc_read,
     .write = aspeed_intc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -747,6 +748,7 @@ static const MemoryRegionOps aspeed_intcio_ops = {
     .read = aspeed_intcio_read,
     .write = aspeed_intcio_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -757,6 +759,7 @@ static const MemoryRegionOps aspeed_ssp_intc_ops = {
     .read = aspeed_intc_read,
     .write = aspeed_ssp_intc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -767,6 +770,7 @@ static const MemoryRegionOps aspeed_ssp_intcio_ops = {
     .read = aspeed_intcio_read,
     .write = aspeed_ssp_intcio_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -777,6 +781,7 @@ static const MemoryRegionOps aspeed_tsp_intc_ops = {
     .read = aspeed_intc_read,
     .write = aspeed_tsp_intc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
@@ -787,6 +792,7 @@ static const MemoryRegionOps aspeed_tsp_intcio_ops = {
     .read = aspeed_intcio_read,
     .write = aspeed_tsp_intcio_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
-- 
2.43.0

Re: [PATCH v1 1/3] hw/intc/aspeed: Set impl.min_access_size to 4

[Cédric Le Goater]

On 5/20/25 09:35, Jamin Lin wrote:
> This patch explicitly sets ".impl.min_access_size = 4" to match the
> declared ".valid.min_access_size = 4", enforcing stricter access size
> checking and preventing inconsistent partial accesses to the interrupt
> controller registers.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/intc/aspeed_intc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 33fcbe729c..19f88853d8 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -737,6 +737,7 @@ static const MemoryRegionOps aspeed_intc_ops = {
>       .read = aspeed_intc_read,
>       .write = aspeed_intc_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,
> @@ -747,6 +748,7 @@ static const MemoryRegionOps aspeed_intcio_ops = {
>       .read = aspeed_intcio_read,
>       .write = aspeed_intcio_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,
> @@ -757,6 +759,7 @@ static const MemoryRegionOps aspeed_ssp_intc_ops = {
>       .read = aspeed_intc_read,
>       .write = aspeed_ssp_intc_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,
> @@ -767,6 +770,7 @@ static const MemoryRegionOps aspeed_ssp_intcio_ops = {
>       .read = aspeed_intcio_read,
>       .write = aspeed_ssp_intcio_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,
> @@ -777,6 +781,7 @@ static const MemoryRegionOps aspeed_tsp_intc_ops = {
>       .read = aspeed_intc_read,
>       .write = aspeed_tsp_intc_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,
> @@ -787,6 +792,7 @@ static const MemoryRegionOps aspeed_tsp_intcio_ops = {
>       .read = aspeed_intcio_read,
>       .write = aspeed_tsp_intcio_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
>           .min_access_size = 4,
>           .max_access_size = 4,

[PATCH v1 2/3] hw/intc/aspeed Fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/intc/aspeed_intc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 19f88853d8..5cd786dee6 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -1001,7 +1001,8 @@ static AspeedINTCIRQ aspeed_2700ssp_intcio_irqs[ASPEED_INTC_MAX_INPINS] = {
     {5, 5, 1, R_SSPINT165_EN, R_SSPINT165_STATUS},
 };
 
-static void aspeed_2700ssp_intcio_class_init(ObjectClass *klass, const void *data)
+static void aspeed_2700ssp_intcio_class_init(ObjectClass *klass,
+                                             const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
@@ -1069,7 +1070,8 @@ static AspeedINTCIRQ aspeed_2700tsp_intcio_irqs[ASPEED_INTC_MAX_INPINS] = {
     {5, 5, 1, R_TSPINT165_EN, R_TSPINT165_STATUS},
 };
 
-static void aspeed_2700tsp_intcio_class_init(ObjectClass *klass, const void *data)
+static void aspeed_2700tsp_intcio_class_init(ObjectClass *klass,
+                                             const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
-- 
2.43.0

Re: [PATCH v1 2/3] hw/intc/aspeed Fix coding style

[Cédric Le Goater]

On 5/20/25 09:35, Jamin Lin wrote:
> Fix coding style issues from checkpatch.pl.

This is just a warning since the line length is below 90 chars.

> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

Anyhow,


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/intc/aspeed_intc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 19f88853d8..5cd786dee6 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -1001,7 +1001,8 @@ static AspeedINTCIRQ aspeed_2700ssp_intcio_irqs[ASPEED_INTC_MAX_INPINS] = {
>       {5, 5, 1, R_SSPINT165_EN, R_SSPINT165_STATUS},
>   };
>   
> -static void aspeed_2700ssp_intcio_class_init(ObjectClass *klass, const void *data)
> +static void aspeed_2700ssp_intcio_class_init(ObjectClass *klass,
> +                                             const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
> @@ -1069,7 +1070,8 @@ static AspeedINTCIRQ aspeed_2700tsp_intcio_irqs[ASPEED_INTC_MAX_INPINS] = {
>       {5, 5, 1, R_TSPINT165_EN, R_TSPINT165_STATUS},
>   };
>   
> -static void aspeed_2700tsp_intcio_class_init(ObjectClass *klass, const void *data)
> +static void aspeed_2700tsp_intcio_class_init(ObjectClass *klass,
> +                                             const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);

[PATCH v1 3/3] hw/arm/aspeed_ast27x0: Fix RAM size detection failure on BE hosts

[Jamin Lin via]

On big-endian hosts, the aspeed_ram_capacity_write() function previously passed
the address of a 64-bit "data" variable directly to address_space_write(),
assuming host and guest endianness matched.

However, the data is expected to be written in little-endian format to DRAM.
On big-endian hosts, this led to incorrect data being written into DRAM,
which caused the guest firmware to misdetect the DRAM size.

As a result, U-Boot fails to boot and hangs.

- Explicitly converting the 32-bit portion of "data" to little-endian format
  using cpu_to_le32(), storing it in a temporary "uint32_t le_data".
- Updating the MemoryRegionOps to restrict access to exactly 4 bytes
  using .valid.{min,max}_access_size = 4 and .impl.min_access_size = 4.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Fixes: 7436db1 ("aspeed/soc: fix incorrect dram size for AST2700")
---
 hw/arm/aspeed_ast27x0.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 1974a25766..7ed0919b3f 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -335,24 +335,34 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
     AspeedSoCState *s = ASPEED_SOC(opaque);
     ram_addr_t ram_size;
     MemTxResult result;
+    uint32_t le_data;
 
     ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
                                         &error_abort);
 
     assert(ram_size > 0);
 
+    if (size != 4) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unsupported write size: %d (only 4-byte allowed)\n",
+                      __func__, size);
+        return;
+    }
+
+    le_data = cpu_to_le32((uint32_t)data);
+
     /*
      * Emulate ddr capacity hardware behavior.
      * If writes the data to the address which is beyond the ram size,
      * it would write the data to the "address % ram_size".
      */
     result = address_space_write(&s->dram_as, addr % ram_size,
-                                 MEMTXATTRS_UNSPECIFIED, &data, 4);
+                                 MEMTXATTRS_UNSPECIFIED, &le_data, 4);
     if (result != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: DRAM write failed, addr:0x%" HWADDR_PRIx
-                      ", data :0x%" PRIx64  "\n",
-                      __func__, addr % ram_size, data);
+                      ", data :0x%x\n",
+                      __func__, addr % ram_size, le_data);
     }
 }
 
@@ -360,9 +370,10 @@ static const MemoryRegionOps aspeed_ram_capacity_ops = {
     .read = aspeed_ram_capacity_read,
     .write = aspeed_ram_capacity_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
     .valid = {
-        .min_access_size = 1,
-        .max_access_size = 8,
+        .min_access_size = 4,
+        .max_access_size = 4,
     },
 };
 
-- 
2.43.0

Re: [PATCH v1 3/3] hw/arm/aspeed_ast27x0: Fix RAM size detection failure on BE hosts

[Cédric Le Goater]

On 5/20/25 09:35, Jamin Lin wrote:
> On big-endian hosts, the aspeed_ram_capacity_write() function previously passed
> the address of a 64-bit "data" variable directly to address_space_write(),
> assuming host and guest endianness matched.
> 
> However, the data is expected to be written in little-endian format to DRAM.
> On big-endian hosts, this led to incorrect data being written into DRAM,
> which caused the guest firmware to misdetect the DRAM size.
> 
> As a result, U-Boot fails to boot and hangs.
> 
> - Explicitly converting the 32-bit portion of "data" to little-endian format
>    using cpu_to_le32(), storing it in a temporary "uint32_t le_data".
> - Updating the MemoryRegionOps to restrict access to exactly 4 bytes
>    using .valid.{min,max}_access_size = 4 and .impl.min_access_size = 4.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Fixes: 7436db1 ("aspeed/soc: fix incorrect dram size for AST2700")
> ---
>   hw/arm/aspeed_ast27x0.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 1974a25766..7ed0919b3f 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -335,24 +335,34 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
>       AspeedSoCState *s = ASPEED_SOC(opaque);
>       ram_addr_t ram_size;
>       MemTxResult result;
> +    uint32_t le_data;
>   
>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>                                           &error_abort);
>   
>       assert(ram_size > 0);
>   
> +    if (size != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unsupported write size: %d (only 4-byte allowed)\n",
> +                      __func__, size);
> +        return;
> +    }

The core memory subsystem should find such issues if the valid attributes
of MemoryRegionOps are set correctly.

> +    le_data = cpu_to_le32((uint32_t)data);
> +
>       /*
>        * Emulate ddr capacity hardware behavior.
>        * If writes the data to the address which is beyond the ram size,
>        * it would write the data to the "address % ram_size".
>        */
>       result = address_space_write(&s->dram_as, addr % ram_size,
> -                                 MEMTXATTRS_UNSPECIFIED, &data, 4);
> +                                 MEMTXATTRS_UNSPECIFIED, &le_data, 4);


This should be enough :

     address_space_stl_le(&s->dram_as, addr % ram_size, data,
                          MEMTXATTRS_UNSPECIFIED, &result);

Sorry for not spotting this earlier. Finding a BE host is difficult.
Thanks for the time you spent on fixing this issue.

C.


>       if (result != MEMTX_OK) {
>           qemu_log_mask(LOG_GUEST_ERROR,
>                         "%s: DRAM write failed, addr:0x%" HWADDR_PRIx
> -                      ", data :0x%" PRIx64  "\n",
> -                      __func__, addr % ram_size, data);
> +                      ", data :0x%x\n",
> +                      __func__, addr % ram_size, le_data);
>       }
>   }
>   
> @@ -360,9 +370,10 @@ static const MemoryRegionOps aspeed_ram_capacity_ops = {
>       .read = aspeed_ram_capacity_read,
>       .write = aspeed_ram_capacity_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
>       .valid = {
> -        .min_access_size = 1,
> -        .max_access_size = 8,
> +        .min_access_size = 4,
> +        .max_access_size = 4,
>       },
>   };
>   

RE: [PATCH v1 3/3] hw/arm/aspeed_ast27x0: Fix RAM size detection failure on BE hosts

[Jamin Lin]

Hi Cédric

> Subject: Re: [PATCH v1 3/3] hw/arm/aspeed_ast27x0: Fix RAM size detection
> failure on BE hosts
> 
> On 5/20/25 09:35, Jamin Lin wrote:
> > On big-endian hosts, the aspeed_ram_capacity_write() function
> > previously passed the address of a 64-bit "data" variable directly to
> > address_space_write(), assuming host and guest endianness matched.
> >
> > However, the data is expected to be written in little-endian format to DRAM.
> > On big-endian hosts, this led to incorrect data being written into
> > DRAM, which caused the guest firmware to misdetect the DRAM size.
> >
> > As a result, U-Boot fails to boot and hangs.
> >
> > - Explicitly converting the 32-bit portion of "data" to little-endian format
> >    using cpu_to_le32(), storing it in a temporary "uint32_t le_data".
> > - Updating the MemoryRegionOps to restrict access to exactly 4 bytes
> >    using .valid.{min,max}_access_size = 4 and .impl.min_access_size = 4.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > Fixes: 7436db1 ("aspeed/soc: fix incorrect dram size for AST2700")
> > ---
> >   hw/arm/aspeed_ast27x0.c | 21 ++++++++++++++++-----
> >   1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > 1974a25766..7ed0919b3f 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -335,24 +335,34 @@ static void aspeed_ram_capacity_write(void
> *opaque, hwaddr addr, uint64_t data,
> >       AspeedSoCState *s = ASPEED_SOC(opaque);
> >       ram_addr_t ram_size;
> >       MemTxResult result;
> > +    uint32_t le_data;
> >
> >       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
> >                                           &error_abort);
> >
> >       assert(ram_size > 0);
> >
> > +    if (size != 4) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unsupported write size: %d (only 4-byte
> allowed)\n",
> > +                      __func__, size);
> > +        return;
> > +    }
> 
> The core memory subsystem should find such issues if the valid attributes of
> MemoryRegionOps are set correctly.
> 
> > +    le_data = cpu_to_le32((uint32_t)data);
> > +
> >       /*
> >        * Emulate ddr capacity hardware behavior.
> >        * If writes the data to the address which is beyond the ram size,
> >        * it would write the data to the "address % ram_size".
> >        */
> >       result = address_space_write(&s->dram_as, addr % ram_size,
> > -                                 MEMTXATTRS_UNSPECIFIED, &data,
> 4);
> > +                                 MEMTXATTRS_UNSPECIFIED,
> &le_data,
> > + 4);
> 
> 
> This should be enough :
> 
>      address_space_stl_le(&s->dram_as, addr % ram_size, data,
>                           MEMTXATTRS_UNSPECIFIED, &result);
> 
> Sorry for not spotting this earlier. Finding a BE host is difficult.
> Thanks for the time you spent on fixing this issue.
> 
Thanks for the review and suggestion.
Will update it.
Jamin
> C.
> 
> 
> >       if (result != MEMTX_OK) {
> >           qemu_log_mask(LOG_GUEST_ERROR,
> >                         "%s: DRAM write failed, addr:0x%"
> HWADDR_PRIx
> > -                      ", data :0x%" PRIx64  "\n",
> > -                      __func__, addr % ram_size, data);
> > +                      ", data :0x%x\n",
> > +                      __func__, addr % ram_size, le_data);
> >       }
> >   }
> >
> > @@ -360,9 +370,10 @@ static const MemoryRegionOps
> aspeed_ram_capacity_ops = {
> >       .read = aspeed_ram_capacity_read,
> >       .write = aspeed_ram_capacity_write,
> >       .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .impl.min_access_size = 4,
> >       .valid = {
> > -        .min_access_size = 1,
> > -        .max_access_size = 8,
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> >       },
> >   };
> >