From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJSiS-00068F-4O for qemu-devel@nongnu.org; Sun, 26 Jul 2015 16:40:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJSiN-00005g-2a for qemu-devel@nongnu.org; Sun, 26 Jul 2015 16:40:12 -0400 Received: from smtp1-g21.free.fr ([2a01:e0c:1:1599::10]:21746) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJSiM-0008PO-Pn for qemu-devel@nongnu.org; Sun, 26 Jul 2015 16:40:07 -0400 Message-ID: <55B544AE.7000709@reactos.org> Date: Sun, 26 Jul 2015 22:35:58 +0200 From: =?ISO-8859-15?Q?Herv=E9_Poussineau?= MIME-Version: 1.0 References: <1437763343-7980-1-git-send-email-hpoussin@reactos.org> <1437763343-7980-3-git-send-email-hpoussin@reactos.org> <20150726201105.GA13016@aurel32.net> In-Reply-To: <20150726201105.GA13016@aurel32.net> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for 2.4 2/3] net/dp8393x: specify memory operations for PROM PROM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: Leon Alrae , qemu-devel@nongnu.org Hi, Le 26/07/2015 22:11, Aurelien Jarno a =E9crit : > On 2015-07-24 20:42, Herv=E9 Poussineau wrote: >> This fixes a guest-triggerable QEMU crash when guest tries to write to= PROM. >> >> Signed-off-by: Herv=E9 Poussineau >> --- >> 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 =3D { >> .endianness =3D DEVICE_NATIVE_ENDIAN, >> }; >> >> +static bool dp8393x_rom_accepts(void *opaque, hwaddr addr, unsigned i= nt size, >> + bool is_write) >> +{ >> + return !is_write; >> +} >> + >> +static const MemoryRegionOps dp8393x_rom_ops =3D { >> + .valid.accepts =3D dp8393x_rom_accepts, >> +}; >> + >> static void dp8393x_watchdog(void *opaque) >> { >> dp8393xState *s =3D opaque; >> @@ -840,7 +850,7 @@ static void dp8393x_realize(DeviceState *dev, Erro= r **errp) >> s->watchdog =3D timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdo= g, s); >> s->regs[SONIC_SR] =3D 0x0004; /* only revision recognized by Lin= ux */ >> >> - 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, N= ULL); >> prom =3D memory_region_get_ram_ptr(&s->prom); >> checksum =3D 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 cal= l > 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=3Dmr@entry=3D0x55555= adb0d50, addr=3Daddr@entry=3D0, size=3Dsize@entry=3D1, is_write=3Dis_writ= e@entry=3Dtrue) at memory.c:1075 1075 if (!mr->ops->valid.unaligned && (addr & (size - 1))) { (gdb) bt #0 0x000055555563a758 in memory_region_access_valid (mr=3Dmr@entry=3D0x5= 5555adb0d50, addr=3Daddr@entry=3D0, size=3Dsize@entry=3D1, is_write=3Dis_= write@entry=3Dtrue) at memory.c:1075 #1 0x000055555563a968 in memory_region_dispatch_write (mr=3D0x55555adb0d= 50, addr=3D0, data=3D82, size=3D1, attrs=3D...) at memory.c:1155 #2 0x00007fffe6516f35 in code_gen_buffer () #3 0x000055555560e4f3 in cpu_tb_exec (tb_ptr=3D0x7fffe6516ec0 "A\213n\374\205\355\017\205\220", cpu=3D0x55555703f1c0) a= t cpu-exec.c:200 #4 cpu_mips_exec (cpu=3Dcpu@entry=3D0x55555703f1c0) at cpu-exec.c:518 #5 0x000055555562aec6 in tcg_cpu_exec (cpu=3D0x55555703f1c0) at cpus.c:1= 402 #6 tcg_exec_all () at cpus.c:1434 #7 qemu_tcg_cpu_thread_fn (arg=3D) at cpus.c:1068 #8 0x00007ffff1dbd0a4 in start_thread (arg=3D0x7fffdf8f8700) at pthread_= create.c:309 #9 0x00007ffff1af204d in clone () at ../sysdeps/unix/sysv/linux/x86_64/c= lone.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: =3D?UTF-8?q?Herv=3DC3=3DA9=3D20Poussineau?=3D Date: Sun, 26 Jul 2015 22:32:55 +0200 Subject: [PATCH] net/dp8393x: do not use memory_region_init_rom_device wi= th NULL memory operations MIME-Version: 1.0 Content-Type: text/plain; charset=3DUTF-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 PR= OM. Signed-off-by: Herv=E9 Poussineau --- 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 =3D DP8393X(dev); int i, checksum; uint8_t *prom; + Error *local_err =3D 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 =3D timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, = s); s->regs[SONIC_SR] =3D 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 =3D memory_region_get_ram_ptr(&s->prom); checksum =3D 0; for (i =3D 0; i < 6; i++) { --=20 2.1.4 Herv=E9