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

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

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

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) */
-- 
1.7.5.4

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 <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) */
> 


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

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

[James Hogan]

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