[Qemu-devel] [PATCH] mipsnet incorrect device ID fix

[Qemu-devel] [PATCH] mipsnet incorrect device ID fix

[Vijay Kumar]

The mipsnet device returns wrong values for device ID, since it returns 
the contents of the pointer rather that the contents of the device ID 
string. Also the contents of the string is returned such that the order 
is host endianess dependent. The patch fixes both these issues.

Signed-off-by: Vijay Kumar B. <vijaykumar@bravegnu.org>

--- qemu-orig/hw/mipsnet.c	2007-12-27 11:18:52.000000000 +0530
+++ qemu-mod/hw/mipsnet.c	2008-02-20 20:23:44.000000000 +0530
@@ -101,6 +101,19 @@
      mipsnet_update_irq(s);
  }

+static uint32_t bytes_to_int32(const unsigned char *arr)
+{
+    int i;
+    uint32_t ret = 0;
+    int nbytes = sizeof(int32_t);
+
+    for (i = 0; i < nbytes; i++) {
+	ret = ret << 8 | arr[nbytes - 1 - i];
+    }
+
+    return ret;
+}
+
  static uint32_t mipsnet_ioport_read(void *opaque, uint32_t addr)
  {
      MIPSnetState *s = opaque;
@@ -110,10 +123,10 @@
      addr &= 0x3f;
      switch (addr) {
      case MIPSNET_DEV_ID:
-	ret = *((uint32_t *)&devid);
+	ret = bytes_to_int32(devid);
          break;
      case MIPSNET_DEV_ID + 4:
-	ret = *((uint32_t *)(&devid + 4));
+	ret = bytes_to_int32(devid + 4);
          break;
      case MIPSNET_BUSY:
  	ret = s->busy;

Re: [Qemu-devel] [PATCH] mipsnet incorrect device ID fix

[Aurelien Jarno]

On Fri, Feb 22, 2008 at 06:16:25AM +0530, Vijay Kumar wrote:
> The mipsnet device returns wrong values for device ID, since it returns  
> the contents of the pointer rather that the contents of the device ID  
> string. Also the contents of the string is returned such that the order  
> is host endianess dependent. The patch fixes both these issues.
>
> Signed-off-by: Vijay Kumar B. <vijaykumar@bravegnu.org>
>
> --- qemu-orig/hw/mipsnet.c	2007-12-27 11:18:52.000000000 +0530
> +++ qemu-mod/hw/mipsnet.c	2008-02-20 20:23:44.000000000 +0530
> @@ -101,6 +101,19 @@
>      mipsnet_update_irq(s);
>  }
>
> +static uint32_t bytes_to_int32(const unsigned char *arr)
> +{
> +    int i;
> +    uint32_t ret = 0;
> +    int nbytes = sizeof(int32_t);
> +
> +    for (i = 0; i < nbytes; i++) {
> +	ret = ret << 8 | arr[nbytes - 1 - i];
> +    }
> +
> +    return ret;
> +}

Why not use le32_to_cpu() which does the same, but in an optimized way?

>  static uint32_t mipsnet_ioport_read(void *opaque, uint32_t addr)
>  {
>      MIPSnetState *s = opaque;
> @@ -110,10 +123,10 @@
>      addr &= 0x3f;
>      switch (addr) {
>      case MIPSNET_DEV_ID:
> -	ret = *((uint32_t *)&devid);
> +	ret = bytes_to_int32(devid);

That's the '&' which is wrong here. The string can be accessed with
*((uint32_t *)devid). So you can simply use:

        ret = le32_to_cpu(*((uint32_t *)devid))

>          break;
>      case MIPSNET_DEV_ID + 4:
> -	ret = *((uint32_t *)(&devid + 4));
> +	ret = bytes_to_int32(devid + 4);

        ret = le32_to_cpu(*((uint32_t *)devid + 4));

>          break;
>      case MIPSNET_BUSY:
>  	ret = s->busy;
>
>
>

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] mipsnet incorrect device ID fix

[Paul Brook]

> That's the '&' which is wrong here. The string can be accessed with
> *((uint32_t *)devid). So you can simply use:
>
>         ret = le32_to_cpu(*((uint32_t *)devid))

No you can't. Even ignoring the aliasing violation, devid might not be 
sufficiently aligned.

Paul

Re: [Qemu-devel] [PATCH] mipsnet incorrect device ID fix

[Aurelien Jarno]

On Thu, Mar 13, 2008 at 01:18:46AM +0000, Paul Brook wrote:
> > That's the '&' which is wrong here. The string can be accessed with
> > *((uint32_t *)devid). So you can simply use:
> >
> >         ret = le32_to_cpu(*((uint32_t *)devid))
> 
> No you can't. Even ignoring the aliasing violation, devid might not be 
> sufficiently aligned.
> 

Oops you are right, I have been troubled by the original code. As
suggested by Paul on IRC, the best is most probably to hardcode the
hex values and put the string on a comment.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net