diff mbox series

[sdl-qemu,v1] /hw/intc/arm_gic WRONG ARGUMENTS

Message ID a4cbfe6c-27d6-4df0-ae31-db0d60d88f9e@nppct.ru (mailing list archive)
State New
Headers show
Series [sdl-qemu,v1] /hw/intc/arm_gic WRONG ARGUMENTS | expand

Commit Message

Andrey Shumilin May 5, 2024, 5:58 p.m. UTC 
1. Possibly mismatched call arguments in function 'gic_apr_ns_view':
    'cpu' and 'regno' passed in place of 'int regno' and 'int cpu'.
 2. Possibly mismatched call arguments in function
    'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int
    regno' and 'int cpu'.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001

From: Andrey Shumilin <shum.sdl@nppct.ru>
Date: Sun, 5 May 2024 20:13:40 +0300
Subject: [PATCH] Patch hw/intc/arm_gic.c

Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru>
---
  hw/intc/arm_gic.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

          }
@@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int 
cpu, int offset,
              s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
-            gic_apr_write_ns_view(s, regno, cpu, value);
+            gic_apr_write_ns_view(s, cpu, regno, value);
          } else {
              s->apr[regno][cpu] = value;
          }

Comments

Michael Tokarev May 5, 2024, 7:51 p.m. UTC | #1 
05.05.2024 20:58, Andrey Shumilin wrote:
[unreadable text skipped]

FWIW, your message is very difficult to read due to colors used within.

/mjt
Andrey Shumilin May 5, 2024, 7:57 p.m. UTC | #2 
1. Possibly mismatched call arguments in function 'gic_apr_ns_view':
    'cpu' and 'regno' passed in place of 'int regno' and 'int cpu'.
 2. Possibly mismatched call arguments in function
    'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int
    regno' and 'int cpu'.

Found by Linux Verification Center (linuxtesting.org) with SVACE.


 From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
From: Andrey Shumilin <shum.sdl@nppct.ru>
Date: Sun, 5 May 2024 20:13:40 +0300
Subject: [PATCH] Patch hw/intc/arm_gic.c

Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru>
---
  hw/intc/arm_gic.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7a34bc0998..47f01e45e3 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int 
cpu, int offset,
              *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
-            *data = gic_apr_ns_view(s, regno, cpu);
+            *data = gic_apr_ns_view(s, cpu, regno);
          } else {
              *data = s->apr[regno][cpu];
          }
@@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int 
cpu, int offset,
              s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
-            gic_apr_write_ns_view(s, regno, cpu, value);
+            gic_apr_write_ns_view(s, cpu, regno, value);
          } else {
              s->apr[regno][cpu] = value;
          }
Philippe Mathieu-Daudé May 6, 2024, 7:49 a.m. UTC | #3 
On 5/5/24 21:57, Andrey Shumilin wrote:
>  1. Possibly mismatched call arguments in function 'gic_apr_ns_view':
>     'cpu' and 'regno' passed in place of 'int regno' and 'int cpu'.
>  2. Possibly mismatched call arguments in function
>     'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int
>     regno' and 'int cpu'.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 

Fixes: 51fd06e0ee ("hw/intc/arm_gic: Fix handling of GICC_APR<n>, 
GICC_NSAPR<n> registers")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 
>  From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
> From: Andrey Shumilin <shum.sdl@nppct.ru>
> Date: Sun, 5 May 2024 20:13:40 +0300
> Subject: [PATCH] Patch hw/intc/arm_gic.c

Your patch is malformed, see:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#submitting-your-patches

> 
> Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru>
> ---
>   hw/intc/arm_gic.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7a34bc0998..47f01e45e3 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int 
> cpu, int offset,
>               *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
> -            *data = gic_apr_ns_view(s, regno, cpu);
> +            *data = gic_apr_ns_view(s, cpu, regno);
>           } else {
>               *data = s->apr[regno][cpu];
>           }
> @@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int 
> cpu, int offset,
>               s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
> -            gic_apr_write_ns_view(s, regno, cpu, value);
> +            gic_apr_write_ns_view(s, cpu, regno, value);
>           } else {
>               s->apr[regno][cpu] = value;
>           }
>
Alex Bennée May 6, 2024, 10:02 a.m. UTC | #4 
Andrey Shumilin <shum.sdl@nppct.ru> writes:

> 1 Possibly mismatched call arguments in function 'gic_apr_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int
>  cpu'.
> 2 Possibly mismatched call arguments in function 'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and
>  'int cpu'.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

So this purely a heuristic test based on the parameter names?

>
> From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
> From: Andrey Shumilin <shum.sdl@nppct.ru>
> Date: Sun, 5 May 2024 20:13:40 +0300
> Subject: [PATCH] Patch hw/intc/arm_gic.c
>
> Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru>
> ---
>  hw/intc/arm_gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7a34bc0998..47f01e45e3 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>              *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
>          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
> -            *data = gic_apr_ns_view(s, regno, cpu);
> +            *data = gic_apr_ns_view(s, cpu, regno);
>          } else {
>              *data = s->apr[regno][cpu];
>          }
> @@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>              s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
>          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
> -            gic_apr_write_ns_view(s, regno, cpu, value);
> +            gic_apr_write_ns_view(s, cpu, regno, value);
>          } else {
>              s->apr[regno][cpu] = value;
>          }

Ahh C's lack of strong typing wins again :-/

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Andrey Shumilin May 6, 2024, 7:11 p.m. UTC | #5 
1. s - This argument passes a pointer to the GICState structure that
    contains the state of the interrupt controller. This argument is
    passed first and used correctly.
 2. regno - This is the register number, which in the context of
    gic_cpu_read is calculated as (offset - 0xd0) / 4. In gic_cpu_read
    code, this number is used to identify the specific APR register to
    be read or modified. In gic_apr_ns_view, this number is also used to
    determine which NSAPR register to read or how to compute bits,
    implying that it is used as a register index.
 3. cpu - This is the CPU number used to address a particular CPU in the
    nsapr, apr, and other arrays.

Based on the context, it is important that regno and cpu are passed to 
gic_apr_ns_view in the correct order for correct handling of arrays and 
indexes within this function. An error in the order of the arguments can 
result in incorrect data handling, as arrays are indexed first by CPU 
number and then by register number. In the considered call 
gic_apr_ns_view the arguments are passed in the wrong order

06.05.2024 13:02, Alex Bennée пишет:
> Andrey Shumilin<shum.sdl@nppct.ru>  writes:
>
>> 1 Possibly mismatched call arguments in function 'gic_apr_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int
>>   cpu'.
>> 2 Possibly mismatched call arguments in function 'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and
>>   'int cpu'.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> So this purely a heuristic test based on the parameter names?
>
>>  From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
>> From: Andrey Shumilin<shum.sdl@nppct.ru>
>> Date: Sun, 5 May 2024 20:13:40 +0300
>> Subject: [PATCH] Patch hw/intc/arm_gic.c
>>
>> Signed-off-by: Andrey Shumilin<shum.sdl@nppct.ru>
>> ---
>>   hw/intc/arm_gic.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 7a34bc0998..47f01e45e3 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>>               *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            *data = gic_apr_ns_view(s, regno, cpu);
>> +            *data = gic_apr_ns_view(s, cpu, regno);
>>           } else {
>>               *data = s->apr[regno][cpu];
>>           }
>> @@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>>               s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            gic_apr_write_ns_view(s, regno, cpu, value);
>> +            gic_apr_write_ns_view(s, cpu, regno, value);
>>           } else {
>>               s->apr[regno][cpu] = value;
>>           }
> Ahh C's lack of strong typing wins again :-/
>
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
>
Andrey Shumilin May 7, 2024, 4:42 a.m. UTC | #6 
1. s - This argument passes a pointer to the GICState structure that
    contains the state of the interrupt controller. This argument is
    passed first and used correctly.
 2. regno - This is the register number, which in the context of
    gic_cpu_read is calculated as (offset - 0xd0) / 4. In gic_cpu_read
    code, this number is used to identify the specific APR register to
    be read or modified. In gic_apr_ns_view, this number is also used to
    determine which NSAPR register to read or how to compute bits,
    implying that it is used as a register index.
 3. cpu - This is the CPU number used to address a particular CPU in the
    nsapr, apr, and other arrays.

Based on the context, it is important that regno and cpu are passed to 
gic_apr_ns_view in the correct order for correct handling of arrays and 
indexes within this function. An error in the order of the arguments can 
result in incorrect data handling, as arrays are indexed first by CPU 
number and then by register number. In the considered call 
gic_apr_ns_view the arguments are passed in the wrong order


Fixes: 51fd06e0ee ("hw/intc/arm_gic: Fix handling of GICC_APR<n>, 
GICC_NSAPR<n> registers")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

06.05.2024 13:02, Alex Bennée пишет:
> Andrey Shumilin<shum.sdl@nppct.ru>  writes:
>
>> 1 Possibly mismatched call arguments in function 'gic_apr_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int
>>   cpu'.
>> 2 Possibly mismatched call arguments in function 'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and
>>   'int cpu'.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> So this purely a heuristic test based on the parameter names?
>
>>  From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
>> From: Andrey Shumilin<shum.sdl@nppct.ru>
>> Date: Sun, 5 May 2024 20:13:40 +0300
>> Subject: [PATCH] Patch hw/intc/arm_gic.c
>>
>> Signed-off-by: Andrey Shumilin<shum.sdl@nppct.ru>
>> ---
>>   hw/intc/arm_gic.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 7a34bc0998..47f01e45e3 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -1658,7 +1658,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>>               *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            *data = gic_apr_ns_view(s, regno, cpu);
>> +            *data = gic_apr_ns_view(s, cpu, regno);
>>           } else {
>>               *data = s->apr[regno][cpu];
>>           }
>> @@ -1746,7 +1746,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>>               s->h_apr[gic_get_vcpu_real_id(cpu)] = value;
>>           } else if (gic_cpu_ns_access(s, cpu, attrs)) {
>>               /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
>> -            gic_apr_write_ns_view(s, regno, cpu, value);
>> +            gic_apr_write_ns_view(s, cpu, regno, value);
>>           } else {
>>               s->apr[regno][cpu] = value;
>>           }
> Ahh C's lack of strong typing wins again :-/
>
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
>
Peter Maydell May 7, 2024, 10:13 a.m. UTC | #7 
On Sun, 5 May 2024 at 20:58, Andrey Shumilin <shum.sdl@nppct.ru> wrote:
>
> Possibly mismatched call arguments in function 'gic_apr_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int cpu'.
> Possibly mismatched call arguments in function 'gic_apr_write_ns_view': 'cpu' and 'regno' passed in place of 'int regno' and 'int cpu'.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
>
> From 23b142f5046ba9d3aec57217f6d8f3127f9bff69 Mon Sep 17 00:00:00 2001
> From: Andrey Shumilin <shum.sdl@nppct.ru>
> Date: Sun, 5 May 2024 20:13:40 +0300
> Subject: [PATCH] Patch hw/intc/arm_gic.c
>
> Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru>
> ---
>  hw/intc/arm_gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Thanks, I have applied your patch to target-arm.next (with a
rewritten commit message).

For future patches, it would be good if you can sort out the
mail format issues. It looks like you're using Thunderbird,
which ought to be configurable to do a reasonable job.
In particular, if you can set it to send plain text only
(not plain-text + HTML multipart) for all mailing list
messages that will help a lot. That will help the automated
tooling to have a better time trying to interpret it.
You might alternatively consider git-send-email.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7a34bc0998..47f01e45e3 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1658,7 +1658,7 @@  static MemTxResult gic_cpu_read(GICState *s, int 
cpu, int offset,
              *data = s->h_apr[gic_get_vcpu_real_id(cpu)];
          } else if (gic_cpu_ns_access(s, cpu, attrs)) {
              /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
-            *data = gic_apr_ns_view(s, regno, cpu);
+            *data = gic_apr_ns_view(s, cpu, regno);
          } else {
              *data = s->apr[regno][cpu];