| Message ID | 20171019151249.13663-2-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: add a witherspoon-bmc machine | expand |
On 19 October 2017 at 16:12, 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 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 <clg@kaod.org> > --- > > 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
On 10/19/2017 05:44 PM, Peter Maydell wrote: > On 19 October 2017 at 16:12, 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 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 <clg@kaod.org> >> --- >> >> 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.
On 10/19/2017 12:44 PM, Peter Maydell wrote: > On 19 October 2017 at 16:12, 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 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 <clg@kaod.org> >> --- >> >> 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. What about the eeprom_buf from patch 6 "Add EEPROM I2C devices"? My understanding is a migrated board would resume with a zeroized eeprom, is this the expected behaviour? Regards, Phil.
On 10/20/2017 05:10 AM, Philippe Mathieu-Daudé wrote: > On 10/19/2017 12:44 PM, Peter Maydell wrote: >> On 19 October 2017 at 16:12, 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 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 <clg@kaod.org> >>> --- >>> >>> 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. > > What about the eeprom_buf from patch 6 "Add EEPROM I2C devices"? > > My understanding is a migrated board would resume with a zeroized > eeprom, is this the expected behaviour? That would be probably the case. yes. As we don't populate the eeprom with VPD at machine init time and that migration is clearly not tested, I think we will address this issue later. Thanks, C.
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);
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 <clg@kaod.org> --- Changes since v4 : - use a ROM memory region hw/arm/aspeed.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)