| 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 |
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~
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>
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 --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;
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(-)