1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v4 0/8] aspeed: add a witherspoon-bmc machine
  4. View patch

[Qemu-devel] [PATCH v4 1/8] aspeed: introduce a dummy ROM device to catch invalid writes

Cédric Le Goater posted 8 patches 8 years, 3 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v4 1/8] aspeed: introduce a dummy ROM device to catch invalid writes
Posted by Cédric Le Goater 8 years, 3 months ago 
Some legacy firmwares access unimplemented addresses on the Aspeed SoC
(old U-Boot code using variables in the bss when it shouldn't do).
Let's add a dummy ROM device to catch the invalid writes and support
new board without using the 'ignore_memory_transaction_failures' flags.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ab895ad490af..e44733153819 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -166,6 +166,19 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
     }
 }
 
+static void boot_rom_rw_flash_write(void *opaque, hwaddr offset, uint64_t value,
+                              unsigned size)
+{
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+                  __func__, offset, value, size);
+}
+
+static const MemoryRegionOps boot_rom_rw_flash_ops = {
+    .write = boot_rom_rw_flash_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
 static void aspeed_board_init(MachineState *machine,
                               const AspeedBoardConfig *cfg)
 {
@@ -209,6 +222,7 @@ static void aspeed_board_init(MachineState *machine,
     if (drive0) {
         AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
         MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+        MemoryRegion *boot_rom_rw =  g_new(MemoryRegion, 1);
 
         /*
          * create a ROM region using the default mapping window size of
@@ -221,6 +235,16 @@ static void aspeed_board_init(MachineState *machine,
         memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
                                     boot_rom);
         write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
+
+        /*
+         * Create a fake ROM region to track invalid writes done by
+         * some legacy firmwares
+         */
+        memory_region_init_rom_device(boot_rom_rw, NULL, &boot_rom_rw_flash_ops,
+                                      NULL, "aspeed.boot_rom_rw", fl->size,
+                                      &error_abort);
+        memory_region_add_subregion_overlap(get_system_memory(), FIRMWARE_ADDR,
+                                            boot_rom_rw, 0);
     }
 
     aspeed_board_binfo.kernel_filename = machine->kernel_filename;
-- 
2.13.6
Re: [Qemu-devel] [PATCH v4 1/8] aspeed: introduce a dummy ROM device to catch invalid writes
Posted by Peter Maydell 8 years, 3 months ago 
On 19 October 2017 at 11:04, Cédric Le Goater <clg@kaod.org> wrote:
> Some legacy firmwares access unimplemented addresses on the Aspeed SoC
> (old U-Boot code using variables in the bss when it shouldn't do).
> Let's add a dummy ROM device to catch the invalid writes and support
> new board without using the 'ignore_memory_transaction_failures' flags.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ab895ad490af..e44733153819 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -166,6 +166,19 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>      }
>  }
>
> +static void boot_rom_rw_flash_write(void *opaque, hwaddr offset, uint64_t value,
> +                              unsigned size)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR,
> +                  "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> +                  __func__, offset, value, size);
> +}
> +
> +static const MemoryRegionOps boot_rom_rw_flash_ops = {
> +    .write = boot_rom_rw_flash_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  static void aspeed_board_init(MachineState *machine,
>                                const AspeedBoardConfig *cfg)
>  {
> @@ -209,6 +222,7 @@ static void aspeed_board_init(MachineState *machine,
>      if (drive0) {
>          AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
>          MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +        MemoryRegion *boot_rom_rw =  g_new(MemoryRegion, 1);
>
>          /*
>           * create a ROM region using the default mapping window size of
> @@ -221,6 +235,16 @@ static void aspeed_board_init(MachineState *machine,
>          memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>                                      boot_rom);
>          write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
> +
> +        /*
> +         * Create a fake ROM region to track invalid writes done by
> +         * some legacy firmwares
> +         */
> +        memory_region_init_rom_device(boot_rom_rw, NULL, &boot_rom_rw_flash_ops,
> +                                      NULL, "aspeed.boot_rom_rw", fl->size,
> +                                      &error_abort);
> +        memory_region_add_subregion_overlap(get_system_memory(), FIRMWARE_ADDR,
> +                                            boot_rom_rw, 0);
>      }

You're already (in some conditions) adding a memory region at this
address -- you can see it just above. I think you probably want
to be always creating this region in the same way, whether you
need to do a write_boot_rom() to it or not, rather than creating
two things at the same address.

I think that currently memory_region_init_rom() regions will
MEMTX_DECODE_ERROR on attempts to write to them -- that's really
a bug, which we might be able to fix, but for now you can do
this with the init_rom_device() and a comment about why we
can't just use memory_region_init_rom().

thanks
-- PMM
Re: [Qemu-devel] [PATCH v4 1/8] aspeed: introduce a dummy ROM device to catch invalid writes
Posted by Cédric Le Goater 8 years, 3 months ago 
On 10/19/2017 01:06 PM, Peter Maydell wrote:
> On 19 October 2017 at 11:04, Cédric Le Goater <clg@kaod.org> wrote:
>> Some legacy firmwares access unimplemented addresses on the Aspeed SoC
>> (old U-Boot code using variables in the bss when it shouldn't do).
>> Let's add a dummy ROM device to catch the invalid writes and support
>> new board without using the 'ignore_memory_transaction_failures' flags.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/arm/aspeed.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index ab895ad490af..e44733153819 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -166,6 +166,19 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>      }
>>  }
>>
>> +static void boot_rom_rw_flash_write(void *opaque, hwaddr offset, uint64_t value,
>> +                              unsigned size)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR,
>> +                  "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
>> +                  __func__, offset, value, size);
>> +}
>> +
>> +static const MemoryRegionOps boot_rom_rw_flash_ops = {
>> +    .write = boot_rom_rw_flash_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>>  static void aspeed_board_init(MachineState *machine,
>>                                const AspeedBoardConfig *cfg)
>>  {
>> @@ -209,6 +222,7 @@ static void aspeed_board_init(MachineState *machine,
>>      if (drive0) {
>>          AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
>>          MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
>> +        MemoryRegion *boot_rom_rw =  g_new(MemoryRegion, 1);
>>
>>          /*
>>           * create a ROM region using the default mapping window size of
>> @@ -221,6 +235,16 @@ static void aspeed_board_init(MachineState *machine,
>>          memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>>                                      boot_rom);
>>          write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>> +
>> +        /*
>> +         * Create a fake ROM region to track invalid writes done by
>> +         * some legacy firmwares
>> +         */
>> +        memory_region_init_rom_device(boot_rom_rw, NULL, &boot_rom_rw_flash_ops,
>> +                                      NULL, "aspeed.boot_rom_rw", fl->size,
>> +                                      &error_abort);
>> +        memory_region_add_subregion_overlap(get_system_memory(), FIRMWARE_ADDR,
>> +                                            boot_rom_rw, 0);
>>      }
> 
> You're already (in some conditions) adding a memory region at this
> address -- you can see it just above. I think you probably want
> to be always creating this region in the same way, whether you
> need to do a write_boot_rom() to it or not, rather than creating
> two things at the same address.

ok. I didn't think that was possible. Let me try that.

Thanks,

C. 

> 
> I think that currently memory_region_init_rom() regions will
> MEMTX_DECODE_ERROR on attempts to write to them -- that's really
> a bug, which we might be able to fix, but for now you can do
> this with the init_rom_device() and a comment about why we
> can't just use memory_region_init_rom().
> 
> thanks
> -- PMM
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.