Re: [Qemu-devel] [PATCH for 2.4 2/3] net/dp8393x: specify memory operations for PROM PROM

["Hervé Poussineau" <hpoussin@reactos.org>]

Hi,

Le 26/07/2015 22:11, Aurelien Jarno a écrit :
> On 2015-07-24 20:42, Hervé Poussineau wrote:
>> This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>   hw/net/dp8393x.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 8fafdb0..55168b5 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -601,6 +601,16 @@ static const MemoryRegionOps dp8393x_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>
>> +static bool dp8393x_rom_accepts(void *opaque, hwaddr addr, unsigned int size,
>> +                                bool is_write)
>> +{
>> +    return !is_write;
>> +}
>> +
>> +static const MemoryRegionOps dp8393x_rom_ops = {
>> +    .valid.accepts = dp8393x_rom_accepts,
>> +};
>> +
>>   static void dp8393x_watchdog(void *opaque)
>>   {
>>       dp8393xState *s = opaque;
>> @@ -840,7 +850,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>>       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
>>       s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>
>> -    memory_region_init_rom_device(&s->prom, OBJECT(dev), NULL, NULL,
>> +    memory_region_init_rom_device(&s->prom, OBJECT(dev), &dp8393x_rom_ops, NULL,
>>                                     "dp8393x-prom", SONIC_PROM_SIZE, NULL);
>>       prom = memory_region_get_ram_ptr(&s->prom);
>>       checksum = 0;
>
> How does it crashes in that case? I would have guess that write access
> to ROM are ignored by default. Looking at other code, it seems they call
> memory_region_set_readonly() instead of providing an accepts function.
> Maybe readonly should be the default for a rom device?

The stack trace is:
0x000055555563a758 in memory_region_access_valid (mr=mr@entry=0x55555adb0d50, addr=addr@entry=0, size=size@entry=1, is_write=is_write@entry=true) at memory.c:1075
1075	    if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
(gdb) bt
#0  0x000055555563a758 in memory_region_access_valid (mr=mr@entry=0x55555adb0d50, addr=addr@entry=0, size=size@entry=1, is_write=is_write@entry=true) at memory.c:1075
#1  0x000055555563a968 in memory_region_dispatch_write (mr=0x55555adb0d50, addr=0, data=82, size=1, attrs=...) at memory.c:1155
#2  0x00007fffe6516f35 in code_gen_buffer ()
#3  0x000055555560e4f3 in cpu_tb_exec (tb_ptr=0x7fffe6516ec0 <code_gen_buffer+8625856> "A\213n\374\205\355\017\205\220", cpu=0x55555703f1c0) at cpu-exec.c:200
#4  cpu_mips_exec (cpu=cpu@entry=0x55555703f1c0) at cpu-exec.c:518
#5  0x000055555562aec6 in tcg_cpu_exec (cpu=0x55555703f1c0) at cpus.c:1402
#6  tcg_exec_all () at cpus.c:1434
#7  qemu_tcg_cpu_thread_fn (arg=<optimized out>) at cpus.c:1068
#8  0x00007ffff1dbd0a4 in start_thread (arg=0x7fffdf8f8700) at pthread_create.c:309
#9  0x00007ffff1af204d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

With mr being the dp8393x prom.


I tested with memory_region_set_readonly() and a NULL operations, and the stack trace is the same.
Only pflash devices use memory_region_init_rom_device. Other devices use memory_region_init_ram + memory_region_set_readonly, which work.
Do you prefer the attached patch?

 From bfe27278e08d1a1239ae674036114a21cb152582 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= <hpoussin@reactos.org>
Date: Sun, 26 Jul 2015 22:32:55 +0200
Subject: [PATCH] net/dp8393x: do not use memory_region_init_rom_device with
  NULL memory operations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace memory_region_init_rom_device() with memory_region_init_ram() and
memory_region_set_readonly().
This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
  hw/net/dp8393x.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 8fafdb0..8ba1d0b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -828,6 +828,7 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
      dp8393xState *s = DP8393X(dev);
      int i, checksum;
      uint8_t *prom;
+    Error *local_err = NULL;

      address_space_init(&s->as, s->dma_mr, "dp8393x");
      memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
@@ -840,8 +841,13 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
      s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */

-    memory_region_init_rom_device(&s->prom, OBJECT(dev), NULL, NULL,
-                                  "dp8393x-prom", SONIC_PROM_SIZE, NULL);
+    memory_region_init_ram(&s->prom, OBJECT(dev),
+                           "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    memory_region_set_readonly(&s->prom, true);
      prom = memory_region_get_ram_ptr(&s->prom);
      checksum = 0;
      for (i = 0; i < 6; i++) {
-- 
2.1.4

Hervé