| Message ID | 20171019100410.26239-2-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: add a witherspoon-bmc machine | expand |
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
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 >
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;
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(+)