Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Ingo Molnar]

* Glauber de Oliveira Costa <gcosta@redhat.com> wrote:

> 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 <gcosta@redhat.com>

thanks, applied.

	Ingo

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Ingo Molnar]

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!

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 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 <gcosta@redhat.com>

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

> 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:

> > 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

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[H. Peter Anvin]

Ingo Molnar wrote:
> 
>> 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:
> 
>>> 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

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Ingo Molnar wrote:
>>
>>> 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:
>>
>>>> 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".

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

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Tom Spink]

On Dec 13, 2007 9:26 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> Ingo Molnar wrote:
> >
> >> 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:
> >
> >>> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> 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?

-- 
Regards,
Tom Spink
University of Edinburgh

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Glauber de Oliveira Costa]

Tom Spink wrote:
> On Dec 13, 2007 9:26 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> Ingo Molnar wrote:
>>>> 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:
>>>
>>>>> 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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> 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?
> 
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.

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[H. Peter Anvin]

Ingo Molnar wrote:
>> 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

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Glauber de Oliveira Costa]

H. Peter Anvin wrote:
> Ingo Molnar wrote:
>>> 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.

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

[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, although the object code is expected to be
different.

Signed-off-by: Glauber de Oliveira Costa <gcosta@redhat.com>
---
 include/asm-x86/desc.h |   58 +++++++++++++++---------------------------------
 1 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/include/asm-x86/desc.h b/include/asm-x86/desc.h
index 8849666..161a6d6 100644
--- a/include/asm-x86/desc.h
+++ b/include/asm-x86/desc.h
@@ -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,9 +169,8 @@ 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,
-		IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1,
-		DESC_TSS);
+	set_tssldt_descriptor(&tss, (unsigned long)addr, DESC_TSS,
+		IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1);
 	write_gdt_entry(d, entry, &tss, DESC_TSS);
 }
 
@@ -206,8 +184,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));
-- 
1.4.4.2

Re: [PATCH] avoid ifdefs in desc.h, getting rid of pack_ldt and pack_tss

[Ingo Molnar]

* Glauber de Oliveira Costa <gcosta@redhat.com> wrote:

> 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, although the object code is expected to 
> be different.

thanks, applied.

	Ingo

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

[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 <gcosta@redhat.com>
---
 include/asm-x86/desc.h |   55 ++++++++++++++---------------------------------
 1 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/include/asm-x86/desc.h b/include/asm-x86/desc.h
index 8849666..e302a02 100644
--- a/include/asm-x86/desc.h
+++ b/include/asm-x86/desc.h
@@ -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));
-- 
1.4.4.2