net/dp8393x: misc fixes

PATCH In review Hervé Poussineau 3 participants 10 messages Jul 24, 2015 – Jul 27, 2015

lore

Share & tools

lore

Hervé Poussineau

author

2015-07-24 18:46 UTC #0 lore raw

cover net/dp8393x: misc fixes

Hi,

Network card dp8393x emulation has been greatly improved during the 2.4 cycle. However, I discovered a few bugs in it which should IMO be fixed before 2.4.

Hervé

Hervé Poussineau (3):
net/dp8393x: disable user creation net/dp8393x: do not crash when trying to write to PROM net/dp8393x: remove check of runt packets

 hw/net/dp8393x.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Signature · 1 line

2.1.4

Hervé Poussineau

author

2015-07-24 18:46 UTC #1 lore raw

PATCH 1/3 net/dp8393x: disable user creation

Netcard needs an address space to write data to, which can't be specified on command line. This fixes a crash when user starts QEMU with "-device dp8393x"

Signed-off-by Hervé Poussineau <[email protected]>

---
 hw/net/dp8393x.c | 2 ++
 1 file changed, 2 insertions(+)

hw/net/dp8393x.c +2 -0

index cd889bc..8fafdb0 100644
@@ -881,6 +881,8 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
     dc->reset = dp8393x_reset;
     dc->vmsd = &vmstate_dp8393x;
     dc->props = dp8393x_properties;
+    /* Reason: dma_mr property can't be set */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo dp8393x_info = {

Signature · 1 line

2.1.4

Aurelien Jarno

Reviewed-by on v1 1/3

2015-07-26 20:11 UTC #2 lore raw

Re: [Qemu-devel] [PATCH for 2.4 1/3] net/dp8393x: disable user creation

Quoted 13 lines · Hervé Poussineau

Netcard needs an address space to write data to, which can't be specified on command line. This fixes a crash when user starts QEMU with "-device dp8393x"
Signed-off-by Hervé Poussineau <[email protected]>
---
 hw/net/dp8393x.c | 2 ++
 1 file changed, 2 insertions(+)
hw/net/dp8393x.c +0 -0
index cd889bc..8fafdb0 100644

hw/net/dp8393x.c static void dp8393x_class_init(ObjectClass *klass, void *data) · line 888

Quoted 8 lines

hw/net/dp8393x.c +2 -0

@@ -881,6 +881,8 @@ static void dp8393x_class_init(ObjectClass *klass, void *data)
     dc->reset = dp8393x_reset;
     dc->vmsd = &vmstate_dp8393x;
     dc->props = dp8393x_properties;
+    /* Reason: dma_mr property can't be set */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo dp8393x_info = {

Reviewed-by Aurelien Jarno <[email protected]>

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Hervé Poussineau

author

2015-07-24 18:46 UTC #3 lore raw

PATCH 2/3 net/dp8393x: specify memory operations for PROM PROM

In reply to Hervé Poussineau

This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.

Signed-off-by Hervé Poussineau <[email protected]>

---
 hw/net/dp8393x.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

hw/net/dp8393x.c +11 -1

index 8fafdb0..55168b5 100644
@@ -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;

Signature · 1 line

2.1.4

Aurelien Jarno

2015-07-26 20:11 UTC #4 lore raw

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

Quoted 29 lines · Hervé Poussineau

This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.

Signed-off-by Hervé Poussineau <[email protected]>

---
 hw/net/dp8393x.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

hw/net/dp8393x.c +10 -0

index 8fafdb0..55168b5 100644
@@ -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;

hw/net/dp8393x.c static void dp8393x_realize(DeviceState *dev, Error **errp) · line 856

Quoted 8 lines

hw/net/dp8393x.c +1 -1

@@ -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?

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Hervé Poussineau

author

2015-07-26 20:40 UTC #5 lore raw

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

Hi,

Quoted 33 lines · Aurelien Jarno

Quoted 32 lines · Hervé Poussineau

This fixes a guest-triggerable QEMU crash when guest tries to write to PROM.

Signed-off-by Hervé Poussineau <[email protected]>

---
  hw/net/dp8393x.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

hw/net/dp8393x.c +10 -0

index 8fafdb0..55168b5 100644
@@ -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 */

Quoted 10 lines

hw/net/dp8393x.c static void dp8393x_realize(DeviceState *dev, Error **errp) · line 856
Quoted 5 lines
hw/net/dp8393x.c +1 -1
-    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?= <[email protected]>
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 <[email protected]>

---
 hw/net/dp8393x.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

hw/net/dp8393x.c +8 -2

index 8fafdb0..8ba1d0b 100644
@@ -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++) {

Signature · 3 lines

2.1.4

Hervé

Aurelien Jarno

2015-07-26 22:08 UTC #6 lore raw

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

Quoted 20 lines · Hervé Poussineau

Quoted 4 lines
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

Quoted 7 lines

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?

I have to say I am not sure what is the best, I don't know this part of the code enough. I have added Paolo in Cc:, I guess he might have an idea about that.

Aurelien

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Paolo Bonzini

2015-07-27 10:39 UTC #7 lore raw

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

In reply to Hervé Poussineau

hw/net/dp8393x.c static void dp8393x_realize(DeviceState *dev, Error **errp) · line 850

Quoted 7 lines · Hervé Poussineau

hw/net/dp8393x.c +7 -0

+    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);

Yup, this is better. memory_region_init_rom_device is used whenever you want to catch writes and do something about them. For example, flash devices enable ROMD mode, where reads also go down the MMIO path.

Paolo

Hervé Poussineau

author

2015-07-24 18:46 UTC #8 lore raw

PATCH 3/3 net/dp8393x: remove check of runt packets

In reply to Hervé Poussineau

Ethernet requires that messages are at least 64 bytes on the wire. This limitation does not exist on emulation (no wire message), so remove the check. Netcard is now able to receive small network packets.

Signed-off-by Hervé Poussineau <[email protected]>

---
 hw/net/dp8393x.c | 5 -----
 1 file changed, 5 deletions(-)

hw/net/dp8393x.c +0 -5

index 55168b5..d4f811d 100644
@@ -645,11 +645,6 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
     static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
     int i;
 
-    /* Check for runt packet (remember that checksum is not there) */
-    if (size < 64 - 4) {
-        return (s->regs[SONIC_RCR] & SONIC_RCR_RNT) ? 0 : -1;
-    }
-
     /* Check promiscuous mode */
     if ((s->regs[SONIC_RCR] & SONIC_RCR_PRO) && (buf[0] & 1) == 0) {
         return 0;

Signature · 1 line

2.1.4

Aurelien Jarno

Reviewed-by on v1 3/3

2015-07-26 20:14 UTC #9 lore raw

Re: [Qemu-devel] [PATCH for 2.4 3/3] net/dp8393x: remove check of runt packets

Quoted 15 lines · Hervé Poussineau

Ethernet requires that messages are at least 64 bytes on the wire. This limitation does not exist on emulation (no wire message), so remove the check. Netcard is now able to receive small network packets.
Signed-off-by Hervé Poussineau <[email protected]>
---
 hw/net/dp8393x.c | 5 -----
 1 file changed, 5 deletions(-)
hw/net/dp8393x.c +0 -0
index 55168b5..d4f811d 100644
@@ -645,11 +645,6 @@ static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf,
     static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};

hw/net/dp8393x.c static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf, · line 650

Quoted 10 lines

hw/net/dp8393x.c +0 -5

     int i;
 
-    /* Check for runt packet (remember that checksum is not there) */
-    if (size < 64 - 4) {
-        return (s->regs[SONIC_RCR] & SONIC_RCR_RNT) ? 0 : -1;
-    }
-
     /* Check promiscuous mode */
     if ((s->regs[SONIC_RCR] & SONIC_RCR_PRO) && (buf[0] & 1) == 0) {
         return 0;

Reviewed-by Aurelien Jarno <[email protected]>

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net