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

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

[Andrey Shumilin]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5442 bytes --]

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Michael Tokarev]

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

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Andrey Shumilin]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 73885 bytes --]

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Philippe Mathieu-Daudé]

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

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[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>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Andrey Shumilin]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4153 bytes --]

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Andrey Shumilin]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4468 bytes --]

Re: [sdl-qemu] [PATCH v1] /hw/intc/arm_gic WRONG ARGUMENTS

[Peter Maydell]

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