| Message ID | 1377244791-56856-1-git-send-email-leon.alrae@imgtec.com |
|---|---|
| State | New |
| Headers | show |
Am 23.08.2013 09:59, schrieb Leon Alrae: > From: James Hogan <james.hogan@imgtec.com> > > Commit a427338 (mips_malta: correct reading MIPS revision at 0x1fc00010) > altered the behaviour of the monitor flash mapping at the reset address > by making it read only. However this causes data bus error exceptions > when it is written to since it is effectively unassigned memory for > writes. This isn't how the real hardware behaves. That memory can be > written to (even with the MFWR jumper not fitted) and the new value read > back from, but it doesn't get written back to the monitor flash so is > volatile. > > This is fixed by converting the bios copy from read only ram to a bios > device with a nop write callback. That sounds like a contradiction: The nop write will not have reads return the new value, will it? Why not just remove the _set_readonly and have it reloaded on reset for volatility? Anyway, having a MemoryRegionOps with just a .write looks dangerous, but I guess you've tested read to work. We had been seeing assertions elsewhere when either was missing. Regards, Andreas > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Paul Burton <paul.burton@imgtec.com> > Cc: Leon Alrae <leon.alrae@imgtec.com> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Signed-off-by: Leon Alrae <leon.alrae@imgtec.com> > --- > hw/mips/mips_malta.c | 14 ++++++++++++-- > 1 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c > index f8d064c..9e721d3 100644 > --- a/hw/mips/mips_malta.c > +++ b/hw/mips/mips_malta.c > @@ -873,6 +873,16 @@ static void cpu_request_exit(void *opaque, int irq, int level) > } > } > > +static void monflash_copy_mem_write(void *opaque, hwaddr ram_addr, > + uint64_t val, unsigned size) > +{ > +} > + > +static const MemoryRegionOps monflash_copy_mem_ops = { > + .write = monflash_copy_mem_write, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > static > void mips_malta_init(QEMUMachineInitArgs *args) > { > @@ -1043,13 +1053,13 @@ void mips_malta_init(QEMUMachineInitArgs *args) > * handled by an overlapping region as the resulting ROM code subpage > * regions are not executable. > */ > - memory_region_init_ram(bios_copy, NULL, "bios.1fc", BIOS_SIZE); > + memory_region_init_rom_device(bios_copy, NULL, &monflash_copy_mem_ops, NULL, > + "bios.1fc", BIOS_SIZE); > if (!rom_copy(memory_region_get_ram_ptr(bios_copy), > FLASH_ADDRESS, BIOS_SIZE)) { > memcpy(memory_region_get_ram_ptr(bios_copy), > memory_region_get_ram_ptr(bios), BIOS_SIZE); > } > - memory_region_set_readonly(bios_copy, true); > memory_region_add_subregion(system_memory, RESET_ADDRESS, bios_copy); > > /* Board ID = 0x420 (Malta Board with CoreLV) */ >
Hi Andreas, On 23/08/13 12:08, Andreas Färber wrote: > Am 23.08.2013 09:59, schrieb Leon Alrae: >> From: James Hogan <james.hogan@imgtec.com> >> >> Commit a427338 (mips_malta: correct reading MIPS revision at 0x1fc00010) >> altered the behaviour of the monitor flash mapping at the reset address >> by making it read only. However this causes data bus error exceptions >> when it is written to since it is effectively unassigned memory for >> writes. This isn't how the real hardware behaves. That memory can be >> written to (even with the MFWR jumper not fitted) and the new value read >> back from, but it doesn't get written back to the monitor flash so is >> volatile. >> >> This is fixed by converting the bios copy from read only ram to a bios >> device with a nop write callback. > > That sounds like a contradiction: The nop write will not have reads > return the new value, will it? correct. > Why not just remove the _set_readonly and have it reloaded on reset for > volatility? That's what I tried first, but the bios copy is normal ram so it doesn't get reloaded on reset. I'll have a play to see if I can use rom_add_blob (although I seem to remember already trying that...). > Anyway, having a MemoryRegionOps with just a .write looks dangerous, but > I guess you've tested read to work. We had been seeing assertions > elsewhere when either was missing. Yeh reads seem to work fine (it also executes from it fine). Thanks for taking a look James
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c index f8d064c..9e721d3 100644 --- a/hw/mips/mips_malta.c +++ b/hw/mips/mips_malta.c @@ -873,6 +873,16 @@ static void cpu_request_exit(void *opaque, int irq, int level) } } +static void monflash_copy_mem_write(void *opaque, hwaddr ram_addr, + uint64_t val, unsigned size) +{ +} + +static const MemoryRegionOps monflash_copy_mem_ops = { + .write = monflash_copy_mem_write, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + static void mips_malta_init(QEMUMachineInitArgs *args) { @@ -1043,13 +1053,13 @@ void mips_malta_init(QEMUMachineInitArgs *args) * handled by an overlapping region as the resulting ROM code subpage * regions are not executable. */ - memory_region_init_ram(bios_copy, NULL, "bios.1fc", BIOS_SIZE); + memory_region_init_rom_device(bios_copy, NULL, &monflash_copy_mem_ops, NULL, + "bios.1fc", BIOS_SIZE); if (!rom_copy(memory_region_get_ram_ptr(bios_copy), FLASH_ADDRESS, BIOS_SIZE)) { memcpy(memory_region_get_ram_ptr(bios_copy), memory_region_get_ram_ptr(bios), BIOS_SIZE); } - memory_region_set_readonly(bios_copy, true); memory_region_add_subregion(system_memory, RESET_ADDRESS, bios_copy); /* Board ID = 0x420 (Malta Board with CoreLV) */