Re: [Qemu-devel] [PATCH] mips/malta: prevent writes to reset flash mapping faulting

[Andreas Färber]

Am 23.08.2013 09:59, schrieb Leon Alrae:
> From: James Hogan <address@hidden>
> 
> 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 <address@hidden>
> Cc: Paul Burton <address@hidden>
> Cc: Leon Alrae <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Signed-off-by: Leon Alrae <address@hidden>
> ---
>  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) */
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg