[PATCH v2 0/5] hw/arm/aspeed_ast2700-fc: Fix null pointer dereference

[PATCH v2 0/5] hw/arm/aspeed_ast2700-fc: Fix null pointer dereference

[Steven Lee via]

Clang's sanitizer reports a runtime error when booting with
'-net nic -net user', due to a null pointer being passed
to memory_region_find(), which subsequently triggers a crash in
flatview_lookup().

Root cause:
- Missing NIC configuration in the CA35 initialization.

Fix:
- Reduce ca35 ram size from 2GiB to 1GiB to align with ast2700a1-evb,
  where the ram-container is defined as 1GiB in its class.
- Add nic configuration in ast2700fc's ca35 init function.


v2:
- Split the CA35 memory mapping into a separate patch.
- Added a new patch to fix BMC memory mapping in the fby35 machine,
  which had a similar issue (unmapped system_memory).
- Removed Change-Id tag from commit messages

Steven Lee (5):
  hw/arm/aspeed_ast2700-fc: Fix null pointer dereference in ca35 init
  hw/arm/aspeed_ast27x0: Fix unimplemented region overlap with vbootrom
  hw/arm/aspeed_ast27x0-fc: Map ca35 memory into system memory
  hw/arm/fby35: Map BMC memory into system memory
  docs: Remove ast2700fc from Aspeed family boards

 docs/system/arm/aspeed.rst |  2 +-
 hw/arm/aspeed_ast27x0-fc.c | 16 ++++++++++++++--
 hw/arm/aspeed_ast27x0.c    |  4 ++--
 hw/arm/fby35.c             |  1 +
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] hw/arm/aspeed_ast2700-fc: Fix null pointer dereference in ca35 init

[Steven Lee via]

Clang's sanitizer reports a runtime error when booting with
'-net nic -net user', due to a null pointer being passed
to memory_region_find(), which subsequently triggers a crash in
flatview_lookup().

Root cause:
- Missing NIC configuration in the CA35 initialization.

Fix:
- Reduce ca35 ram size from 2GiB to 1GiB to align with ast2700a1-evb,
  where the ram-container is defined as 1GiB in its class.
- Add nic configuration in ast2700fc's ca35 init function.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0-fc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
index 125a3ade40..ff64605663 100644
--- a/hw/arm/aspeed_ast27x0-fc.c
+++ b/hw/arm/aspeed_ast27x0-fc.c
@@ -48,7 +48,7 @@ struct Ast2700FCState {
     bool mmio_exec;
 };
 
-#define AST2700FC_BMC_RAM_SIZE (2 * GiB)
+#define AST2700FC_BMC_RAM_SIZE (1 * GiB)
 #define AST2700FC_CM4_DRAM_SIZE (32 * MiB)
 
 #define AST2700FC_HW_STRAP1 0x000000C0
@@ -59,6 +59,7 @@ struct Ast2700FCState {
 static void ast2700fc_ca35_init(MachineState *machine)
 {
     Ast2700FCState *s = AST2700A1FC(machine);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
     AspeedSoCState *soc;
     AspeedSoCClass *sc;
 
@@ -86,6 +87,14 @@ static void ast2700fc_ca35_init(MachineState *machine)
                                  AST2700FC_BMC_RAM_SIZE, &error_abort)) {
         return;
     }
+
+    for (int i = 0; i < sc->macs_num; i++) {
+        if ((amc->macs_mask & (1 << i)) &&
+            !qemu_configure_nic_device(DEVICE(&soc->ftgmac100[i]),
+                                       true, NULL)) {
+            break;
+        }
+    }
     if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
                                  AST2700FC_HW_STRAP1, &error_abort)) {
         return;
@@ -171,6 +180,7 @@ static void ast2700fc_init(MachineState *machine)
 static void ast2700fc_class_init(ObjectClass *oc, const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
 
     mc->alias = "ast2700fc";
     mc->desc = "ast2700 full core support";
@@ -178,12 +188,13 @@ static void ast2700fc_class_init(ObjectClass *oc, const void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->min_cpus = mc->max_cpus = mc->default_cpus = 6;
+    amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON;
 }
 
 static const TypeInfo ast2700fc_types[] = {
     {
         .name           = MACHINE_TYPE_NAME("ast2700fc"),
-        .parent         = TYPE_MACHINE,
+        .parent         = TYPE_ASPEED_MACHINE,
         .class_init     = ast2700fc_class_init,
         .instance_size  = sizeof(Ast2700FCState),
     },
-- 
2.43.0

Re: [PATCH v2 1/5] hw/arm/aspeed_ast2700-fc: Fix null pointer dereference in ca35 init

[Cédric Le Goater]

On 5/14/25 11:03, Steven Lee wrote:
> Clang's sanitizer reports a runtime error when booting with
> '-net nic -net user', due to a null pointer being passed
> to memory_region_find(), which subsequently triggers a crash in
> flatview_lookup().
> 
> Root cause:
> - Missing NIC configuration in the CA35 initialization.
> 
> Fix:
> - Reduce ca35 ram size from 2GiB to 1GiB to align with ast2700a1-evb,
>    where the ram-container is defined as 1GiB in its class.
> - Add nic configuration in ast2700fc's ca35 init function.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast27x0-fc.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> index 125a3ade40..ff64605663 100644
> --- a/hw/arm/aspeed_ast27x0-fc.c
> +++ b/hw/arm/aspeed_ast27x0-fc.c
> @@ -48,7 +48,7 @@ struct Ast2700FCState {
>       bool mmio_exec;
>   };
>   
> -#define AST2700FC_BMC_RAM_SIZE (2 * GiB)
> +#define AST2700FC_BMC_RAM_SIZE (1 * GiB)
>   #define AST2700FC_CM4_DRAM_SIZE (32 * MiB)
>   
>   #define AST2700FC_HW_STRAP1 0x000000C0
> @@ -59,6 +59,7 @@ struct Ast2700FCState {
>   static void ast2700fc_ca35_init(MachineState *machine)
>   {
>       Ast2700FCState *s = AST2700A1FC(machine);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
>       AspeedSoCState *soc;
>       AspeedSoCClass *sc;
>   
> @@ -86,6 +87,14 @@ static void ast2700fc_ca35_init(MachineState *machine)
>                                    AST2700FC_BMC_RAM_SIZE, &error_abort)) {
>           return;
>       }
> +
> +    for (int i = 0; i < sc->macs_num; i++) {
> +        if ((amc->macs_mask & (1 << i)) &&
> +            !qemu_configure_nic_device(DEVICE(&soc->ftgmac100[i]),
> +                                       true, NULL)) {
> +            break;
> +        }
> +    }
>       if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
>                                    AST2700FC_HW_STRAP1, &error_abort)) {
>           return;
> @@ -171,6 +180,7 @@ static void ast2700fc_init(MachineState *machine)
>   static void ast2700fc_class_init(ObjectClass *oc, const void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>   
>       mc->alias = "ast2700fc";
>       mc->desc = "ast2700 full core support";
> @@ -178,12 +188,13 @@ static void ast2700fc_class_init(ObjectClass *oc, const void *data)
>       mc->no_floppy = 1;
>       mc->no_cdrom = 1;
>       mc->min_cpus = mc->max_cpus = mc->default_cpus = 6;
> +    amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON;
>   }
>   
>   static const TypeInfo ast2700fc_types[] = {
>       {
>           .name           = MACHINE_TYPE_NAME("ast2700fc"),
> -        .parent         = TYPE_MACHINE,
> +        .parent         = TYPE_ASPEED_MACHINE,
>           .class_init     = ast2700fc_class_init,
>           .instance_size  = sizeof(Ast2700FCState),
>       },

The "ast2700fc" machine cannot inherit from TYPE_ASPEED_MACHINE.
These are two different type of machines.

An "ast2700fc" machine state is described by :

     struct Ast2700FCState {
         MachineState parent_obj;
     
         MemoryRegion ca35_memory;
         MemoryRegion ca35_dram;
         MemoryRegion ssp_memory;
         MemoryRegion tsp_memory;
     
         Clock *ssp_sysclk;
         Clock *tsp_sysclk;
     
         Aspeed27x0SoCState ca35;
         Aspeed27x0SSPSoCState ssp;
         Aspeed27x0TSPSoCState tsp;
     
         bool mmio_exec;
     };
     
and a TYPE_ASPEED_MACHINE machine state is described by :

     struct AspeedMachineState {
         /* Private */
         MachineState parent_obj;
         /* Public */
     
         AspeedSoCState *soc;
         MemoryRegion boot_rom;
         bool mmio_exec;
         uint32_t uart_chosen;
         char *fmc_model;
         char *spi_model;
         uint32_t hw_strap1;
     };

These are not compatible.

You will need to redefine the attributes (state and class) you need
in the "ast2700fc" machine.


Thanks,

C.

RE: [PATCH v2 1/5] hw/arm/aspeed_ast2700-fc: Fix null pointer dereference in ca35 init

[Steven Lee]

Hi Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Wednesday, May 14, 2025 9:28 PM
> To: Steven Lee <steven_lee@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.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>; longzl2@lenovo.com; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v2 1/5] hw/arm/aspeed_ast2700-fc: Fix null pointer
> dereference in ca35 init
> 
> On 5/14/25 11:03, Steven Lee wrote:
> > Clang's sanitizer reports a runtime error when booting with '-net nic
> > -net user', due to a null pointer being passed to
> > memory_region_find(), which subsequently triggers a crash in
> > flatview_lookup().
> >
> > Root cause:
> > - Missing NIC configuration in the CA35 initialization.
> >
> > Fix:
> > - Reduce ca35 ram size from 2GiB to 1GiB to align with ast2700a1-evb,
> >    where the ram-container is defined as 1GiB in its class.
> > - Add nic configuration in ast2700fc's ca35 init function.
> >
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >   hw/arm/aspeed_ast27x0-fc.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index 125a3ade40..ff64605663 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -48,7 +48,7 @@ struct Ast2700FCState {
> >       bool mmio_exec;
> >   };
> >
> > -#define AST2700FC_BMC_RAM_SIZE (2 * GiB)
> > +#define AST2700FC_BMC_RAM_SIZE (1 * GiB)
> >   #define AST2700FC_CM4_DRAM_SIZE (32 * MiB)
> >
> >   #define AST2700FC_HW_STRAP1 0x000000C0 @@ -59,6 +59,7 @@ struct
> > Ast2700FCState {
> >   static void ast2700fc_ca35_init(MachineState *machine)
> >   {
> >       Ast2700FCState *s = AST2700A1FC(machine);
> > +    AspeedMachineClass *amc =
> ASPEED_MACHINE_GET_CLASS(machine);
> >       AspeedSoCState *soc;
> >       AspeedSoCClass *sc;
> >
> > @@ -86,6 +87,14 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> >                                    AST2700FC_BMC_RAM_SIZE,
> &error_abort)) {
> >           return;
> >       }
> > +
> > +    for (int i = 0; i < sc->macs_num; i++) {
> > +        if ((amc->macs_mask & (1 << i)) &&
> > +            !qemu_configure_nic_device(DEVICE(&soc->ftgmac100[i]),
> > +                                       true, NULL)) {
> > +            break;
> > +        }
> > +    }
> >       if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
> >                                    AST2700FC_HW_STRAP1,
> &error_abort)) {
> >           return;
> > @@ -171,6 +180,7 @@ static void ast2700fc_init(MachineState *machine)
> >   static void ast2700fc_class_init(ObjectClass *oc, const void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> > +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> >
> >       mc->alias = "ast2700fc";
> >       mc->desc = "ast2700 full core support"; @@ -178,12 +188,13 @@
> > static void ast2700fc_class_init(ObjectClass *oc, const void *data)
> >       mc->no_floppy = 1;
> >       mc->no_cdrom = 1;
> >       mc->min_cpus = mc->max_cpus = mc->default_cpus = 6;
> > +    amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON |
> > + ASPEED_MAC2_ON;
> >   }
> >
> >   static const TypeInfo ast2700fc_types[] = {
> >       {
> >           .name           = MACHINE_TYPE_NAME("ast2700fc"),
> > -        .parent         = TYPE_MACHINE,
> > +        .parent         = TYPE_ASPEED_MACHINE,
> >           .class_init     = ast2700fc_class_init,
> >           .instance_size  = sizeof(Ast2700FCState),
> >       },
> 
> The "ast2700fc" machine cannot inherit from TYPE_ASPEED_MACHINE.
> These are two different type of machines.
> 
> An "ast2700fc" machine state is described by :
> 
>      struct Ast2700FCState {
>          MachineState parent_obj;
> 
>          MemoryRegion ca35_memory;
>          MemoryRegion ca35_dram;
>          MemoryRegion ssp_memory;
>          MemoryRegion tsp_memory;
> 
>          Clock *ssp_sysclk;
>          Clock *tsp_sysclk;
> 
>          Aspeed27x0SoCState ca35;
>          Aspeed27x0SSPSoCState ssp;
>          Aspeed27x0TSPSoCState tsp;
> 
>          bool mmio_exec;
>      };
> 
> and a TYPE_ASPEED_MACHINE machine state is described by :
> 
>      struct AspeedMachineState {
>          /* Private */
>          MachineState parent_obj;
>          /* Public */
> 
>          AspeedSoCState *soc;
>          MemoryRegion boot_rom;
>          bool mmio_exec;
>          uint32_t uart_chosen;
>          char *fmc_model;
>          char *spi_model;
>          uint32_t hw_strap1;
>      };
> 
> These are not compatible.
> 
> You will need to redefine the attributes (state and class) you need in the
> "ast2700fc" machine.
> 


Thanks for the review.
Will fix this in the next patch series.

Regards,
Steven

[PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented region overlap with vbootrom

[Steven Lee via]

The unimplemented memory region overlaps with the VBootROM address
range, causing incorrect memory layout and potential behavior issues.

This patch adjusts the size and start address of the unimplemented
region to avoid collision. The IO memory region (ASPEED_DEV_IOMEM) is
now moved to 0x20000 to reserve space for VBootROM at 0x0.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 1974a25766..bb61c30cf4 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -23,14 +23,14 @@
 #include "qobject/qlist.h"
 #include "qemu/log.h"
 
-#define AST2700_SOC_IO_SIZE          0x01000000
+#define AST2700_SOC_IO_SIZE          0x00FE0000
 #define AST2700_SOC_IOMEM_SIZE       0x01000000
 #define AST2700_SOC_DPMCU_SIZE       0x00040000
 #define AST2700_SOC_LTPI_SIZE        0x01000000
 
 static const hwaddr aspeed_soc_ast2700_memmap[] = {
-    [ASPEED_DEV_IOMEM]     =  0x00000000,
     [ASPEED_DEV_VBOOTROM]  =  0x00000000,
+    [ASPEED_DEV_IOMEM]     =  0x00020000,
     [ASPEED_DEV_SRAM]      =  0x10000000,
     [ASPEED_DEV_DPMCU]     =  0x11000000,
     [ASPEED_DEV_IOMEM0]    =  0x12000000,
-- 
2.43.0

Re: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented region overlap with vbootrom

[Cédric Le Goater]

On 5/14/25 11:03, Steven Lee wrote:
> The unimplemented memory region overlaps with the VBootROM address
> range, causing incorrect memory layout and potential behavior issues.
> 
> This patch adjusts the size and start address of the unimplemented
> region to avoid collision. The IO memory region (ASPEED_DEV_IOMEM) is
> now moved to 0x20000 to reserve space for VBootROM at 0x0.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>

You didn't reply to the question I asked on the v1 series.
How useful is this ASPEED_DEV_IOMEM region ?

Thanks,

C.



> ---
>   hw/arm/aspeed_ast27x0.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 1974a25766..bb61c30cf4 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -23,14 +23,14 @@
>   #include "qobject/qlist.h"
>   #include "qemu/log.h"
>   
> -#define AST2700_SOC_IO_SIZE          0x01000000
> +#define AST2700_SOC_IO_SIZE          0x00FE0000
>   #define AST2700_SOC_IOMEM_SIZE       0x01000000
>   #define AST2700_SOC_DPMCU_SIZE       0x00040000
>   #define AST2700_SOC_LTPI_SIZE        0x01000000
>   
>   static const hwaddr aspeed_soc_ast2700_memmap[] = {
> -    [ASPEED_DEV_IOMEM]     =  0x00000000,
>       [ASPEED_DEV_VBOOTROM]  =  0x00000000,
> +    [ASPEED_DEV_IOMEM]     =  0x00020000,
>       [ASPEED_DEV_SRAM]      =  0x10000000,
>       [ASPEED_DEV_DPMCU]     =  0x11000000,
>       [ASPEED_DEV_IOMEM0]    =  0x12000000,

RE: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented region overlap with vbootrom

[Steven Lee]

Hi Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Wednesday, May 14, 2025 9:28 PM
> To: Steven Lee <steven_lee@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.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>; longzl2@lenovo.com; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented
> region overlap with vbootrom
> 
> On 5/14/25 11:03, Steven Lee wrote:
> > The unimplemented memory region overlaps with the VBootROM address
> > range, causing incorrect memory layout and potential behavior issues.
> >
> > This patch adjusts the size and start address of the unimplemented
> > region to avoid collision. The IO memory region (ASPEED_DEV_IOMEM) is
> > now moved to 0x20000 to reserve space for VBootROM at 0x0.
> >
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> 
> You didn't reply to the question I asked on the v1 series.
> How useful is this ASPEED_DEV_IOMEM region ?
> 

Sorry for not replying to your question about this patch in the v1 series earlier.
Somehow our mail server mistakenly flagged that particular message as spam, so I missed it initially.

Regarding the ASPEED_DEV_IOMEM region, I checked the datasheet, and you're right, no devices are mapping registers in this window.
Since it's unused, there's no need to map it in an unimplemented region. I will drop this patch in v3 patch series.

Regards,
Steven

> 
> > ---
> >   hw/arm/aspeed_ast27x0.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > 1974a25766..bb61c30cf4 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -23,14 +23,14 @@
> >   #include "qobject/qlist.h"
> >   #include "qemu/log.h"
> >
> > -#define AST2700_SOC_IO_SIZE          0x01000000
> > +#define AST2700_SOC_IO_SIZE          0x00FE0000
> >   #define AST2700_SOC_IOMEM_SIZE       0x01000000
> >   #define AST2700_SOC_DPMCU_SIZE       0x00040000
> >   #define AST2700_SOC_LTPI_SIZE        0x01000000
> >
> >   static const hwaddr aspeed_soc_ast2700_memmap[] = {
> > -    [ASPEED_DEV_IOMEM]     =  0x00000000,
> >       [ASPEED_DEV_VBOOTROM]  =  0x00000000,
> > +    [ASPEED_DEV_IOMEM]     =  0x00020000,
> >       [ASPEED_DEV_SRAM]      =  0x10000000,
> >       [ASPEED_DEV_DPMCU]     =  0x11000000,
> >       [ASPEED_DEV_IOMEM0]    =  0x12000000,

RE: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented region overlap with vbootrom

[Steven Lee]

Hi Cédric,

> -----Original Message-----
> From: Steven Lee
> Sent: Thursday, May 15, 2025 1:06 PM
> To: Cédric Le Goater <clg@redhat.com>; Peter Maydell
> <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.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>; longzl2@lenovo.com; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: RE: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented
> region overlap with vbootrom
> 
> Hi Cédric,
> 
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Sent: Wednesday, May 14, 2025 9:28 PM
> > To: Steven Lee <steven_lee@aspeedtech.com>; Peter Maydell
> > <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> > <jamin_lin@aspeedtech.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>; longzl2@lenovo.com; Yunlin
> > Tang <yunlin.tang@aspeedtech.com>
> > Subject: Re: [PATCH v2 2/5] hw/arm/aspeed_ast27x0: Fix unimplemented
> > region overlap with vbootrom
> >
> > On 5/14/25 11:03, Steven Lee wrote:
> > > The unimplemented memory region overlaps with the VBootROM address
> > > range, causing incorrect memory layout and potential behavior issues.
> > >
> > > This patch adjusts the size and start address of the unimplemented
> > > region to avoid collision. The IO memory region (ASPEED_DEV_IOMEM)
> > > is now moved to 0x20000 to reserve space for VBootROM at 0x0.
> > >
> > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> >
> > You didn't reply to the question I asked on the v1 series.
> > How useful is this ASPEED_DEV_IOMEM region ?
> >
> 
> Sorry for not replying to your question about this patch in the v1 series earlier.
> Somehow our mail server mistakenly flagged that particular message as spam,
> so I missed it initially.
> 
> Regarding the ASPEED_DEV_IOMEM region, I checked the datasheet, and
> you're right, no devices are mapping registers in this window.
> Since it's unused, there's no need to map it in an unimplemented region. I will
> drop this patch in v3 patch series.
> 

I’d like to revise my previous statement regarding the removal of the ASPEED_DEV_IOMEM region.
After further testing, I discovered that either the OP-TEE firmware or u-boot in our AST27xx image performs accesses at address 0x400000. If we remove the unimplemented region mapping for ASPEED_DEV_IOMEM, the firmware hangs during early boot.
Although I haven’t yet had time to fully investigate the OP-TEE firmware behavior, I believe it’s safer to keep the unimplemented region for now. This will help prevent similar hangs if other firmware components access that memory range unexpectedly.
So instead of dropping the patch in the v3 series, I plan to keep the ASPEED_DEV_IOMEM mapping as a safeguard.

Please let me know if this approach looks acceptable to you, or if you have any concerns or suggestions.

Best regards,
Steven

[PATCH v2 3/5] hw/arm/aspeed_ast27x0-fc: Map ca35 memory into system memory

[Steven Lee via]

Attach CA35 memory to system_memory to ensure a valid FlatView.
Without this, dma_memory_write() used by ftgmac fail silently,
causing dhcp to break on ast2700fc, as flatview_write() returns
an error when system_memory is empty.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0-fc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
index ff64605663..ccba5fc8a1 100644
--- a/hw/arm/aspeed_ast27x0-fc.c
+++ b/hw/arm/aspeed_ast27x0-fc.c
@@ -69,6 +69,7 @@ static void ast2700fc_ca35_init(MachineState *machine)
 
     memory_region_init(&s->ca35_memory, OBJECT(&s->ca35), "ca35-memory",
                        UINT64_MAX);
+    memory_region_add_subregion(get_system_memory(), 0, &s->ca35_memory);
 
     if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35), "ca35-dram",
                                 AST2700FC_BMC_RAM_SIZE, &error_abort)) {
-- 
2.43.0

Re: [PATCH v2 3/5] hw/arm/aspeed_ast27x0-fc: Map ca35 memory into system memory

[Cédric Le Goater]

Hello Steven,

On 5/14/25 11:03, Steven Lee wrote:
> Attach CA35 memory to system_memory to ensure a valid FlatView.
> Without this, dma_memory_write() used by ftgmac fail silently,
> causing dhcp to break on ast2700fc, as flatview_write() returns
> an error when system_memory is empty.

The change below fixes the network DMA transactions indeed but I think
this case can be addressed differently.

The transactions on address_space_memory in the ftgmac100 device model
should be replaced by transactions on a local address space which would
be initialized from a memory region passed to the model with a property.
This is very similar to what we do in the Aspeed SMC model. Since it is
more work, it can be addressed separately and later.

However, let's keep the change below for all other places which are
difficult to address, like rom_check_and_register_reset(). The commit
should be rephrased.

Thanks,

C.



> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>   hw/arm/aspeed_ast27x0-fc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> index ff64605663..ccba5fc8a1 100644
> --- a/hw/arm/aspeed_ast27x0-fc.c
> +++ b/hw/arm/aspeed_ast27x0-fc.c
> @@ -69,6 +69,7 @@ static void ast2700fc_ca35_init(MachineState *machine)
>   
>       memory_region_init(&s->ca35_memory, OBJECT(&s->ca35), "ca35-memory",
>                          UINT64_MAX);
> +    memory_region_add_subregion(get_system_memory(), 0, &s->ca35_memory);
>   
>       if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35), "ca35-dram",
>                                   AST2700FC_BMC_RAM_SIZE, &error_abort)) {

RE: [PATCH v2 3/5] hw/arm/aspeed_ast27x0-fc: Map ca35 memory into system memory

[Steven Lee]

Hi Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Wednesday, May 14, 2025 11:32 PM
> To: Steven Lee <steven_lee@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.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>; longzl2@lenovo.com; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v2 3/5] hw/arm/aspeed_ast27x0-fc: Map ca35 memory
> into system memory
> 
> Hello Steven,
> 
> On 5/14/25 11:03, Steven Lee wrote:
> > Attach CA35 memory to system_memory to ensure a valid FlatView.
> > Without this, dma_memory_write() used by ftgmac fail silently, causing
> > dhcp to break on ast2700fc, as flatview_write() returns an error when
> > system_memory is empty.
> 
> The change below fixes the network DMA transactions indeed but I think this
> case can be addressed differently.
> 
> The transactions on address_space_memory in the ftgmac100 device model
> should be replaced by transactions on a local address space which would be
> initialized from a memory region passed to the model with a property.
> This is very similar to what we do in the Aspeed SMC model. Since it is more
> work, it can be addressed separately and later.
> 
> However, let's keep the change below for all other places which are difficult to
> address, like rom_check_and_register_reset(). The commit should be
> rephrased.
> 


Thanks for the suggestion, I will rewrite the commit message

Regards,
Steven

> 
> 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >   hw/arm/aspeed_ast27x0-fc.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index ff64605663..ccba5fc8a1 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -69,6 +69,7 @@ static void ast2700fc_ca35_init(MachineState
> > *machine)
> >
> >       memory_region_init(&s->ca35_memory, OBJECT(&s->ca35),
> "ca35-memory",
> >                          UINT64_MAX);
> > +    memory_region_add_subregion(get_system_memory(), 0,
> > + &s->ca35_memory);
> >
> >       if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35),
> "ca35-dram",
> >                                   AST2700FC_BMC_RAM_SIZE,
> > &error_abort)) {

[PATCH v2 4/5] hw/arm/fby35: Map BMC memory into system memory

[Steven Lee via]

Add the BMC memory region as a subregion of system_memory so that
modules relying on system memory can operate correctly.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 hw/arm/fby35.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index e123fa69e1..c14fc2efe9 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -77,6 +77,7 @@ static void fby35_bmc_init(Fby35State *s)
 
     memory_region_init(&s->bmc_memory, OBJECT(&s->bmc), "bmc-memory",
                        UINT64_MAX);
+    memory_region_add_subregion(get_system_memory(), 0, &s->bmc_memory);
     memory_region_init_ram(&s->bmc_dram, OBJECT(&s->bmc), "bmc-dram",
                            FBY35_BMC_RAM_SIZE, &error_abort);
 
-- 
2.43.0

Re: [PATCH v2 4/5] hw/arm/fby35: Map BMC memory into system memory

[Cédric Le Goater]

On 5/14/25 11:03, Steven Lee wrote:
> Add the BMC memory region as a subregion of system_memory so that
> modules relying on system memory can operate correctly.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>


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

Thanks,

C.


> ---
>   hw/arm/fby35.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
> index e123fa69e1..c14fc2efe9 100644
> --- a/hw/arm/fby35.c
> +++ b/hw/arm/fby35.c
> @@ -77,6 +77,7 @@ static void fby35_bmc_init(Fby35State *s)
>   
>       memory_region_init(&s->bmc_memory, OBJECT(&s->bmc), "bmc-memory",
>                          UINT64_MAX);
> +    memory_region_add_subregion(get_system_memory(), 0, &s->bmc_memory);
>       memory_region_init_ram(&s->bmc_dram, OBJECT(&s->bmc), "bmc-dram",
>                              FBY35_BMC_RAM_SIZE, &error_abort);
>   

[PATCH v2 5/5] docs: Remove ast2700fc from Aspeed family boards

[Steven Lee via]

The ast2700fc machine is now covered in the dedicated ast2700-evb
section. Listing it in the general Aspeed board family list is
redundant.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 docs/system/arm/aspeed.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 58a8020eec..43d27d83cb 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -1,4 +1,4 @@
-Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``ast2700fc``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)
+Aspeed family boards (``ast2500-evb``, ``ast2600-evb``, ``ast2700-evb``, ``bletchley-bmc``, ``fuji-bmc``, ``fby35-bmc``, ``fp5280g2-bmc``, ``g220a-bmc``, ``palmetto-bmc``, ``qcom-dc-scm-v1-bmc``, ``qcom-firework-bmc``, ``quanta-q71l-bmc``, ``rainier-bmc``, ``romulus-bmc``, ``sonorapass-bmc``, ``supermicrox11-bmc``, ``supermicrox11spi-bmc``, ``tiogapass-bmc``, ``witherspoon-bmc``, ``yosemitev2-bmc``)
 =================================================================================================================================================================================================================================================================================================================================================================================================================================
 
 The QEMU Aspeed machines model BMCs of various OpenPOWER systems and
-- 
2.43.0