avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

PATCH Applied Glauber de Oliveira Costa 4 participants 16 messages Dec 13, 2007 virtio core

Share & tools

Glauber de Oliveira Costa

author

2007-12-13 17:13 UTC #0 lore raw

PATCH 1/1 avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

By Andi Kleen's suggestion, this patch removes pack_ldt() and pack_tss() wrappers in favour of a general wrapper. It saves us an ifdef and some lines of code, but more importantly, it's more elegant.

No functional change is made.

Signed-off-by Glauber de Oliveira Costa <[email protected]>

---
 include/asm-x86/desc.h |   55 ++++++++++++++---------------------------------
 1 files changed, 17 insertions(+), 38 deletions(-)

include/asm-x86/desc.h +17 ~~-38~~

index 8849666..e302a02 100644
@@ -125,21 +125,6 @@ static inline void native_write_gdt_entry(struct desc_struct *gdt, int entry,
 	memcpy(&gdt[entry], desc, size);
 }
 
-static inline void set_tssldt_descriptor(struct ldttss_desc64 *d,
-					 unsigned long tss, unsigned type,
-					 unsigned size)
-{
-	memset(d, 0, sizeof(*d));
-	d->limit0 = size & 0xFFFF;
-	d->base0 = PTR_LOW(tss);
-	d->base1 = PTR_MIDDLE(tss) & 0xFF;
-	d->type = type;
-	d->p = 1;
-	d->limit1 = (size >> 16) & 0xF;
-	d->base2 = (PTR_MIDDLE(tss) >> 8) & 0xFF;
-	d->base3 = PTR_HIGH(tss);
-}
-
 static inline void pack_descriptor(struct desc_struct *desc, unsigned long base,
 				   unsigned long limit, unsigned char type,
 				   unsigned char flags)

@@ -151,30 +136,24 @@ static inline void pack_descriptor(struct desc_struct *desc, unsigned long base,
 	desc->p = 1;
 }
 
-static inline void pack_ldt(ldt_desc *ldt, unsigned long addr,
-			   unsigned size)
-{
 
-#ifdef CONFIG_X86_64
-		set_tssldt_descriptor(ldt,
-			     addr, DESC_LDT, size);
-#else
-		pack_descriptor(ldt, (unsigned long)addr,
-				size,
-				0x80 | DESC_LDT, 0);
-#endif
-}
-
-static inline void pack_tss(tss_desc *tss, unsigned long addr,
-			   unsigned size, unsigned entry)
+static inline void set_tssldt_descriptor(void *d, unsigned long addr,
+					 unsigned type, unsigned size)
 {
 #ifdef CONFIG_X86_64
-		set_tssldt_descriptor(tss,
-			     addr, entry, size);
+	struct ldttss_desc64 *desc = d;
+	memset(desc, 0, sizeof(*desc));
+	desc->limit0 = size & 0xFFFF;
+	desc->base0 = PTR_LOW(addr);
+	desc->base1 = PTR_MIDDLE(addr) & 0xFF;
+	desc->type = type;
+	desc->p = 1;
+	desc->limit1 = (size >> 16) & 0xF;
+	desc->base2 = (PTR_MIDDLE(addr) >> 8) & 0xFF;
+	desc->base3 = PTR_HIGH(addr);
 #else
-		pack_descriptor(tss, (unsigned long)addr,
-				size,
-				0x80 | entry, 0);
+
+	pack_descriptor((struct desc_struct *)d, addr, size, 0x80 | type, 0);
 #endif
 }

@@ -190,7 +169,7 @@ static inline void __set_tss_desc(unsigned cpu, unsigned int entry, void *addr)
 	 * -1? seg base+limit should be pointing to the address of the
 	 * last valid byte
 	 */
-	pack_tss(&tss, (unsigned long)addr,
+	set_tssldt_descriptor(&tss, (unsigned long)addr,
 		IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1,
 		DESC_TSS);
 	write_gdt_entry(d, entry, &tss, DESC_TSS);

@@ -206,8 +185,8 @@ static inline void native_set_ldt(const void *addr, unsigned int entries)
 		unsigned cpu = smp_processor_id();
 		ldt_desc ldt;
 
-		pack_ldt(&ldt, (unsigned long)addr,
-				entries * sizeof(ldt) - 1);
+		set_tssldt_descriptor(&ldt, (unsigned long)addr,
+				      DESC_LDT, entries * sizeof(ldt) - 1);
 		write_gdt_entry(get_cpu_gdt_table(cpu), GDT_ENTRY_LDT,
 				&ldt, DESC_LDT);
 		__asm__ __volatile__("lldt %w0"::"q" (GDT_ENTRY_LDT*8));

Signature · 1 line

1.4.4.2

Ingo Molnar

Applied

2007-12-13 20:28 UTC #1 lore raw

Quoted 7 lines · Glauber de Oliveira Costa

By Andi Kleen's suggestion, this patch removes pack_ldt() and pack_tss() wrappers in favour of a general wrapper. It saves us an ifdef and some lines of code, but more importantly, it's more elegant.
No functional change is made.
Signed-off-by Glauber de Oliveira Costa <[email protected]>

thanks, applied.

Ingo

Ingo Molnar

Applied

2007-12-13 20:29 UTC #2 lore raw

In reply to Glauber de Oliveira Costa

Quoted 7 lines · Glauber de Oliveira Costa

By Andi Kleen's suggestion, this patch removes pack_ldt() and pack_tss() wrappers in favour of a general wrapper. It saves us an ifdef and some lines of code, but more importantly, it's more elegant.
No functional change is made.
Signed-off-by Glauber de Oliveira Costa <[email protected]>

thanks, applied.

Ingo

Ingo Molnar

2007-12-13 20:58 UTC #3 lore raw

just got the crash below. config attached. Suspects are:

Subject: unify struct desc_ptr
Subject: unify paravirt pieces of descriptor handling

could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?

Ingo

------------>
Enabling fast FPU save and restore... done. Enabling unmasked SIMD FPU exception support... done. Initializing CPU#0

general protection fault: 0080 [#1] SMP

Pid: 0, comm: swapper Not tainted (2.6.24-rc5 #490)
EIP: 0060:[<40118008>] EFLAGS: 00010086 CPU: 0
EIP is at native_load_tr_desc+0x8/0x10
EAX: 00000080 EBX: 00000009 ECX: 409c9efc EDX: 41e0d080
ESI: 41e0d000 EDI: 40a62000 EBP: 409c9edc ESP: 409c9edc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process swapper (pid: 0, ti=409c8000 task=4090e3e0 task.ti=409c8000)

Stack: 409c9f10 409dcde4 40850594 00000000 000000e5 00000000 409ba000 409ba000 
       77c00009 4100ebe5 00000001 409ba000 409ba000 409c9fc4 409d916d 4088a1f3 
       0060680c 4010ef00 00607400 40108f00 006074b0 40108f00 006073f4 40108f00 
Call Trace:
 [<4010796a>] show_trace_log_lvl+0x1a/0x30
 [<40107a29>] show_stack_log_lvl+0xa9/0xd0
 [<40107b05>] show_registers+0xb5/0x1a0
 [<40107d06>] die+0x116/0x220
 [<401086ac>] do_general_protection+0x19c/0x260
 [<4073b542>] error_code+0x72/0x78
 [<409dcde4>] cpu_init+0x154/0x280
 [<409d916d>] trap_init+0x3cd/0x460
 [<409d1c93>] start_kernel+0x213/0x3d0
 [<00000000>] 0x0
 =======================
Code: 90 90 90 55 89 e5 9c 5a 81 e2 ff cf ff ff 09 c2 52 9d c9 c3 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90 90 55 b8 80 00 00 00 89 e5 <0f> 00 d8 c9 c3 8d 76 00 55 89 e5 0f 01 10 c9 c3 90 8d b4 26 00 
EIP: [<40118008>] native_load_tr_desc+0x8/0x10 SS:ESP 0068:409c9edc
Kernel panic - not syncing: Attempted to kill the idle task!

Ingo Molnar

2007-12-13 21:22 UTC #4 lore raw

Quoted 4 lines · Ingo Molnar

just got the crash below. config attached. Suspects are:
Subject: unify struct desc_ptr
Subject: unify paravirt pieces of descriptor handling

i've bisected it down to:

Subject: avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss
From: Glauber de Oliveira Costa <[email protected]>

i have removed this patch from x86.git. Btw., note the advantage of small deltas, so only your v3->v4 delta patch got reverted instead of the whole 19-patch v3 kit ;-)

Quoted 3 lines

could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?

case in point, the patch claims:

Quoted 1 line

Quoted 1 line
No functional change is made.

but vmlinux before/after size comparison shows:

text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after

Ingo

Ingo Molnar

2007-12-13 21:23 UTC #5 lore raw

In reply to Ingo Molnar

Quoted 4 lines · Ingo Molnar

just got the crash below. config attached. Suspects are:
Subject: unify struct desc_ptr
Subject: unify paravirt pieces of descriptor handling

i've bisected it down to:

Subject: avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss
From: Glauber de Oliveira Costa <[email protected]>

i have removed this patch from x86.git. Btw., note the advantage of small deltas, so only your v3->v4 delta patch got reverted instead of the whole 19-patch v3 kit ;-)

Quoted 3 lines

could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?

case in point, the patch claims:

Quoted 1 line

Quoted 1 line
No functional change is made.

but vmlinux before/after size comparison shows:

text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after

Ingo

H. Peter Anvin

2007-12-13 21:26 UTC #6 lore raw

Quoted 5 lines · Ingo Molnar

Quoted 2 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:

Quoted 8 lines

Quoted 1 line
Quoted 1 line
No functional change is made.
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after

Well, "no functional changes" is not quite the same thing as "no object code changes".

-hpa

Ingo Molnar

2007-12-13 21:35 UTC #7 lore raw

Quoted 9 lines · H. Peter Anvin

Quoted 8 lines · Ingo Molnar
Quoted 3 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 1 line
Quoted 1 line
No functional change is made.

Quoted 8 lines

Quoted 5 lines
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".

yeah, true, but the safest way to ensure no functional changes is to get identical object code. In sched-devel.git i include obj comparisons to cleanup patches as a self-assurance (and later bughunt helper) to make sure a cleanup is really just a cleanup.

also, this patch, at first sight, should result in very similar (perhaps even the very same) object code. Not 1.2K larger.

Ingo

Ingo Molnar

2007-12-13 21:36 UTC #8 lore raw

In reply to H. Peter Anvin

Quoted 9 lines · H. Peter Anvin

Quoted 8 lines · Ingo Molnar
Quoted 3 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 1 line
Quoted 1 line
No functional change is made.

Quoted 8 lines

Quoted 5 lines
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".

also, this patch, at first sight, should result in very similar (perhaps even the very same) object code. Not 1.2K larger.

Ingo

H. Peter Anvin

2007-12-13 21:40 UTC #9 lore raw

Quoted 3 lines · Ingo Molnar

Quoted 1 line
Well, "no functional changes" is not quite the same thing as "no object code changes".
yeah, true, but the safest way to ensure no functional changes is to get identical object code. In sched-devel.git i include obj comparisons to cleanup patches as a self-assurance (and later bughunt helper) to make sure a cleanup is really just a cleanup.

Of course. "No object code changes" is a stronger statement, however, not all types of cleanups result in that.

It's highly useful when applied appropriately, though. In particular, the programmer should know when object code changes are expected.

-hpa

Glauber de Oliveira Costa

author

2007-12-13 21:51 UTC #10 lore raw

Quoted 7 lines · H. Peter Anvin

Quoted 3 lines · Ingo Molnar
Quoted 1 line
Well, "no functional changes" is not quite the same thing as "no object code changes".
yeah, true, but the safest way to ensure no functional changes is to get identical object code. In sched-devel.git i include obj comparisons to cleanup patches as a self-assurance (and later bughunt helper) to make sure a cleanup is really just a cleanup.
Of course. "No object code changes" is a stronger statement, however, not all types of cleanups result in that.

Which is the case here. I wasn't expecting the same object code. Anyway, the mistake is explained, and I'm about to send the correct version. Sorry again.

Glauber de Oliveira Costa

author

2007-12-13 21:52 UTC #11 lore raw

In reply to H. Peter Anvin

Quoted 7 lines · H. Peter Anvin

Quoted 3 lines · Ingo Molnar
Quoted 1 line
Well, "no functional changes" is not quite the same thing as "no object code changes".
yeah, true, but the safest way to ensure no functional changes is to get identical object code. In sched-devel.git i include obj comparisons to cleanup patches as a self-assurance (and later bughunt helper) to make sure a cleanup is really just a cleanup.
Of course. "No object code changes" is a stronger statement, however, not all types of cleanups result in that.

Which is the case here. I wasn't expecting the same object code. Anyway, the mistake is explained, and I'm about to send the correct version. Sorry again.

Tom Spink

2007-12-13 21:37 UTC #12 lore raw

In reply to H. Peter Anvin

Quoted 20 lines · H. Peter Anvin

Quoted 7 lines · Ingo Molnar
Quoted 4 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 8 lines
Quoted 1 line
Quoted 1 line
No functional change is made.
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".

Quoted 8 lines

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I'm by far no expert on the subject, but after a quick eyes-debug, have you got the size and type parameters reversed here:

+ set_tssldt_descriptor(&tss, (unsigned long)addr,
IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1, DESC_TSS);

Should DESC_TSS be the parameter before that big size equation?

Signature · 3 lines

Regards,
Tom Spink
University of Edinburgh

Tom Spink

2007-12-13 21:37 UTC #13 lore raw

In reply to H. Peter Anvin

Quoted 20 lines · H. Peter Anvin

Quoted 7 lines · Ingo Molnar
Quoted 4 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 8 lines
Quoted 1 line
Quoted 1 line
No functional change is made.
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".

Quoted 8 lines

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I'm by far no expert on the subject, but after a quick eyes-debug, have you got the size and type parameters reversed here:

+ set_tssldt_descriptor(&tss, (unsigned long)addr,
IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1, DESC_TSS);

Should DESC_TSS be the parameter before that big size equation?

Signature · 3 lines

Regards,
Tom Spink
University of Edinburgh

Glauber de Oliveira Costa

author

2007-12-13 21:40 UTC #14 lore raw

Quoted 26 lines · Tom Spink

Quoted 16 lines · H. Peter Anvin
Quoted 12 lines · Ingo Molnar
Quoted 3 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 1 line
Quoted 1 line
No functional change is made.
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".
Quoted 8 lines
-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines

I'm by far no expert on the subject, but after a quick eyes-debug, have you got the size and type parameters reversed here:
+ set_tssldt_descriptor(&tss, (unsigned long)addr,
IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1, DESC_TSS);
Should DESC_TSS be the parameter before that big size equation?

That's it. I made this mistake once, and fixed it. But I may have sent an older, and obviously wrong version of the patch. My fault.

Glauber de Oliveira Costa

author

2007-12-13 21:41 UTC #15 lore raw

In reply to Tom Spink

Quoted 26 lines · Tom Spink

Quoted 16 lines · H. Peter Anvin
Quoted 12 lines · Ingo Molnar
Quoted 3 lines
could we _please_ use basic tools as vmlinux size comparison and objcompare when unifying, to make sure it's a 100% identity conversion?
case in point, the patch claims:
Quoted 1 line
Quoted 1 line
No functional change is made.
but vmlinux before/after size comparison shows:
text data bss dec hex filename
8802894 1224910 3526656 13554460 ced31c vmlinux.before
8803042 1224910 3526656 13554608 ced3b0 vmlinux.after
Well, "no functional changes" is not quite the same thing as "no object code changes".
Quoted 8 lines
-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines

I'm by far no expert on the subject, but after a quick eyes-debug, have you got the size and type parameters reversed here:
+ set_tssldt_descriptor(&tss, (unsigned long)addr,
IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1, DESC_TSS);
Should DESC_TSS be the parameter before that big size equation?

That's it. I made this mistake once, and fixed it. But I may have sent an older, and obviously wrong version of the patch. My fault.