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

[Cédric Le Goater]

On 10/19/2017 05:44 PM, Peter Maydell wrote:
> 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.

ah. Indeed :/ 

I tend to forget about migration for these emulated machines 
but people are starting to use these Aspeed guests on large 
Intel hosts as if they were real BMCs. And so being able to 
change host is a valid requirement. I will add a comment.


>>  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 twickende 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...

Do you mean that we should create the boot ROM device even if there 
is no file backend ? I think so yes. Let's do a v6 :)

Thanks,

C.