Patchwork [09/11] i386: avoid a write only variable

login
register
mail settings
Submitter Blue Swirl
Date Oct. 6, 2010, 9:34 p.m.
Message ID <AANLkTinWu7ABrSP9EVZFeC==+ZS+qDOur8rAPNjdzo6i@mail.gmail.com>
Download mbox | patch
Permalink /patch/66980/
State New
Headers show

Comments

Blue Swirl - Oct. 6, 2010, 9:34 p.m.
Compiling with GCC 4.6.0 20100925 produced warnings:
/src/qemu/target-i386/op_helper.c: In function 'switch_tss':
/src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
set but not used [-Werror=unused-but-set-variable]

Fix by deleting the variable.

Alternatively the flag could be implemented by generating a debug
exception if the flag is set.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 target-i386/op_helper.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
Paolo Bonzini - Oct. 7, 2010, 7:27 a.m.
On 10/06/2010 11:34 PM, Blue Swirl wrote:
> Compiling with GCC 4.6.0 20100925 produced warnings:
> /src/qemu/target-i386/op_helper.c: In function 'switch_tss':
> /src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
> set but not used [-Werror=unused-but-set-variable]
>
> Fix by deleting the variable.

Again, this warning tells us the emulation is incorrect, so it's wrong 
to remove it.

Paolo
malc - Oct. 7, 2010, 8:27 a.m.
On Thu, 7 Oct 2010, Paolo Bonzini wrote:

> On 10/06/2010 11:34 PM, Blue Swirl wrote:
> > Compiling with GCC 4.6.0 20100925 produced warnings:
> > /src/qemu/target-i386/op_helper.c: In function 'switch_tss':
> > /src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
> > set but not used [-Werror=unused-but-set-variable]
> > 
> > Fix by deleting the variable.
> 
> Again, this warning tells us the emulation is incorrect, so it's wrong to
> remove it.
> 

http://support.amd.com/us/Processor_TechDocs/24593.pdf 

12.2.5
T (Trap) Bit--■Bit 0 of byte 64h, static field. This bit, when set to1,
causes a debug exception (#DB) to occur on a task switch. See "Beakpoint
Instruction (INT3)" page 340 for additional information.

current code never checks and never traps, which is indeed not correct.
Blue Swirl - Oct. 7, 2010, 5:32 p.m.
On Thu, Oct 7, 2010 at 7:27 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/06/2010 11:34 PM, Blue Swirl wrote:
>>
>> Compiling with GCC 4.6.0 20100925 produced warnings:
>> /src/qemu/target-i386/op_helper.c: In function 'switch_tss':
>> /src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
>> set but not used [-Werror=unused-but-set-variable]
>>
>> Fix by deleting the variable.
>
> Again, this warning tells us the emulation is incorrect, so it's wrong to
> remove it.

The warning tells us that the emulation is unfinished.

It's probably unfinished because debugging TSS switches hasn't ever
been needed by anyone. If anyone in the future cares enough about T
bit, it's easy to implement it even after this part has been removed.

But if someone proposes a better patch, I'd be happy to use that
instead of this.
Blue Swirl - Oct. 7, 2010, 5:36 p.m.
On Thu, Oct 7, 2010 at 8:27 AM, malc <av1474@comtv.ru> wrote:
> On Thu, 7 Oct 2010, Paolo Bonzini wrote:
>
>> On 10/06/2010 11:34 PM, Blue Swirl wrote:
>> > Compiling with GCC 4.6.0 20100925 produced warnings:
>> > /src/qemu/target-i386/op_helper.c: In function 'switch_tss':
>> > /src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
>> > set but not used [-Werror=unused-but-set-variable]
>> >
>> > Fix by deleting the variable.
>>
>> Again, this warning tells us the emulation is incorrect, so it's wrong to
>> remove it.
>>
>
> http://support.amd.com/us/Processor_TechDocs/24593.pdf
>
> 12.2.5
> T (Trap) Bit--■Bit 0 of byte 64h, static field. This bit, when set to1,
> causes a debug exception (#DB) to occur on a task switch. See "Beakpoint
> Instruction (INT3)" page 340 for additional information.

Yes, I read that before making the patch. But page 340 didn't seem to
give any relevant information.
malc - Oct. 7, 2010, 5:44 p.m.
On Thu, 7 Oct 2010, Blue Swirl wrote:

> On Thu, Oct 7, 2010 at 8:27 AM, malc <av1474@comtv.ru> wrote:
> > On Thu, 7 Oct 2010, Paolo Bonzini wrote:
> >
> >> On 10/06/2010 11:34 PM, Blue Swirl wrote:
> >> > Compiling with GCC 4.6.0 20100925 produced warnings:
> >> > /src/qemu/target-i386/op_helper.c: In function 'switch_tss':
> >> > /src/qemu/target-i386/op_helper.c:283:53: error: variable 'new_trap'
> >> > set but not used [-Werror=unused-but-set-variable]
> >> >
> >> > Fix by deleting the variable.
> >>
> >> Again, this warning tells us the emulation is incorrect, so it's wrong to
> >> remove it.
> >>
> >
> > http://support.amd.com/us/Processor_TechDocs/24593.pdf
> >
> > 12.2.5
> > T (Trap) Bit--■Bit 0 of byte 64h, static field. This bit, when set to1,
> > causes a debug exception (#DB) to occur on a task switch. See "Beakpoint
> > Instruction (INT3)" page 340 for additional information.
> 
> Yes, I read that before making the patch. But page 340 didn't seem to
> give any relevant information.

13.2.4  Breakpoint Instruction (INT3)
Is what was referred to apparently.

Patch

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index ec6b3e9..a5462a9 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -280,7 +280,7 @@  static void switch_tss(int tss_selector,
     int tss_limit, tss_limit_max, type, old_tss_limit_max, old_type, v1, v2, i;
     target_ulong tss_base;
     uint32_t new_regs[8], new_segs[6];
-    uint32_t new_eflags, new_eip, new_cr3, new_ldt, new_trap;
+    uint32_t new_eflags, new_eip, new_cr3, new_ldt;
     uint32_t old_eflags, eflags_mask;
     SegmentCache *dt;
     int index;
@@ -334,7 +334,6 @@  static void switch_tss(int tss_selector,
         for(i = 0; i < 6; i++)
             new_segs[i] = lduw_kernel(tss_base + (0x48 + i * 4));
         new_ldt = lduw_kernel(tss_base + 0x60);
-        new_trap = ldl_kernel(tss_base + 0x64);
     } else {
         /* 16 bit */
         new_cr3 = 0;
@@ -347,7 +346,6 @@  static void switch_tss(int tss_selector,
         new_ldt = lduw_kernel(tss_base + 0x2a);
         new_segs[R_FS] = 0;
         new_segs[R_GS] = 0;
-        new_trap = 0;
     }

     /* NOTE: we must avoid memory exceptions during the task switch,