[PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Joe Damato]

Hi -

This is my first submission to the kernel, so (beware!) please let me know if I can make any improvements on these patches.

I attempted to clean up the x86 structs for 32bit cpus that store IDT/LDT/GDT data by removing the fields labeled "a" and "b" in favor of more descriptive field names. I added some macros and went through the kernel cleaning up the various places where "a" and "b" were used.

I tried building my kernel with my .config and then also did a make allyesconfig build to help ensure I found everything that was using the old structure names. I also tried a few grep patterns. Hopefully I got everyone out.


Thanks,
Joe Damato

[PATCH 01/12] x86: Cleanup x86 descriptors, remove a/b fields from structs

[Joe Damato]

Split the single descriptor struct into an IDT struct and a struct for ldt/gdt/tss. This was done because the fields in IDTs are not the same or in the same order as ldt/gdt/tss descriptors. More meaningful field names were added and the a/b fields were removed.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc_defs.h |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
index b881db6..68abda4 100644
--- a/include/asm-x86/desc_defs.h
+++ b/include/asm-x86/desc_defs.h
@@ -11,27 +11,21 @@
 
 #include <linux/types.h>
 
-/*
- * FIXME: Acessing the desc_struct through its fields is more elegant,
- * and should be the one valid thing to do. However, a lot of open code
- * still touches the a and b acessors, and doing this allow us to do it
- * incrementally. We keep the signature as a struct, rather than an union,
- * so we can get rid of it transparently in the future -- glommer
- */
+/* 8 byte idt gate descriptor */
+struct gate_struct {
+	u16 base0;
+	u16 seg_sel;
+	u8 reserved;
+	unsigned type: 4, zero: 1, dpl: 2, p: 1;
+	u16 base1;
+} __attribute__((packed));
+
 /* 8 byte segment descriptor */
 struct desc_struct {
-	union {
-		struct {
-			unsigned int a;
-			unsigned int b;
-		};
-		struct {
-			u16 limit0;
-			u16 base0;
-			unsigned base1: 8, type: 4, s: 1, dpl: 2, p: 1;
-			unsigned limit: 4, avl: 1, l: 1, d: 1, g: 1, base2: 8;
-		};
-	};
+	u16 limit0;
+	u16 base0;
+	unsigned base1: 8, type: 4, s: 1, dpl: 2, p: 1;
+	unsigned limit: 4, avl: 1, l: 1, d: 1, g: 1, base2: 8;
 } __attribute__((packed));
 
 enum {
-- 
1.5.4.3

[PATCH 02/12] x86: Use new gate_struct for gate_desc

[Joe Damato]

Change the gate_desc typedef to use the new gate_struct.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc_defs.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
index 68abda4..012df1f 100644
--- a/include/asm-x86/desc_defs.h
+++ b/include/asm-x86/desc_defs.h
@@ -72,7 +72,7 @@ typedef struct ldttss_desc64 tss_desc;
 #define gate_offset(g) ((g).offset_low | ((unsigned long)(g).offset_middle << 16) | ((unsigned long)(g).offset_high << 32))
 #define gate_segment(g) ((g).segment)
 #else
-typedef struct desc_struct gate_desc;
+typedef struct gate_struct gate_desc;
 typedef struct desc_struct ldt_desc;
 typedef struct desc_struct tss_desc;
 #define gate_offset(g)		(((g).b & 0xffff0000) | ((g).a & 0x0000ffff))
-- 
1.5.4.3

Re: [PATCH 02/12] x86: Use new gate_struct for gate_desc

[Jeremy Fitzhardinge]

Joe Damato wrote:
> Change the gate_desc typedef to use the new gate_struct.
>   

Blerk, yes.  I'd really prefer these typedefs to use the _t convention.

    J

[PATCH 03/12] x86: Cleanup usage of struct desc_struct

[Joe Damato]

Use gate_desc typedef for IDT entries instead of struct desc_struct.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 arch/x86/kernel/vmi_32.c              |    2 +-
 drivers/lguest/interrupts_and_traps.c |   12 ++++++------
 drivers/lguest/lg.h                   |    2 +-
 include/asm-x86/lguest.h              |    4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/vmi_32.c b/arch/x86/kernel/vmi_32.c
index 8b6c393..8c87b9b 100644
--- a/arch/x86/kernel/vmi_32.c
+++ b/arch/x86/kernel/vmi_32.c
@@ -63,7 +63,7 @@ static struct {
 	void (*cpuid)(void /* non-c */);
 	void (*_set_ldt)(u32 selector);
 	void (*set_tr)(u32 selector);
-	void (*write_idt_entry)(struct desc_struct *, int, u32, u32);
+	void (*write_idt_entry)(gate_desc *, int, u32, u32);
 	void (*write_gdt_entry)(struct desc_struct *, int, u32, u32);
 	void (*write_ldt_entry)(struct desc_struct *, int, u32, u32);
 	void (*set_kernel_stack)(u32 selector, u32 sp0);
diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
index a103906..48aa15b 100644
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -133,7 +133,7 @@ void maybe_do_interrupt(struct lg_cpu *cpu)
 {
 	unsigned int irq;
 	DECLARE_BITMAP(blk, LGUEST_IRQS);
-	struct desc_struct *idt;
+	gate_desc *idt;
 
 	/* If the Guest hasn't even initialized yet, we can do nothing. */
 	if (!cpu->lg->lguest_data)
@@ -353,7 +353,7 @@ void guest_set_stack(struct lg_cpu *cpu, u32 seg, u32 esp, unsigned int pages)
 
 /*H:235 This is the routine which actually checks the Guest's IDT entry and
  * transfers it into the entry in "struct lguest": */
-static void set_trap(struct lg_cpu *cpu, struct desc_struct *trap,
+static void set_trap(struct lg_cpu *cpu, gate_desc *trap,
 		     unsigned int num, u32 lo, u32 hi)
 {
 	u8 type = idt_type(lo, hi);
@@ -404,10 +404,10 @@ void load_guest_idt_entry(struct lg_cpu *cpu, unsigned int num, u32 lo, u32 hi)
 /* The default entry for each interrupt points into the Switcher routines which
  * simply return to the Host.  The run_guest() loop will then call
  * deliver_trap() to bounce it back into the Guest. */
-static void default_idt_entry(struct desc_struct *idt,
+static void default_idt_entry(gate_desc *idt,
 			      int trap,
 			      const unsigned long handler,
-			      const struct desc_struct *base)
+			      const gate_desc *base)
 {
 	/* A present interrupt gate. */
 	u32 flags = 0x8e00;
@@ -439,7 +439,7 @@ void setup_default_idt_entries(struct lguest_ro_state *state,
 /*H:240 We don't use the IDT entries in the "struct lguest" directly, instead
  * we copy them into the IDT which we've set up for Guests on this CPU, just
  * before we run the Guest.  This routine does that copy. */
-void copy_traps(const struct lg_cpu *cpu, struct desc_struct *idt,
+void copy_traps(const struct lg_cpu *cpu, gate_desc *idt,
 		const unsigned long *def)
 {
 	unsigned int i;
@@ -447,7 +447,7 @@ void copy_traps(const struct lg_cpu *cpu, struct desc_struct *idt,
 	/* We can simply copy the direct traps, otherwise we use the default
 	 * ones in the Switcher: they will return to the Host. */
 	for (i = 0; i < ARRAY_SIZE(cpu->arch.idt); i++) {
-		const struct desc_struct *gidt = &cpu->arch.idt[i];
+		const gate_desc *gidt = &cpu->arch.idt[i];
 
 		/* If no Guest can ever override this trap, leave it alone. */
 		if (!direct_trap(i))
diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
index 5faefea..4377488 100644
--- a/drivers/lguest/lg.h
+++ b/drivers/lguest/lg.h
@@ -147,7 +147,7 @@ void guest_set_stack(struct lg_cpu *cpu, u32 seg, u32 esp, unsigned int pages);
 void pin_stack_pages(struct lg_cpu *cpu);
 void setup_default_idt_entries(struct lguest_ro_state *state,
 			       const unsigned long *def);
-void copy_traps(const struct lg_cpu *cpu, struct desc_struct *idt,
+void copy_traps(const struct lg_cpu *cpu, gate_desc *idt,
 		const unsigned long *def);
 void guest_set_clockevent(struct lg_cpu *cpu, unsigned long delta);
 void init_clockdev(struct lg_cpu *cpu);
diff --git a/include/asm-x86/lguest.h b/include/asm-x86/lguest.h
index 7505e94..9ccf6f4 100644
--- a/include/asm-x86/lguest.h
+++ b/include/asm-x86/lguest.h
@@ -61,7 +61,7 @@ struct lguest_ro_state {
 	struct desc_ptr guest_idt_desc;
 	struct desc_ptr guest_gdt_desc;
 	struct x86_hw_tss guest_tss;
-	struct desc_struct guest_idt[IDT_ENTRIES];
+	gate_desc guest_idt[IDT_ENTRIES];
 	struct desc_struct guest_gdt[GDT_ENTRIES];
 };
 
@@ -70,7 +70,7 @@ struct lg_cpu_arch {
 	struct desc_struct gdt[GDT_ENTRIES];
 
 	/* The IDT entries: some copied into lguest_ro_state when running. */
-	struct desc_struct idt[IDT_ENTRIES];
+	gate_desc idt[IDT_ENTRIES];
 
 	/* The address of the last guest-visible pagefault (ie. cr2). */
 	unsigned long last_pagefault;
-- 
1.5.4.3

Re: [PATCH 03/12] x86: Cleanup usage of struct desc_struct

[Jeremy Fitzhardinge]

Joe Damato wrote:
> Use gate_desc typedef for IDT entries instead of struct desc_struct.
>   

Why's that?  In general we try to avoid typedefs unless they're really 
necessary.  If it is necessary, it should be named gate_desc_t.

    J

[PATCH 04/12] x86: Add macros for gate_desc

[Joe Damato]

Add useful macros which can be used to access the lo and hi 32bit words, get the offset of the handler, and get the segment for gate_descs.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc_defs.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
index 012df1f..d6a5310 100644
--- a/include/asm-x86/desc_defs.h
+++ b/include/asm-x86/desc_defs.h
@@ -75,8 +75,13 @@ typedef struct ldttss_desc64 tss_desc;
 typedef struct gate_struct gate_desc;
 typedef struct desc_struct ldt_desc;
 typedef struct desc_struct tss_desc;
-#define gate_offset(g)		(((g).b & 0xffff0000) | ((g).a & 0x0000ffff))
-#define gate_segment(g)		((g).a >> 16)
+#define desc_lo(d)		(((u32 *)&d)[0])
+#define desc_hi(d)		(((u32 *)&d)[1])
+#define gate_offset(g)		(((g).base1 << 16) | ((g).base0 & 0x0000ffff))
+#define gate_segment(g)		((g).seg_sel >> 16)
+#define ldttss_offset(d)	(((d).base2 << 24 ) | ((d).base1 << 16) |\
+				 ((d).base0 & 0x0000ffff))
+#define ldttss_limit(d)		((d).limit0)
 #endif
 
 struct desc_ptr {
-- 
1.5.4.3

Re: [PATCH 04/12] x86: Add macros for gate_desc

[Jeremy Fitzhardinge]

Joe Damato wrote:
> Add useful macros which can be used to access the lo and hi 32bit words, get the offset of the handler, and get the segment for gate_descs.
>   

Can you make them inline functions?

    J

[PATCH 05/12] x86: Refactor pack_gate for gate_desc

[Joe Damato]

Rewrite pack_gate to use the newly add struct fields instead of bitmasks.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/asm-x86/desc.h b/include/asm-x86/desc.h
index f06adac..6b4f3e6 100644
--- a/include/asm-x86/desc.h
+++ b/include/asm-x86/desc.h
@@ -66,9 +66,14 @@ static inline void pack_gate(gate_desc *gate, unsigned char type,
 			     unsigned long base, unsigned dpl, unsigned flags,
 			     unsigned short seg)
 {
-	gate->a = (seg << 16) | (base & 0xffff);
-	gate->b = (base & 0xffff0000) |
-		  (((0x80 | type | (dpl << 5)) & 0xff) << 8);
+	gate->base0 = base & 0xffff;
+	gate->seg_sel = seg;
+	gate->reserved = 0;
+	gate->type = type;
+	gate->zero = 0;
+	gate->dpl = dpl;
+	gate->p = 1;
+	gate->base1 = (base >> 16) & 0xffff;
 }
 
 #endif
-- 
1.5.4.3

[PATCH 06/12] x86: Refactor pack_descriptor

[Joe Damato]

Refactor pack_descriptor to use fields in the struct instead of bitmasks

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc.h |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/asm-x86/desc.h b/include/asm-x86/desc.h
index 6b4f3e6..ac9aad5 100644
--- a/include/asm-x86/desc.h
+++ b/include/asm-x86/desc.h
@@ -151,11 +151,19 @@ static inline void pack_descriptor(struct desc_struct *desc, unsigned long base,
 				   unsigned long limit, unsigned char type,
 				   unsigned char flags)
 {
-	desc->a = ((base & 0xffff) << 16) | (limit & 0xffff);
-	desc->b = (base & 0xff000000) | ((base & 0xff0000) >> 16) |
-		(limit & 0x000f0000) | ((type & 0xff) << 8) |
-		((flags & 0xf) << 20);
-	desc->p = 1;
+	desc->limit0 = limit & 0xffff;
+	desc->base0  = base  & 0xffff;
+	desc->base1  = (base  >> 16) & 0xff;
+	desc->base2  = (base  >> 24) & 0xff;
+	desc->type   = type;
+	desc->s      = 0;
+	desc->dpl    = 0;
+	desc->p      = 1;
+	desc->limit  = (limit >> 16) & 0xf;
+	desc->avl    = flags & 0x1;
+	desc->l      = (flags & 0x2) >> 1;
+	desc->d      = (flags & 0x4) >> 2;
+	desc->g      = (flags & 0x8) >> 3;
 }
 
 
-- 
1.5.4.3

[PATCH 07/12] x86: Add a static initializer for IDTs

[Joe Damato]

Add static intializer for IDT entries.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc_defs.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
index d6a5310..a1c6516 100644
--- a/include/asm-x86/desc_defs.h
+++ b/include/asm-x86/desc_defs.h
@@ -82,6 +82,15 @@ typedef struct desc_struct tss_desc;
 #define ldttss_offset(d)	(((d).base2 << 24 ) | ((d).base1 << 16) |\
 				 ((d).base0 & 0x0000ffff))
 #define ldttss_limit(d)		((d).limit0)
+#define __IDT_INITIALIZER(lo,hi) \
+                { .base0 = lo & 0xffff \
+                , .seg_sel = (lo >> 16) & 0xffff \
+                , .reserved = 0 \
+                , .type = (hi >> 8) & 0xf \
+                , .zero = 0 \
+                , .dpl = (hi >> 13) & 3 \
+                , .p   = (hi >> 15) & 1 \
+                , .base1 = (hi >> 16) & 0xffff }
 #endif
 
 struct desc_ptr {
-- 
1.5.4.3

[PATCH 08/12] x86: Use static intializer for IDT entries

[Joe Damato]

Use static intializer for IDT entries.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 arch/x86/kernel/traps.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e062974..b308d71 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -86,7 +86,7 @@ char ignore_fpu_irq;
  * for this.
  */
 gate_desc idt_table[256]
-	__attribute__((__section__(".data.idt"))) = { { { { 0, 0 } } }, };
+	__attribute__((__section__(".data.idt"))) = { __IDT_INITIALIZER(0, 0) };
 #endif
 
 static int ignore_nmis;
-- 
1.5.4.3

[PATCH 09/12] x86: Add static initiazlier for descriptors

[Joe Damato]

Add static initializer for LDT/GDT/TSS descriptors.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 include/asm-x86/desc_defs.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
index a1c6516..5119051 100644
--- a/include/asm-x86/desc_defs.h
+++ b/include/asm-x86/desc_defs.h
@@ -91,6 +91,20 @@ typedef struct desc_struct tss_desc;
                 , .dpl = (hi >> 13) & 3 \
                 , .p   = (hi >> 15) & 1 \
                 , .base1 = (hi >> 16) & 0xffff }
+#define __DESC_INITIALIZER(lo,hi) \
+                { .limit0 = lo & 0xffff \
+                , .base0  = (lo >> 16) & 0xffff \
+                , .base1  = hi & 0xff \
+                , .type   = (hi >> 8) & 0xf \
+                , .s      = (hi >> 12) & 1 \
+                , .dpl    = (hi >> 13) & 3 \
+                , .p      = (hi >> 15) & 1 \
+                , .limit  = (hi >> 16) & 0xf \
+                , .avl    = (hi >> 20) & 1 \
+                , .l      = (hi >> 21) & 1 \
+                , .d      = (hi >> 22) & 1 \
+                , .g      = (hi >> 23) & 1 \
+                ,.base2   = (hi >> 24) & 0xf }
 #endif
 
 struct desc_ptr {
-- 
1.5.4.3

Re: [PATCH 09/12] x86: Add static initiazlier for descriptors

[Jeremy Fitzhardinge]

Joe Damato wrote:
> Add static initializer for LDT/GDT/TSS descriptors.
>
> Signed-off-by: Joe Damato <ice799@gmail.com>
> ---
>  include/asm-x86/desc_defs.h |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/include/asm-x86/desc_defs.h b/include/asm-x86/desc_defs.h
> index a1c6516..5119051 100644
> --- a/include/asm-x86/desc_defs.h
> +++ b/include/asm-x86/desc_defs.h
> @@ -91,6 +91,20 @@ typedef struct desc_struct tss_desc;
>                  , .dpl = (hi >> 13) & 3 \
>                  , .p   = (hi >> 15) & 1 \
>                  , .base1 = (hi >> 16) & 0xffff }
> +#define __DESC_INITIALIZER(lo,hi) \
> +                { .limit0 = lo & 0xffff \
> +                , .base0  = (lo >> 16) & 0xffff \
> +                , .base1  = hi & 0xff \
> +                , .type   = (hi >> 8) & 0xf \
> +                , .s      = (hi >> 12) & 1 \
> +                , .dpl    = (hi >> 13) & 3 \
> +                , .p      = (hi >> 15) & 1 \
> +                , .limit  = (hi >> 16) & 0xf \
> +                , .avl    = (hi >> 20) & 1 \
> +                , .l      = (hi >> 21) & 1 \
> +                , .d      = (hi >> 22) & 1 \
> +                , .g      = (hi >> 23) & 1 \
> +                ,.base2   = (hi >> 24) & 0xf }
>   

Reasonable as a transition, I guess, but it would be better to just make 
the initializers pass in proper values.  But I think it would be better 
to implement this with a union rather than repacking the packed values.

    J

[PATCH 10/12] x86: Use static initializers for descriptors

[Joe Damato]

Use static intializers for GDT/LDT entries.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 arch/x86/kernel/apm_32.c     |    2 +-
 arch/x86/kernel/cpu/common.c |   28 ++++++++++++++--------------
 include/asm-x86/lguest.h     |    4 ++--
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c
index 5145a6e..5cacdad 100644
--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c
@@ -407,7 +407,7 @@ static DECLARE_WAIT_QUEUE_HEAD(apm_waitqueue);
 static DECLARE_WAIT_QUEUE_HEAD(apm_suspend_waitqueue);
 static struct apm_user *user_list;
 static DEFINE_SPINLOCK(user_list_lock);
-static const struct desc_struct	bad_bios_desc = { { { 0, 0x00409200 } } };
+static const struct desc_struct	bad_bios_desc = __DESC_INITIALIZER(0, 0x00409200);
 
 static const char driver_version[] = "1.16ac";	/* no spaces */
 
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 25581dc..018e1ba 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -57,38 +57,38 @@ DEFINE_PER_CPU(struct gdt_page, gdt_page) = { .gdt = {
 } };
 #else
 DEFINE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page) = { .gdt = {
-	[GDT_ENTRY_KERNEL_CS] = { { { 0x0000ffff, 0x00cf9a00 } } },
-	[GDT_ENTRY_KERNEL_DS] = { { { 0x0000ffff, 0x00cf9200 } } },
-	[GDT_ENTRY_DEFAULT_USER_CS] = { { { 0x0000ffff, 0x00cffa00 } } },
-	[GDT_ENTRY_DEFAULT_USER_DS] = { { { 0x0000ffff, 0x00cff200 } } },
+	[GDT_ENTRY_KERNEL_CS] =  __DESC_INITIALIZER(0x0000ffff, 0x00cf9a00),
+	[GDT_ENTRY_KERNEL_DS] =  __DESC_INITIALIZER(0x0000ffff, 0x00cf9200),
+	[GDT_ENTRY_DEFAULT_USER_CS] = __DESC_INITIALIZER(0x0000ffff, 0x00cffa00),
+	[GDT_ENTRY_DEFAULT_USER_DS] = __DESC_INITIALIZER(0x0000ffff, 0x00cff200),
 	/*
 	 * Segments used for calling PnP BIOS have byte granularity.
 	 * They code segments and data segments have fixed 64k limits,
 	 * the transfer segment sizes are set at run time.
 	 */
 	/* 32-bit code */
-	[GDT_ENTRY_PNPBIOS_CS32] = { { { 0x0000ffff, 0x00409a00 } } },
+	[GDT_ENTRY_PNPBIOS_CS32] = __DESC_INITIALIZER(0x0000ffff, 0x00409a00),
 	/* 16-bit code */
-	[GDT_ENTRY_PNPBIOS_CS16] = { { { 0x0000ffff, 0x00009a00 } } },
+	[GDT_ENTRY_PNPBIOS_CS16] = __DESC_INITIALIZER(0x0000ffff, 0x00009a00),
 	/* 16-bit data */
-	[GDT_ENTRY_PNPBIOS_DS] = { { { 0x0000ffff, 0x00009200 } } },
+	[GDT_ENTRY_PNPBIOS_DS] = __DESC_INITIALIZER(0x0000ffff, 0x00009200),
 	/* 16-bit data */
-	[GDT_ENTRY_PNPBIOS_TS1] = { { { 0x00000000, 0x00009200 } } },
+	[GDT_ENTRY_PNPBIOS_TS1] = __DESC_INITIALIZER(0x00000000, 0x00009200),
 	/* 16-bit data */
-	[GDT_ENTRY_PNPBIOS_TS2] = { { { 0x00000000, 0x00009200 } } },
+	[GDT_ENTRY_PNPBIOS_TS2] = __DESC_INITIALIZER(0x00000000, 0x00009200),
 	/*
 	 * The APM segments have byte granularity and their bases
 	 * are set at run time.  All have 64k limits.
 	 */
 	/* 32-bit code */
-	[GDT_ENTRY_APMBIOS_BASE] = { { { 0x0000ffff, 0x00409a00 } } },
+	[GDT_ENTRY_APMBIOS_BASE] = __DESC_INITIALIZER(0x0000ffff, 0x00409a00),
 	/* 16-bit code */
-	[GDT_ENTRY_APMBIOS_BASE+1] = { { { 0x0000ffff, 0x00009a00 } } },
+	[GDT_ENTRY_APMBIOS_BASE+1] = __DESC_INITIALIZER(0x0000ffff, 0x00009a00),
 	/* data */
-	[GDT_ENTRY_APMBIOS_BASE+2] = { { { 0x0000ffff, 0x00409200 } } },
+	[GDT_ENTRY_APMBIOS_BASE+2] = __DESC_INITIALIZER(0x0000ffff, 0x00409200),
 
-	[GDT_ENTRY_ESPFIX_SS] = { { { 0x00000000, 0x00c09200 } } },
-	[GDT_ENTRY_PERCPU] = { { { 0x00000000, 0x00000000 } } },
+	[GDT_ENTRY_ESPFIX_SS] = __DESC_INITIALIZER(0x00000000, 0x00c09200),
+	[GDT_ENTRY_PERCPU] = __DESC_INITIALIZER(0x00000000, 0x00000000),
 } };
 #endif
 EXPORT_PER_CPU_SYMBOL_GPL(gdt_page);
diff --git a/include/asm-x86/lguest.h b/include/asm-x86/lguest.h
index 9ccf6f4..f766b6c 100644
--- a/include/asm-x86/lguest.h
+++ b/include/asm-x86/lguest.h
@@ -86,8 +86,8 @@ static inline void lguest_set_ts(void)
 }
 
 /* Full 4G segment descriptors, suitable for CS and DS. */
-#define FULL_EXEC_SEGMENT ((struct desc_struct){ { {0x0000ffff, 0x00cf9b00} } })
-#define FULL_SEGMENT ((struct desc_struct){ { {0x0000ffff, 0x00cf9300} } })
+#define FULL_EXEC_SEGMENT ((struct desc_struct) __DESC_INITIALIZER(0x0000ffff, 0x00cf9b00))
+#define FULL_SEGMENT ((struct desc_struct) __DESC_INITIALIZER(0x0000ffff, 0x00cf9300))
 
 #endif /* __ASSEMBLY__ */
 
-- 
1.5.4.3

[PATCH 11/12] x86: Use macros for getting/setting descriptors

[Joe Damato]

Use macros for getting/setting the hi and lo 32bits of descriptors since 'a' and 'b' were removed.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 arch/x86/kernel/tls.c                 |    2 +-
 arch/x86/kernel/vmi_32.c              |    3 ++-
 arch/x86/lguest/boot.c                |    2 +-
 arch/x86/math-emu/fpu_system.h        |   19 +++++++++----------
 drivers/lguest/interrupts_and_traps.c |   18 +++++++++---------
 drivers/lguest/segments.c             |   14 +++++++-------
 drivers/pnp/pnpbios/bioscalls.c       |    4 ++--
 include/asm-x86/xen/hypercall.h       |    4 ++--
 8 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
index 6bb7b85..653c995 100644
--- a/arch/x86/kernel/tls.c
+++ b/arch/x86/kernel/tls.c
@@ -42,7 +42,7 @@ static void set_tls_desc(struct task_struct *p, int idx,
 
 	while (n-- > 0) {
 		if (LDT_empty(info))
-			desc->a = desc->b = 0;
+			desc_hi(desc) = desc_lo(desc) = 0;
 		else
 			fill_ldt(desc, info);
 		++info;
diff --git a/arch/x86/kernel/vmi_32.c b/arch/x86/kernel/vmi_32.c
index 8c87b9b..939363b 100644
--- a/arch/x86/kernel/vmi_32.c
+++ b/arch/x86/kernel/vmi_32.c
@@ -189,7 +189,8 @@ static void vmi_cpuid(unsigned int *ax, unsigned int *bx,
 
 static inline void vmi_maybe_load_tls(struct desc_struct *gdt, int nr, struct desc_struct *new)
 {
-	if (gdt[nr].a != new->a || gdt[nr].b != new->b)
+	if (desc_hi(gdt[nr]) != desc_hi(*new) ||
+	    desc_lo(gdt[nr]) != desc_lo(*new))
 		write_gdt_entry(gdt, nr, new, 0);
 }
 
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 48ee4f9..a2391d8 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -236,7 +236,7 @@ static void lguest_load_idt(const struct desc_ptr *desc)
 	struct desc_struct *idt = (void *)desc->address;
 
 	for (i = 0; i < (desc->size+1)/8; i++)
-		hcall(LHCALL_LOAD_IDT_ENTRY, i, idt[i].a, idt[i].b);
+		hcall(LHCALL_LOAD_IDT_ENTRY, i, desc_lo(idt[i]), desc_hi(idt[i]));
 }
 
 /*
diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index 13488fa..3fb2237 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -23,16 +23,15 @@
 /* s is always from a cpu register, and the cpu does bounds checking
  * during register load --> no further bounds checks needed */
 #define LDT_DESCRIPTOR(s)	(((struct desc_struct *)current->mm->context.ldt)[(s) >> 3])
-#define SEG_D_SIZE(x)		((x).b & (3 << 21))
-#define SEG_G_BIT(x)		((x).b & (1 << 23))
-#define SEG_GRANULARITY(x)	(((x).b & (1 << 23)) ? 4096 : 1)
-#define SEG_286_MODE(x)		((x).b & ( 0xff000000 | 0xf0000 | (1 << 23)))
-#define SEG_BASE_ADDR(s)	(((s).b & 0xff000000) \
-				 | (((s).b & 0xff) << 16) | ((s).a >> 16))
-#define SEG_LIMIT(s)		(((s).b & 0xff0000) | ((s).a & 0xffff))
-#define SEG_EXECUTE_ONLY(s)	(((s).b & ((1 << 11) | (1 << 9))) == (1 << 11))
-#define SEG_WRITE_PERM(s)	(((s).b & ((1 << 11) | (1 << 9))) == (1 << 9))
-#define SEG_EXPAND_DOWN(s)	(((s).b & ((1 << 11) | (1 << 10))) \
+#define SEG_D_SIZE(x)		(desc_hi(x) & (3 << 21))
+#define SEG_G_BIT(x)		(desc_hi(x) & (1 << 23))
+#define SEG_GRANULARITY(x)	((desc_hi(x) & (1 << 23)) ? 4096 : 1)
+#define SEG_286_MODE(x)		(desc_hi(x) & ( 0xff000000 | 0xf0000 | (1 << 23)))
+#define SEG_BASE_ADDR(s)	(ldttss_offset(s))
+#define SEG_LIMIT(s)		(ldttss_limit(s))
+#define SEG_EXECUTE_ONLY(s)	((desc_hi(s) & ((1 << 11) | (1 << 9))) == (1 << 11))
+#define SEG_WRITE_PERM(s)	((desc_hi(s) & ((1 << 11) | (1 << 9))) == (1 << 9))
+#define SEG_EXPAND_DOWN(s)	((desc_hi(s) & ((1 << 11) | (1 << 10))) \
 				 == (1 << 10))
 
 #define I387			(current->thread.xstate)
diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
index 48aa15b..bfb24d9 100644
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -184,7 +184,7 @@ void maybe_do_interrupt(struct lg_cpu *cpu)
 		/* set_guest_interrupt() takes the interrupt descriptor and a
 		 * flag to say whether this interrupt pushes an error code onto
 		 * the stack as well: virtual interrupts never do. */
-		set_guest_interrupt(cpu, idt->a, idt->b, 0);
+		set_guest_interrupt(cpu, desc_lo(idt), desc_hi(idt), 0);
 	}
 
 	/* Every time we deliver an interrupt, we update the timestamp in the
@@ -256,8 +256,8 @@ int deliver_trap(struct lg_cpu *cpu, unsigned int num)
 	 * bogus one in): if we fail here, the Guest will be killed. */
 	if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))
 		return 0;
-	set_guest_interrupt(cpu, cpu->arch.idt[num].a,
-			    cpu->arch.idt[num].b, has_err(num));
+	set_guest_interrupt(cpu, desc_lo(cpu->arch.idt[num]),
+			    desc_hi(cpu->arch.idt[num]), has_err(num));
 	return 1;
 }
 
@@ -360,7 +360,7 @@ static void set_trap(struct lg_cpu *cpu, gate_desc *trap,
 
 	/* We zero-out a not-present entry */
 	if (!idt_present(lo, hi)) {
-		trap->a = trap->b = 0;
+		desc_hi(trap) = desc_lo(trap) = 0;
 		return;
 	}
 
@@ -372,8 +372,8 @@ static void set_trap(struct lg_cpu *cpu, gate_desc *trap,
 	 * type.  The privilege level controls where the trap can be triggered
 	 * manually with an "int" instruction.  This is usually GUEST_PL,
 	 * except for system calls which userspace can use. */
-	trap->a = ((__KERNEL_CS|GUEST_PL)<<16) | (lo&0x0000FFFF);
-	trap->b = (hi&0xFFFFEF00);
+	desc_lo(trap) = ((__KERNEL_CS|GUEST_PL)<<16) | (lo&0x0000FFFF);
+	desc_hi(trap) = (hi&0xFFFFEF00);
 }
 
 /*H:230 While we're here, dealing with delivering traps and interrupts to the
@@ -419,11 +419,11 @@ static void default_idt_entry(gate_desc *idt,
 	else if (base)
 		/* Copy priv. level from what Guest asked for.  This allows
 		 * debug (int 3) traps from Guest userspace, for example. */
-		flags |= (base->b & 0x6000);
+		flags |= (desc_hi(base) & 0x6000);
 
 	/* Now pack it into the IDT entry in its weird format. */
-	idt->a = (LGUEST_CS<<16) | (handler&0x0000FFFF);
-	idt->b = (handler&0xFFFF0000) | flags;
+	desc_lo(idt) = (LGUEST_CS<<16) | (handler&0x0000FFFF);
+	desc_hi(idt) = (handler&0xFFFF0000) | flags;
 }
 
 /* When the Guest first starts, we put default entries into the IDT. */
diff --git a/drivers/lguest/segments.c b/drivers/lguest/segments.c
index ec6aa3f..8506880 100644
--- a/drivers/lguest/segments.c
+++ b/drivers/lguest/segments.c
@@ -71,14 +71,14 @@ static void fixup_gdt_table(struct lg_cpu *cpu, unsigned start, unsigned end)
 		/* Segment descriptors contain a privilege level: the Guest is
 		 * sometimes careless and leaves this as 0, even though it's
 		 * running at privilege level 1.  If so, we fix it here. */
-		if ((cpu->arch.gdt[i].b & 0x00006000) == 0)
-			cpu->arch.gdt[i].b |= (GUEST_PL << 13);
+		if ((desc_hi(cpu->arch.gdt[i]) & 0x00006000) == 0)
+			desc_hi(cpu->arch.gdt[i]) |= (GUEST_PL << 13);
 
 		/* Each descriptor has an "accessed" bit.  If we don't set it
 		 * now, the CPU will try to set it when the Guest first loads
 		 * that entry into a segment register.  But the GDT isn't
 		 * writable by the Guest, so bad things can happen. */
-		cpu->arch.gdt[i].b |= 0x00000100;
+		desc_hi(cpu->arch.gdt[i]) |= 0x00000100;
 	}
 }
 
@@ -102,8 +102,8 @@ void setup_default_gdt_entries(struct lguest_ro_state *state)
 	 * Forgive the magic flags: the 0x8900 means the entry is Present, it's
 	 * privilege level 0 Available 386 TSS system segment, and the 0x67
 	 * means Saturn is eclipsed by Mercury in the twelfth house. */
-	gdt[GDT_ENTRY_TSS].a = 0x00000067 | (tss << 16);
-	gdt[GDT_ENTRY_TSS].b = 0x00008900 | (tss & 0xFF000000)
+	desc_lo(gdt[GDT_ENTRY_TSS]) = 0x00000067 | (tss << 16);
+	desc_hi(gdt[GDT_ENTRY_TSS]) = 0x00008900 | (tss & 0xFF000000)
 		| ((tss >> 16) & 0x000000FF);
 }
 
@@ -116,8 +116,8 @@ void setup_guest_gdt(struct lg_cpu *cpu)
 	cpu->arch.gdt[GDT_ENTRY_KERNEL_DS] = FULL_SEGMENT;
 	/* ...except the Guest is allowed to use them, so set the privilege
 	 * level appropriately in the flags. */
-	cpu->arch.gdt[GDT_ENTRY_KERNEL_CS].b |= (GUEST_PL << 13);
-	cpu->arch.gdt[GDT_ENTRY_KERNEL_DS].b |= (GUEST_PL << 13);
+	desc_hi(cpu->arch.gdt[GDT_ENTRY_KERNEL_CS]) |= (GUEST_PL << 13);
+	desc_hi(cpu->arch.gdt[GDT_ENTRY_KERNEL_DS]) |= (GUEST_PL << 13);
 }
 
 /*H:650 An optimization of copy_gdt(), for just the three "thead-local storage"
diff --git a/drivers/pnp/pnpbios/bioscalls.c b/drivers/pnp/pnpbios/bioscalls.c
index 7ff8244..2b9bb7c 100644
--- a/drivers/pnp/pnpbios/bioscalls.c
+++ b/drivers/pnp/pnpbios/bioscalls.c
@@ -476,8 +476,8 @@ void pnpbios_calls_init(union pnp_bios_install_struct *header)
 	pnp_bios_callpoint.offset = header->fields.pm16offset;
 	pnp_bios_callpoint.segment = PNP_CS16;
 
-	bad_bios_desc.a = 0;
-	bad_bios_desc.b = 0x00409200;
+	desc_lo(bad_bios_desc) = 0;
+	desc_hi(bad_bios_desc) = 0x00409200;
 
 	set_base(bad_bios_desc, __va((unsigned long)0x40 << 4));
 	_set_limit((char *)&bad_bios_desc, 4095 - (0x40 << 4));
diff --git a/include/asm-x86/xen/hypercall.h b/include/asm-x86/xen/hypercall.h
index 44f4259..6e36e20 100644
--- a/include/asm-x86/xen/hypercall.h
+++ b/include/asm-x86/xen/hypercall.h
@@ -472,8 +472,8 @@ MULTI_update_descriptor(struct multicall_entry *mcl, u64 maddr,
 	} else {
 		mcl->args[0] = maddr;
 		mcl->args[1] = maddr >> 32;
-		mcl->args[2] = desc.a;
-		mcl->args[3] = desc.b;
+		mcl->args[2] = desc_lo(desc);
+		mcl->args[3] = desc_hi(desc);
 	}
 }
 
-- 
1.5.4.3

[PATCH 12/12] x86: Use struct fields instead of bitmasks

[Joe Damato]

Use fields in structs instead of bitmasks for getting/setting descriptor data.

Signed-off-by: Joe Damato <ice799@gmail.com>
---
 drivers/lguest/interrupts_and_traps.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
index bfb24d9..0d9c065 100644
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -178,7 +178,7 @@ void maybe_do_interrupt(struct lg_cpu *cpu)
 	 * over them. */
 	idt = &cpu->arch.idt[FIRST_EXTERNAL_VECTOR+irq];
 	/* If they don't have a handler (yet?), we just ignore it */
-	if (idt_present(idt->a, idt->b)) {
+	if (idt->p) {
 		/* OK, mark it no longer pending and deliver it. */
 		clear_bit(irq, cpu->irqs_pending);
 		/* set_guest_interrupt() takes the interrupt descriptor and a
@@ -254,7 +254,7 @@ int deliver_trap(struct lg_cpu *cpu, unsigned int num)
 
 	/* Early on the Guest hasn't set the IDT entries (or maybe it put a
 	 * bogus one in): if we fail here, the Guest will be killed. */
-	if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))
+	if (!cpu->arch.idt[num].p)
 		return 0;
 	set_guest_interrupt(cpu, desc_lo(cpu->arch.idt[num]),
 			    desc_hi(cpu->arch.idt[num]), has_err(num));
@@ -461,7 +461,7 @@ void copy_traps(const struct lg_cpu *cpu, gate_desc *idt,
 		 * If it can't go direct, we still need to copy the priv. level:
 		 * they might want to give userspace access to a software
 		 * interrupt. */
-		if (idt_type(gidt->a, gidt->b) == 0xF)
+		if (gidt->type == 0xF)
 			idt[i] = *gidt;
 		else
 			default_idt_entry(&idt[i], i, def[i], gidt);
-- 
1.5.4.3

Re: [PATCH 12/12] x86: Use struct fields instead of bitmasks

[Jeremy Fitzhardinge]

Joe Damato wrote:
> Use fields in structs instead of bitmasks for getting/setting descriptor data.
>
> Signed-off-by: Joe Damato <ice799@gmail.com>
> ---
>  drivers/lguest/interrupts_and_traps.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
> index bfb24d9..0d9c065 100644
> --- a/drivers/lguest/interrupts_and_traps.c
> +++ b/drivers/lguest/interrupts_and_traps.c
> @@ -178,7 +178,7 @@ void maybe_do_interrupt(struct lg_cpu *cpu)
>  	 * over them. */
>  	idt = &cpu->arch.idt[FIRST_EXTERNAL_VECTOR+irq];
>  	/* If they don't have a handler (yet?), we just ignore it */
> -	if (idt_present(idt->a, idt->b)) {
> +	if (idt->p) {
>   

No, using an idt_present() accessor is better, but just pass it an idt.

>  		/* OK, mark it no longer pending and deliver it. */
>  		clear_bit(irq, cpu->irqs_pending);
>  		/* set_guest_interrupt() takes the interrupt descriptor and a
> @@ -254,7 +254,7 @@ int deliver_trap(struct lg_cpu *cpu, unsigned int num)
>  
>  	/* Early on the Guest hasn't set the IDT entries (or maybe it put a
>  	 * bogus one in): if we fail here, the Guest will be killed. */
> -	if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))
> +	if (!cpu->arch.idt[num].p)
>  		return 0;
>  	set_guest_interrupt(cpu, desc_lo(cpu->arch.idt[num]),
>  			    desc_hi(cpu->arch.idt[num]), has_err(num));
> @@ -461,7 +461,7 @@ void copy_traps(const struct lg_cpu *cpu, gate_desc *idt,
>  		 * If it can't go direct, we still need to copy the priv. level:
>  		 * they might want to give userspace access to a software
>  		 * interrupt. */
> -		if (idt_type(gidt->a, gidt->b) == 0xF)
> +		if (gidt->type == 0xF)
>   

We should have symbolic names for the IDT types.

    J

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Willy Tarreau]

On Fri, Oct 24, 2008 at 08:15:20PM -0700, Joe Damato wrote:
> Hi -
> 
> This is my first submission to the kernel, so (beware!) please let me know if I can make any improvements on these patches.

It looks like you provide one patch per file. You should group some of them
together so that each patch does a functional change and everything still
builds after incrementally applying that patch. The way you have splitted
them will break builds if someone tries to build after patch 1 for instance.

Willy

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Ingo Molnar]

* Willy Tarreau <w@1wt.eu> wrote:

> On Fri, Oct 24, 2008 at 08:15:20PM -0700, Joe Damato wrote:
> > Hi -
> > 
> > This is my first submission to the kernel, so (beware!) please let me know if I can make any improvements on these patches.
> 
> It looks like you provide one patch per file. You should group some 
> of them together so that each patch does a functional change and 
> everything still builds after incrementally applying that patch. The 
> way you have splitted them will break builds if someone tries to 
> build after patch 1 for instance.

yes, at every step the kernel is expected to build and boot fine.

	Ingo

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[walter harms]

hi joe,
i am not a maintainer here so my comments are my private commonts so ..
1. this is a lot of work and thx for it

patch 2/12:
personaly i am not a fan of typedefs especially like this one:
typedef struct gate_struct gate_desc;
they make people think they move a int or something around that is a fat
struct in real. do you really need a typedef ? or can you live with "struct gate_struct" instead ?

(see: also http://www.linuxjournal.com/article/5780 section: typedef Is Evil )

patch 6/12 (same goes for others)
is is possible to be more verbose ? what does l/d/g mean ? maybe an enum ?
+	desc->avl    = flags & 0x1;
+	desc->l      = (flags & 0x2) >> 1;
+	desc->d      = (flags & 0x4) >> 2;
+	desc->g      = (flags & 0x8) >> 3;

i noticed you are accessing hi/lo often. you can use a macro like:

from: highuid.h
#define low_16_bits(x)  ((x) & 0xFFFF)
#define high_16_bits(x) (((x) & 0xFFFF0000) >> 16)

or use a union {
	int foo;
	char bar[4];
	}



keep on going ...
	walter


Joe Damato schrieb:
> Hi -
> 
> This is my first submission to the kernel, so (beware!) please let me know if I can make any improvements on these patches.
> 
> I attempted to clean up the x86 structs for 32bit cpus that store IDT/LDT/GDT data by removing the fields labeled "a" and "b" in favor of more descriptive field names. I added some macros and went through the kernel cleaning up the various places where "a" and "b" were used.
> 
> I tried building my kernel with my .config and then also did a make allyesconfig build to help ensure I found everything that was using the old structure names. I also tried a few grep patterns. Hopefully I got everyone out.
> 
> 
> Thanks,
> Joe Damato
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Ingo Molnar]

* Joe Damato <ice799@gmail.com> wrote:

> Hi -
> 
> This is my first submission to the kernel, so (beware!) please let 
> me know if I can make any improvements on these patches.
> 
> I attempted to clean up the x86 structs for 32bit cpus that store 
> IDT/LDT/GDT data by removing the fields labeled "a" and "b" in favor 
> of more descriptive field names. I added some macros and went 
> through the kernel cleaning up the various places where "a" and "b" 
> were used.
> 
> I tried building my kernel with my .config and then also did a make 
> allyesconfig build to help ensure I found everything that was using 
> the old structure names. I also tried a few grep patterns. Hopefully 
> I got everyone out.

hm, a couple of comments.

Firstly, a patch logistical one: we moved all the x86 header files 
from include/asm-x86/ to arch/x86/include/asm/ in v2.6.28-rc1 - your 
patchset is against an older kernel. Should be easy enough to fix up.

Secondly, i'm not that convinced about the expanded use of bitfields 
that your patchset implements. Their semantics are notoriously fragile 
so we'd rather get _away_ from them, not expand them. _But_, this area 
could be cleaned up some more - just in a different way. I'd suggest 
you introduce field accessor inline functions to descriptors.

I.e. instead of:

        if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))

we could do a more compact form:

	if (!idt_present(cpu->arch.idt + num))

and get away from the open-coded use of desc->a and desc->b fields, 
with proper inlined helpers.

Small detail, the syntactic form you chose:

+       if (!cpu->arch.idt[num].p)

is not very readable because it's not obvious at first sight that ".p" 
intends to mean "present bit". If then idt[num].present would have 
been the better choice - but it's even better to not do bitfields at 
all but an idt_present(desc *) helper inline function.

Thirdly, as you can see it form my comments, this is not something 
that is really a best choice for a newbie, as it's a wide patchset 
that impacts a lot of critical code, wich has very high quality 
requirements.

But if you dont mind having to go through a couple of iterations to 
get it right (with the inevitable feeling of ftrustration about such a 
difficult process) then sure, feel free to work on this!

	Ingo

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> Small detail, the syntactic form you chose:
> 
> +       if (!cpu->arch.idt[num].p)
> 
> is not very readable because it's not obvious at first sight that ".p" 
> intends to mean "present bit". If then idt[num].present would have 
> been the better choice - but it's even better to not do bitfields at 
> all but an idt_present(desc *) helper inline function.
> 

There is, however, some benefit to use the field names that are in the 
official documentation, which include P.

	-hpa

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Joe Damato]

Ingo Molnar wrote:
> * Joe Damato <ice799@gmail.com> wrote:
>
>   
>> Hi -
>>
>> This is my first submission to the kernel, so (beware!) please let 
>> me know if I can make any improvements on these patches.
>>
>> I attempted to clean up the x86 structs for 32bit cpus that store 
>> IDT/LDT/GDT data by removing the fields labeled "a" and "b" in favor 
>> of more descriptive field names. I added some macros and went 
>> through the kernel cleaning up the various places where "a" and "b" 
>> were used.
>>
>> I tried building my kernel with my .config and then also did a make 
>> allyesconfig build to help ensure I found everything that was using 
>> the old structure names. I also tried a few grep patterns. Hopefully 
>> I got everyone out.
>>     
>
> hm, a couple of comments.
>   

Thanks for your very useful comments and feedback. I've included a few 
questions/comments below.

> Firstly, a patch logistical one: we moved all the x86 header files 
> from include/asm-x86/ to arch/x86/include/asm/ in v2.6.28-rc1 - your 
> patchset is against an older kernel. Should be easy enough to fix up.
>   

Ah, sorry about that. Should be easy enough to fix with git.

> Secondly, i'm not that convinced about the expanded use of bitfields 
> that your patchset implements. Their semantics are notoriously fragile 
> so we'd rather get _away_ from them, not expand them.

Out of curiosity what exactly do you mean when you say "fragile"? Sorry 
for my ignorance here...

>  _But_, this area 
> could be cleaned up some more - just in a different way. I'd suggest 
> you introduce field accessor inline functions to descriptors.
>
> I.e. instead of:
>
>         if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))
>
> we could do a more compact form:
>
> 	if (!idt_present(cpu->arch.idt + num))
>
> and get away from the open-coded use of desc->a and desc->b fields, 
> with proper inlined helpers.
>   

That sounds reasonable, I will play around, write a few, and probably 
resubmit in a few days.

> Small detail, the syntactic form you chose:
>
> +       if (!cpu->arch.idt[num].p)
>
> is not very readable because it's not obvious at first sight that ".p" 
> intends to mean "present bit". If then idt[num].present would have 
> been the better choice - but it's even better to not do bitfields at 
> all but an idt_present(desc *) helper inline function.
>
>   

OK, I'll try to use more descriptive names. As hpa pointed out in his 
email, 'p' is the name of the field in the intel x86 documentation. 
That's why I chose that, but I agree it isn't particularly clear.

> Thirdly, as you can see it form my comments, this is not something 
> that is really a best choice for a newbie, as it's a wide patchset 
> that impacts a lot of critical code, wich has very high quality 
> requirements.
>
> But if you dont mind having to go through a couple of iterations to 
> get it right (with the inevitable feeling of ftrustration about such a 
> difficult process) then sure, feel free to work on this!
>   

I will probably continue to play around with it and try to resubmit 
something in a few days that incorporates your feedback. I've done some 
x86 stuff before (never with linux, though) and I enjoy crawling though 
the intel docs and pushing bits around =].


Thanks again for the feedback,
Joe

Re: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs

[Jeremy Fitzhardinge]

Joe Damato wrote:
>> Small detail, the syntactic form you chose:
>>
>> +       if (!cpu->arch.idt[num].p)
>>
>> is not very readable because it's not obvious at first sight that 
>> ".p" intends to mean "present bit". If then idt[num].present would 
>> have been the better choice - but it's even better to not do 
>> bitfields at all but an idt_present(desc *) helper inline function.
>>
>>   
>
> OK, I'll try to use more descriptive names. As hpa pointed out in his 
> email, 'p' is the name of the field in the intel x86 documentation. 
> That's why I chose that, but I agree it isn't particularly clear.

Using bitfields would be a lot more appealing if the x86 design weren't 
so batshit insane.  Given that the addresses and limits are split of 
multiple bitfields, you need to have a set of accessors for those at 
least.  If you're going to do that, it might be worth having them for 
all the fields, at least for consistency.  Perhaps this would be too 
ugly and clumsy, but there isn't much code which really does anything 
with descriptors in detail.

    J
>
> Thanks again for the feedback,
> Joe