diff mbox series

target/i386: Remove dead assignment to ss in do_interrupt64()

Message ID 20240723162525.1585743-1-peter.maydell@linaro.org
State Superseded
Headers show
Series target/i386: Remove dead assignment to ss in do_interrupt64() | expand

Commit Message

Peter Maydell July 23, 2024, 4:25 p.m. UTC
Coverity points out that in do_interrupt64() in the "to inner
privilege" codepath we set "ss = 0", but because we also set
"new_stack = 1" there, later in the function we will always override
that value of ss with "ss = 0 | dpl".

Remove the unnecessary initialization of ss, which allows us to
reduce the scope of the variable to only where it is used.  Borrow a
comment from helper_lcall_protected() that explains what "0 | dpl"
means here.

Resolves: Coverity CID 1527395
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/i386/tcg/seg_helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Richard Henderson July 24, 2024, 12:01 a.m. UTC | #1
On 7/24/24 02:25, Peter Maydell wrote:
> Coverity points out that in do_interrupt64() in the "to inner
> privilege" codepath we set "ss = 0", but because we also set
> "new_stack = 1" there, later in the function we will always override
> that value of ss with "ss = 0 | dpl".
> 
> Remove the unnecessary initialization of ss, which allows us to
> reduce the scope of the variable to only where it is used.  Borrow a
> comment from helper_lcall_protected() that explains what "0 | dpl"
> means here.
> 
> Resolves: Coverity CID 1527395
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/i386/tcg/seg_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Philippe Mathieu-Daudé July 24, 2024, 6:57 a.m. UTC | #2
On 23/7/24 18:25, Peter Maydell wrote:
> Coverity points out that in do_interrupt64() in the "to inner
> privilege" codepath we set "ss = 0", but because we also set
> "new_stack = 1" there, later in the function we will always override
> that value of ss with "ss = 0 | dpl".
> 
> Remove the unnecessary initialization of ss, which allows us to
> reduce the scope of the variable to only where it is used.  Borrow a
> comment from helper_lcall_protected() that explains what "0 | dpl"
> means here.
> 
> Resolves: Coverity CID 1527395
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/i386/tcg/seg_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Peter Maydell July 29, 2024, 4:02 p.m. UTC | #3
On Tue, 23 Jul 2024 at 17:25, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity points out that in do_interrupt64() in the "to inner
> privilege" codepath we set "ss = 0", but because we also set
> "new_stack = 1" there, later in the function we will always override
> that value of ss with "ss = 0 | dpl".
>
> Remove the unnecessary initialization of ss, which allows us to
> reduce the scope of the variable to only where it is used.  Borrow a
> comment from helper_lcall_protected() that explains what "0 | dpl"
> means here.
>
> Resolves: Coverity CID 1527395
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I'll take this via target-arm.next since I'm doing a pullreq
anyway, unless you'd prefer otherwise.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
index aac092a356b..bab552cd535 100644
--- a/target/i386/tcg/seg_helper.c
+++ b/target/i386/tcg/seg_helper.c
@@ -926,7 +926,7 @@  static void do_interrupt64(CPUX86State *env, int intno, int is_int,
     target_ulong ptr;
     int type, dpl, selector, cpl, ist;
     int has_error_code, new_stack;
-    uint32_t e1, e2, e3, ss, eflags;
+    uint32_t e1, e2, e3, eflags;
     target_ulong old_eip, offset;
     bool set_rf;
     StackAccess sa;
@@ -1007,7 +1007,6 @@  static void do_interrupt64(CPUX86State *env, int intno, int is_int,
         /* to inner privilege */
         new_stack = 1;
         sa.sp = get_rsp_from_tss(env, ist != 0 ? ist + 3 : dpl);
-        ss = 0;
     } else {
         /* to same privilege */
         if (env->eflags & VM_MASK) {
@@ -1040,7 +1039,7 @@  static void do_interrupt64(CPUX86State *env, int intno, int is_int,
     env->eflags &= ~(TF_MASK | VM_MASK | RF_MASK | NT_MASK);
 
     if (new_stack) {
-        ss = 0 | dpl;
+        uint32_t ss = 0 | dpl; /* SS = NULL selector with RPL = new CPL */
         cpu_x86_load_seg_cache(env, R_SS, ss, 0, 0, dpl << DESC_DPL_SHIFT);
     }
     env->regs[R_ESP] = sa.sp;