Re: [Qemu-devel] [PATCH v5 1/8] aspeed: use a ROM memory region to catch invalid writes

[Peter Maydell]

On 19 October 2017 at 16:12, Cédric Le Goater <address@hidden> 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 use a ROM memory region to catch the invalid writes and support
> new boards without using the 'ignore_memory_transaction_failures'
> flag.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>
>  Changes since v4 :
>
>  - use a ROM memory region

Probably worth mentioning in the commit message that this
is a migration compatibility break for these boards.

>  hw/arm/aspeed.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ab895ad490af..d88a8b5120b6 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -166,6 +166,23 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, 
> const char *flashtype,
>      }
>  }
>
> +/*
> + * This is to track invalid writes done in the ROM by some legacy
> + * firmwares
> + */
> +static void boot_rom_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_ops = {
> +    .write = boot_rom_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  static void aspeed_board_init(MachineState *machine,
>                                const AspeedBoardConfig *cfg)
>  {
> @@ -216,8 +233,9 @@ static void aspeed_board_init(MachineState *machine,
>           * SoC and 128MB for the AST2500 SoC, which is twice as big as
>           * needed by the flash modules of the Aspeed machines.
>           */
> -        memory_region_init_rom_nomigrate(boot_rom, OBJECT(bmc), 
> "aspeed.boot_rom",
> -                               fl->size, &error_abort);
> +        memory_region_init_rom_device(boot_rom, OBJECT(bmc), &boot_rom_ops,
> +                                      NULL, "aspeed.boot_rom", fl->size,
> +                                      &error_abort);
>          memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>                                      boot_rom);
>          write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
> --

Don't you want to create the ROM device even if there happens
not to be any contents specified by the user? The thing is still
there in the SoC even if it's empty...

thanks
-- PMM