x86: Cleanup idt, gdt/ldt/tss structs
Subsystems (4)
cover x86: Cleanup idt, gdt/ldt/tss structs
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 1/12 x86: Cleanup x86 descriptors, remove a/b fields from structs
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.
--- include/asm-x86/desc_defs.h | 32 +++++++++++++------------------- 1 files changed, 13 insertions(+), 19 deletions(-)
include/asm-x86/desc_defs.h +13 -19
@@ -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 {
Signature · 1 line
1.5.4.3
PATCH 2/12 x86: Use new gate_struct for gate_desc
Change the gate_desc typedef to use the new gate_struct.
--- include/asm-x86/desc_defs.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
include/asm-x86/desc_defs.h +1 -1
@@ -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))
Signature · 1 line
1.5.4.3
PATCH 3/12 x86: Cleanup usage of struct desc_struct
Use gate_desc typedef for IDT entries instead of struct desc_struct.
--- 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(-)
arch/x86/kernel/vmi_32.c +1 -1
@@ -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);
drivers/lguest/interrupts_and_traps.c +6 -6
@@ -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))
drivers/lguest/lg.h +1 -1
@@ -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);
include/asm-x86/lguest.h +2 -2
@@ -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;
Signature · 1 line
1.5.4.3
PATCH 4/12 x86: Add macros for gate_desc
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.
--- include/asm-x86/desc_defs.h | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
include/asm-x86/desc_defs.h +7 -2
@@ -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 {
Signature · 1 line
1.5.4.3
PATCH 5/12 x86: Refactor pack_gate for gate_desc
Rewrite pack_gate to use the newly add struct fields instead of bitmasks.
--- include/asm-x86/desc.h | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
include/asm-x86/desc.h +8 -3
@@ -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
Signature · 1 line
1.5.4.3
PATCH 6/12 x86: Refactor pack_descriptor
Refactor pack_descriptor to use fields in the struct instead of bitmasks
--- include/asm-x86/desc.h | 18 +++++++++++++----- 1 files changed, 13 insertions(+), 5 deletions(-)
include/asm-x86/desc.h +13 -5
@@ -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 ; }
Signature · 1 line
1.5.4.3
PATCH 7/12 x86: Add a static initializer for IDTs
Add static intializer for IDT entries.
--- include/asm-x86/desc_defs.h | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
include/asm-x86/desc_defs.h +9 -0
@@ -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 {
Signature · 1 line
1.5.4.3
PATCH 8/12 x86: Use static intializer for IDT entries
Use static intializer for IDT entries.
--- arch/x86/kernel/traps.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/traps.c +1 -1
@@ -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;
Signature · 1 line
1.5.4.3
PATCH 9/12 x86: Add static initiazlier for descriptors
Add static initializer for LDT/GDT/TSS descriptors.
--- include/asm-x86/desc_defs.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
include/asm-x86/desc_defs.h +14 -0
@@ -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 {
Signature · 1 line
1.5.4.3
PATCH 10/12 x86: Use static initializers for descriptors
Use static intializers for GDT/LDT entries.
--- 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(-)
arch/x86/kernel/apm_32.c +1 -1
@@ -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 */
arch/x86/kernel/cpu/common.c +14 -14
@@ -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);
include/asm-x86/lguest.h +2 -2
@@ -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__ */
Signature · 1 line
1.5.4.3
PATCH 11/12 x86: Use macros for getting/setting descriptors
Use macros for getting/setting the hi and lo 32bits of descriptors since 'a' and 'b' were removed.
--- 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(-)
arch/x86/kernel/tls.c +1 -1
@@ -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;
arch/x86/kernel/vmi_32.c +2 -1
@@ -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 ); }
arch/x86/lguest/boot.c +1 -1
@@ -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])); }/*
arch/x86/math-emu/fpu_system.h +9 -10
@@ -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)
drivers/lguest/interrupts_and_traps.c +9 -9
@@ -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. */
drivers/lguest/segments.c +7 -7
@@ -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"
drivers/pnp/pnpbios/bioscalls.c +2 -2
@@ -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 ));
include/asm-x86/xen/hypercall.h +2 -2
@@ -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); } }
Signature · 1 line
1.5.4.3
PATCH 12/12 x86: Use struct fields instead of bitmasks
Use fields in structs instead of bitmasks for getting/setting descriptor data.
--- drivers/lguest/interrupts_and_traps.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/lguest/interrupts_and_traps.c +3 -3
@@ -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);
Signature · 1 line
1.5.4.3
Re: [PATCH 12/12] x86: Use struct fields instead of bitmasks
Quoted 12 lines · Joe Damato
Use fields in structs instead of bitmasks for getting/setting descriptor data.
Signed-off-by Joe Damato <[email protected]>--- drivers/lguest/interrupts_and_traps.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)drivers/lguest/interrupts_and_traps.c +0
-0
drivers/lguest/interrupts_and_traps.c void maybe_do_interrupt(struct lg_cpu *cpu) · line 182
Quoted 6 lines
drivers/lguest/interrupts_and_traps.c +1
-1@@ -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.
Quoted 13 lines
drivers/lguest/interrupts_and_traps.c +0
-0/* OK, mark it no longer pending and deliver it. */ clear_bit (irq, cpu->irqs_pending);drivers/lguest/interrupts_and_traps.c +1
-1/* 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));
drivers/lguest/interrupts_and_traps.c void copy_traps(const struct lg_cpu *cpu, gate_desc *idt, · line 465
Quoted 6 lines
drivers/lguest/interrupts_and_traps.c +1
-1@@ -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 09/12] x86: Add static initiazlier for descriptors
In reply to Joe DamatoQuoted 20 lines · Joe Damato
Add static initializer for LDT/GDT/TSS descriptors.
Signed-off-by Joe Damato <[email protected]>--- include/asm-x86/desc_defs.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)include/asm-x86/desc_defs.h +5
-0@@ -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 \
include/asm-x86/desc_defs.h typedef struct desc_struct tss_desc; · line 108
Quoted 10 lines
include/asm-x86/desc_defs.h +9
-0+ , .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
Re: [PATCH 04/12] x86: Add macros for gate_desc
In reply to Joe DamatoQuoted 2 lines · 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.
Can you make them inline functions?
J
Re: [PATCH 03/12] x86: Cleanup usage of struct desc_struct
In reply to Joe DamatoQuoted 2 lines · Joe Damato
Use
gate_desctypedef for IDT entries instead of structdesc_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
Re: [PATCH 02/12] x86: Use new gate_struct for gate_desc
In reply to Joe DamatoQuoted 2 lines · Joe Damato
Change the
gate_desctypedef to use the newgate_struct.
Blerk, yes. I'd really prefer these typedefs to use the _t convention.
J
Quoted 3 lines · 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.
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
Quoted 10 lines · Willy Tarreau
Quoted 3 lines · 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.
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
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
Quoted 18 lines · 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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · 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.
Quoted 4 lines
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
H. Peter Anvin
reviewerQuoted 10 lines · Ingo Molnar
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
Quoted 14 lines · Ingo Molnar
Quoted 11 lines · 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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 4 lines
Firstly, a patch logistical one: we moved all the x86 header files from
include/asm-x86/toarch/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.
Quoted 3 lines
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...
Quoted 6 lines
_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:
Quoted 9 lines
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->aanddesc->bfields, with proper inlined helpers.
That sounds reasonable, I will play around, write a few, and probably resubmit in a few days.
Quoted 10 lines
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.
Quoted 9 lines
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
Quoted 11 lines · Joe Damato
Quoted 10 lines
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.
Quoted 3 lines
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
Quoted 3 lines
Thanks again for the feedback,
Joe