[Qemu-devel] [PATCH for 2.4 0/3] net/dp8393x: misc fixes

[Qemu-devel] [PATCH for 2.4 0/3] net/dp8393x: misc fixes

[Hervé Poussineau]

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(-)

-- 
2.1.4

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

[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 <hpoussin@reactos.org>
---
 hw/net/dp8393x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cd889bc..8fafdb0 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -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 = {
-- 
2.1.4

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

[Hervé Poussineau]

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;
-- 
2.1.4

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

[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 <hpoussin@reactos.org>
---
 hw/net/dp8393x.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 55168b5..d4f811d 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -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;
-- 
2.1.4

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

[Aurelien Jarno]

On 2015-07-24 20:42, Hervé Poussineau wrote:
> 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 <hpoussin@reactos.org>
> ---
>  hw/net/dp8393x.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cd889bc..8fafdb0 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -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 <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

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

[Aurelien Jarno]

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?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

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

[Aurelien Jarno]

On 2015-07-24 20:42, Hervé Poussineau wrote:
> 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 <hpoussin@reactos.org>
> ---
>  hw/net/dp8393x.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 55168b5..d4f811d 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -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;

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

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

[Hervé Poussineau]

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é

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

[Aurelien Jarno]

On 2015-07-26 22:35, Hervé Poussineau wrote:
> >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?
> 

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

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

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

[Paolo Bonzini]

On 26/07/2015 22:35, Hervé Poussineau wrote:
> +    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