Message ID | a4cbfe6c-27d6-4df0-ae31-db0d60d88f9e@nppct.ru |
---|---|
State | New |
Headers | show |
Series | [sdl-qemu,v1] /hw/intc/arm_gic WRONG ARGUMENTS | expand |
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
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; }
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; > } >
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>
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> >
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> >
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 --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];