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

[Cédric Le Goater]

On 10/19/2017 01:06 PM, Peter Maydell wrote:
> On 19 October 2017 at 11:04, 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 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 <address@hidden>
>> ---
>>  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
>