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

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

Posted by Steven Lee via 6 months ago

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

Posted by Cédric Le Goater 6 months ago

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

Posted by Steven Lee 6 months ago

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

Posted by Steven Lee 6 months ago

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