[Qemu-devel] [PATCH 0/2] register: fix incorrect read mask

[Qemu-devel] [PATCH 0/2] register: fix incorrect read mask

[Philippe Mathieu-Daudé]

Hi,

This serie fix a bug in register_read_memory(). To avoid duplicated code, a new
inlined function register_enabled_mask() is introduced in the first patch.

Philippe Mathieu-Daudé (2):
  register: inline register_enabled_mask
  register: fix incorrect read mask

 hw/core/register.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] register: inline register_enabled_mask

[Philippe Mathieu-Daudé]

This patch prepares for the fix of register_read() incorrect mask.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/register.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/core/register.c b/hw/core/register.c
index 4bfbc508de..12f4c1e62d 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -59,6 +59,15 @@ static inline uint64_t register_read_val(RegisterInfo *reg)
     return 0; /* unreachable */
 }
 
+static inline uint64_t register_enabled_mask(int data_size, unsigned size)
+{
+    if (data_size < size) {
+        size = data_size;
+    }
+
+    return MAKE_64BIT_MASK(0, size * 8);
+}
+
 void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
                     const char *prefix, bool debug)
 {
@@ -192,11 +201,7 @@ void register_write_memory(void *opaque, hwaddr addr,
     }
 
     /* Generate appropriate write enable mask */
-    if (reg->data_size < size) {
-        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
-    } else {
-        we = MAKE_64BIT_MASK(0, size * 8);
-    }
+    we = register_enabled_mask(reg->data_size, size);
 
     register_write(reg, value, we, reg_array->prefix,
                    reg_array->debug);
-- 
2.11.0

[Qemu-devel] [PATCH 2/2] register: fix incorrect read mask

[Philippe Mathieu-Daudé]

The register_read() function expects a bitmask argument.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/register.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/core/register.c b/hw/core/register.c
index 12f4c1e62d..482e3f827a 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -213,6 +213,7 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
     RegisterInfoArray *reg_array = opaque;
     RegisterInfo *reg = NULL;
     uint64_t read_val;
+    uint64_t re;
     int i;
 
     for (i = 0; i < reg_array->num_elements; i++) {
@@ -228,7 +229,10 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
         return 0;
     }
 
-    read_val = register_read(reg, size * 8, reg_array->prefix,
+    /* Generate appropriate read enable mask */
+    re = register_enabled_mask(reg->data_size, size);
+
+    read_val = register_read(reg, re, reg_array->prefix,
                              reg_array->debug);
 
     return extract64(read_val, 0, size * 8);
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/2] register: fix incorrect read mask

[Alistair Francis]

On Mon, Feb 6, 2017 at 3:39 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The register_read() function expects a bitmask argument.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

The functionality of these two patches looks good to me.

I think it's weird that we convert the write function in one patch
(when we add the function) and the read in another.

I think both patches should just be squashed together. It's a pretty
small change.

Thanks,

Alistair

> ---
>  hw/core/register.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 12f4c1e62d..482e3f827a 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -213,6 +213,7 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      RegisterInfoArray *reg_array = opaque;
>      RegisterInfo *reg = NULL;
>      uint64_t read_val;
> +    uint64_t re;
>      int i;
>
>      for (i = 0; i < reg_array->num_elements; i++) {
> @@ -228,7 +229,10 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>          return 0;
>      }
>
> -    read_val = register_read(reg, size * 8, reg_array->prefix,
> +    /* Generate appropriate read enable mask */
> +    re = register_enabled_mask(reg->data_size, size);
> +
> +    read_val = register_read(reg, re, reg_array->prefix,
>                               reg_array->debug);
>
>      return extract64(read_val, 0, size * 8);
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH 2/2] register: fix incorrect read mask

[Philippe Mathieu-Daudé]

On 02/10/2017 07:19 PM, Alistair Francis wrote:
> On Mon, Feb 6, 2017 at 3:39 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> The register_read() function expects a bitmask argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> The functionality of these two patches looks good to me.
>
> I think it's weird that we convert the write function in one patch
> (when we add the function) and the read in another.
>
> I think both patches should just be squashed together. It's a pretty
> small change.
>
> Thanks,
>
> Alistair

Sure, will do in v2.

Thanks!

>
>> ---
>>  hw/core/register.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index 12f4c1e62d..482e3f827a 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -213,6 +213,7 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>>      RegisterInfoArray *reg_array = opaque;
>>      RegisterInfo *reg = NULL;
>>      uint64_t read_val;
>> +    uint64_t re;
>>      int i;
>>
>>      for (i = 0; i < reg_array->num_elements; i++) {
>> @@ -228,7 +229,10 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>>          return 0;
>>      }
>>
>> -    read_val = register_read(reg, size * 8, reg_array->prefix,
>> +    /* Generate appropriate read enable mask */
>> +    re = register_enabled_mask(reg->data_size, size);
>> +
>> +    read_val = register_read(reg, re, reg_array->prefix,
>>                               reg_array->debug);
>>
>>      return extract64(read_val, 0, size * 8);
>> --
>> 2.11.0
>>
>>

[Qemu-devel] [PATCH v2] register: fix incorrect read mask

[Philippe Mathieu-Daudé]

The register_read() function expects a bitmask argument. To avoid duplicated
code, a new inlined function register_enabled_mask() is introduced.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 v2: squashed together both previous commits as suggested by Francis Alistair.

 hw/core/register.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/core/register.c b/hw/core/register.c
index 4bfbc508de..482e3f827a 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -59,6 +59,15 @@ static inline uint64_t register_read_val(RegisterInfo *reg)
     return 0; /* unreachable */
 }
 
+static inline uint64_t register_enabled_mask(int data_size, unsigned size)
+{
+    if (data_size < size) {
+        size = data_size;
+    }
+
+    return MAKE_64BIT_MASK(0, size * 8);
+}
+
 void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
                     const char *prefix, bool debug)
 {
@@ -192,11 +201,7 @@ void register_write_memory(void *opaque, hwaddr addr,
     }
 
     /* Generate appropriate write enable mask */
-    if (reg->data_size < size) {
-        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
-    } else {
-        we = MAKE_64BIT_MASK(0, size * 8);
-    }
+    we = register_enabled_mask(reg->data_size, size);
 
     register_write(reg, value, we, reg_array->prefix,
                    reg_array->debug);
@@ -208,6 +213,7 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
     RegisterInfoArray *reg_array = opaque;
     RegisterInfo *reg = NULL;
     uint64_t read_val;
+    uint64_t re;
     int i;
 
     for (i = 0; i < reg_array->num_elements; i++) {
@@ -223,7 +229,10 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
         return 0;
     }
 
-    read_val = register_read(reg, size * 8, reg_array->prefix,
+    /* Generate appropriate read enable mask */
+    re = register_enabled_mask(reg->data_size, size);
+
+    read_val = register_read(reg, re, reg_array->prefix,
                              reg_array->debug);
 
     return extract64(read_val, 0, size * 8);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2] register: fix incorrect read mask

[Alistair Francis]

On Fri, Feb 10, 2017 at 7:43 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The register_read() function expects a bitmask argument. To avoid duplicated

The register_read() and register_write() functions

> code, a new inlined function register_enabled_mask() is introduced.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  v2: squashed together both previous commits as suggested by Francis Alistair.
>
>  hw/core/register.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 4bfbc508de..482e3f827a 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -59,6 +59,15 @@ static inline uint64_t register_read_val(RegisterInfo *reg)
>      return 0; /* unreachable */
>  }
>
> +static inline uint64_t register_enabled_mask(int data_size, unsigned size)
> +{
> +    if (data_size < size) {
> +        size = data_size;
> +    }
> +
> +    return MAKE_64BIT_MASK(0, size * 8);
> +}
> +
>  void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
>                      const char *prefix, bool debug)
>  {
> @@ -192,11 +201,7 @@ void register_write_memory(void *opaque, hwaddr addr,
>      }
>
>      /* Generate appropriate write enable mask */
> -    if (reg->data_size < size) {
> -        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
> -    } else {
> -        we = MAKE_64BIT_MASK(0, size * 8);
> -    }
> +    we = register_enabled_mask(reg->data_size, size);
>
>      register_write(reg, value, we, reg_array->prefix,
>                     reg_array->debug);
> @@ -208,6 +213,7 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      RegisterInfoArray *reg_array = opaque;
>      RegisterInfo *reg = NULL;
>      uint64_t read_val;
> +    uint64_t re;
>      int i;
>
>      for (i = 0; i < reg_array->num_elements; i++) {
> @@ -223,7 +229,10 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>          return 0;
>      }
>
> -    read_val = register_read(reg, size * 8, reg_array->prefix,
> +    /* Generate appropriate read enable mask */
> +    re = register_enabled_mask(reg->data_size, size);
> +
> +    read_val = register_read(reg, re, reg_array->prefix,
>                               reg_array->debug);
>
>      return extract64(read_val, 0, size * 8);
> --
> 2.11.0
>
>