x86: Remove direct access to internal x86 descriptor fields
Subsystems (4)
cover x86: Remove direct access to internal x86 descriptor fields
Hi -
This patch set aims to remove the direct access to the 'a' and 'b' fields of 32-bit x86 descriptors. Static inline getters/setters are provided.
This is a resumbmit of a patch set I submitted several months ago[1]. My last patch set was a bit large, so I've tried to take a smaller bite this time and slowly build the cleanup effort over a series of patch sets.
Any and all comments, suggestions, and questions are welcome.
Thanks,
Joe Damato
[1] http://lkml.org/lkml/2008/10/24/459
---
Joe Damato (2):
x86: Add getter/setter static inlines for x86 descriptors
x86: Replace direct access to descriptor fields with getter/setters
arch/x86/include/asm/desc.h | 32 ++++++++++++++++++++++++++------ arch/x86/include/asm/xen/hypercall.h | 5 +++-- arch/x86/kernel/tls.c | 6 ++++-- arch/x86/kernel/vmi_32.c | 3 ++- arch/x86/lguest/boot.c | 2 +- arch/x86/math-emu/fpu_system.h | 20 ++++++++++---------- drivers/lguest/interrupts_and_traps.c | 25 +++++++++++++------------ drivers/lguest/segments.c | 20 ++++++++++++-------- drivers/pnp/pnpbios/bioscalls.c | 4 ++-- 9 files changed, 73 insertions(+), 44 deletions(-)
PATCH 1/2 x86: Add getter/setter static inlines for x86 descriptors
Static inline getters/setters have been provided to encourage consumers not to touch the internals of 32bit x86 descriptors directly.
--- arch/x86/include/asm/desc.h | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
arch/x86/include/asm/desc.h +20 -0
@@ -44,6 +44,26 @@ static inline struct desc_struct *get_cpu_gdt_table(unsigned int cpu)return per_cpu (gdt_page, cpu).gdt; } +static inline void desc_set_lo (struct desc_struct *d,unsigned int lo) +{ + d->a = lo; +} + +static inline void desc_set_hi (struct desc_struct *d,unsigned int hi) +{ + d->b = hi; +} + +static inline unsigned int desc_get_lo (const struct desc_struct *d) +{ +return d->a; +} + +static inline unsigned int desc_get_hi (const struct desc_struct *d) +{ +return d->b; +} +#ifdef CONFIG_X86_64 static inline void pack_gate(gate_desc *gate, unsigned type, unsigned long func,
Signature · 1 line
1.6.2
PATCH 2/2 x86: Replace direct access to descriptor fields with getter/setters
Use the getters/setters for 32bit x86 descriptors instead of touching the internal fields directly.
--- arch/x86/include/asm/desc.h | 12 ++++++------ arch/x86/include/asm/xen/hypercall.h | 5 +++-- arch/x86/kernel/tls.c | 6 ++++-- arch/x86/kernel/vmi_32.c | 3 ++- arch/x86/lguest/boot.c | 2 +- arch/x86/math-emu/fpu_system.h | 20 ++++++++++---------- drivers/lguest/interrupts_and_traps.c | 25 +++++++++++++------------ drivers/lguest/segments.c | 20 ++++++++++++-------- drivers/pnp/pnpbios/bioscalls.c | 4 ++-- 9 files changed, 53 insertions(+), 44 deletions(-)
arch/x86/include/asm/desc.h +6 -6
@@ -86,9 +86,9 @@ 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 ); + desc_set_lo(gate, (seg << 16) | (base & 0xffff)); +desc_set_hi (gate, (base &0xffff0000 ) | + (((0x80 | type | (dpl <<5 )) &0xff ) <<8 )); }#endif
@@ -166,10 +166,10 @@ 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 ) | + desc_set_lo(desc, ((base & 0xffff) << 16) | (limit & 0xffff)); +desc_set_hi (desc, (base &0xff000000 ) | ((base &0xff0000 ) >>16 ) | (limit &0x000f0000 ) | ((type &0xff ) <<8 ) | - ((flags &0xf ) <<20 ); + ((flags &0xf ) <<20 )); desc->p =1 ; }
arch/x86/include/asm/xen/hypercall.h +3 -2
@@ -39,6 +39,7 @@#include <linux/string.h> #include <linux/types.h> +#include <asm/desc.h> #include <asm/page.h> #include <asm/pgtable.h>
@@ -478,8 +479,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_get_lo (&desc); + mcl->args[3 ] =desc_get_hi (&desc); } }
arch/x86/kernel/tls.c +4 -2
@@ -41,8 +41,10 @@ static void set_tls_desc(struct task_struct *p, int idx, cpu =get_cpu ();while (n-- >0 ) { -if (LDT_empty (info)) - desc->a = desc->b =0 ; +if (LDT_empty (info)) { +desc_set_lo (desc,0 ); +desc_set_hi (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_get_lo (gdt + nr) !=desc_get_lo (new) || +desc_get_hi (gdt + nr) !=desc_get_hi (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_get_lo (idt + i),desc_get_hi (idt + i)); }/*
arch/x86/math-emu/fpu_system.h +10 -10
@@ -19,16 +19,16 @@/* 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_get_hi(&x) & (3 << 21)) +#define SEG_G_BIT (x) (desc_get_hi(&x) & (1 << 23)) +#define SEG_GRANULARITY (x) ((desc_get_hi(&x) & (1 << 23)) ? 4096 : 1) +#define SEG_286_MODE (x) (desc_get_hi(&x) & (0xff000000 | 0xf0000 | (1 << 23))) +#define SEG_BASE_ADDR (s) ((desc_get_hi(&s) & 0xff000000) \ + | ((desc_get_hi(&s) & 0xff) << 16) | (desc_get_lo(&s) >> 16)) +#define SEG_LIMIT (s) ((desc_get_hi(&s) & 0xff0000) | (desc_get_lo(&s) & 0xffff)) +#define SEG_EXECUTE_ONLY (s) ((desc_get_hi(&s) & ((1 << 11) | (1 << 9))) = (1 << 11)) +#define SEG_WRITE_PERM (s) ((desc_get_hi(&s) & ((1 << 11) | (1 << 9))) = (1 << 9)) +#define SEG_EXPAND_DOWN (s) ((desc_get_hi(&s) & ((1 << 11) | (1 << 10))) \ = (1 << 10))#define I387 (current->thread.xstate)
drivers/lguest/interrupts_and_traps.c +13 -12
@@ -178,13 +178,13 @@ 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_present (desc_get_lo (idt),desc_get_hi (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 * 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_get_lo (idt),desc_get_hi (idt),0 ); }/* Every time we deliver an interrupt, we update the timestamp in the
@@ -259,10 +259,10 @@ 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 (!idt_present (desc_get_lo (cpu->arch.idt + num),desc_get_hi (cpu->arch.idt + num)))return 0 ; -set_guest_interrupt (cpu, cpu->arch.idt[num].a, - cpu->arch.idt[num].b,has_err (num)); +set_guest_interrupt (cpu,desc_get_lo (cpu->arch.idt + num), +desc_get_hi (cpu->arch.idt + num),has_err (num));return 1 ; }
@@ -365,7 +365,8 @@ static void set_trap(struct lg_cpu *cpu, struct desc_struct *trap,/* We zero-out a not-present entry */ if (!idt_present (lo, hi)) { - trap->a = trap->b =0 ; +desc_set_lo (trap,0 ); +desc_set_hi (trap,0 );return ; }
@@ -377,8 +378,8 @@ static void set_trap(struct lg_cpu *cpu, struct desc_struct *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_set_lo (trap, ((__KERNEL_CS|GUEST_PL )<<16 ) | (lo&0x0000FFFF )); +desc_set_hi (trap, (hi&0xFFFFEF00 )); }/*H:230 While we're here, dealing with delivering traps and interrupts to the
@@ -424,11 +425,11 @@ static void default_idt_entry(struct desc_struct *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_get_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_set_lo (idt, (LGUEST_CS <<16 ) | (handler&0x0000FFFF )); +desc_set_hi (idt, (handler&0xFFFF0000 ) | flags); }/* When the Guest first starts, we put default entries into the IDT. */
@@ -466,7 +467,7 @@ void copy_traps(const struct lg_cpu *cpu, struct desc_struct *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 (idt_type (desc_get_lo (gidt),desc_get_hi (gidt)) =0xF ) idt[i] = *gidt;else default_idt_entry (&idt[i], i, def[i], gidt);
drivers/lguest/segments.c +12 -8
@@ -61,6 +61,7 @@ static int ignored_gdt(unsigned int num)static void fixup_gdt_table (struct lg_cpu *cpu,unsigned start,unsigned end) {unsigned int i; +struct desc_struct *tmp =NULL ;for (i = start; i < end; i++) {/* We never copy these ones to real GDT, so we don't care what
@@ -71,14 +72,15 @@ 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 ); + tmp = &cpu->arch.gdt[i]; +if ((desc_get_hi (tmp) &0x00006000 ) =0 ) +desc_set_hi (tmp,desc_get_hi (tmp) | (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_set_hi (tmp,desc_get_hi (tmp) |0x00000100 ); } }
@@ -102,9 +104,9 @@ 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 ) - | ((tss >>16 ) &0x000000FF ); +desc_set_lo (gdt +GDT_ENTRY_TSS ,0x00000067 | (tss <<16 )); +desc_set_hi (gdt +GDT_ENTRY_TSS ,0x00008900 | (tss &0xFF000000 ) + | ((tss >>16 ) &0x000000FF )); }/* This routine sets up the initial Guest GDT for booting. All entries start
@@ -116,8 +118,10 @@ 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_set_hi (cpu->arch.gdt +GDT_ENTRY_KERNEL_CS , +desc_get_hi (cpu->arch.gdt +GDT_ENTRY_KERNEL_CS ) | (GUEST_PL <<13 )); +desc_set_hi (cpu->arch.gdt +GDT_ENTRY_KERNEL_DS , +desc_get_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_set_lo (&bad_bios_desc,0 ); +desc_set_hi (&bad_bios_desc,0x00409200 );set_base (bad_bios_desc,__va ((unsigned long )0x40 <<4 ));_set_limit ((char *)&bad_bios_desc,4095 - (0x40 <<4 ));
Signature · 1 line
1.6.2
H. Peter Anvin
reviewerRe: [PATCH 1/2] x86: Add getter/setter static inlines for x86 descriptors
In reply to Joe DamatoQuoted 3 lines · Joe Damato
Static inline getters/setters have been provided to encourage consumers not to touch the internals of 32bit x86 descriptors directly.
Signed-off-by Joe Damato <[email protected]>
Okay, what is the motivation for this?
This is a serious question. The x86 descriptors are so complex that it's not clear to me that this restriction makes the code any more clear. Especially not with things like:
- cpu->arch.gdt[i].b |= 0x00000100;
+ desc_set_hi(tmp, desc_get_hi(tmp) | 0x00000100);
This isn't an improvement. If you're doing to so something like this, you need to actually implement the *intent* here.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH 1/2] x86: Add getter/setter static inlines for x86 descriptors
Quoted 11 lines · H. Peter Anvin
Quoted 3 lines · Joe Damato
Static inline getters/setters have been provided to encourage consumers not to touch the internals of 32bit x86 descriptors directly.
Signed-off-by Joe Damato <[email protected]>Okay, what is the motivation for this?
This is a serious question. The x86 descriptors are so complex that it's not clear to me that this restriction makes the code any more clear. Especially not with things like:
Quoted 5 lines
- cpu->arch.gdt[i].b |=
0x00000100;
+desc_set_hi(tmp, desc_get_hi(tmp) | 0x00000100);This isn't an improvement. If you're doing to so something like this, you need to actually implement the *intent* here.
Hi -
In my first patch set several months ago I replaced the structure desc_struct with bit fields that exposed the fields for IDT/LDT/TSS entries. This patch set was rejected for several reasons, one of which was that Linux is trying to move away from bit fields. I also received other comments such as "take a smaller bite", etc.
So instead of the bit fields, I implemented static inline getters/setters. I thought that this small change would be a good first step to re-test the waters of submitting patches to the kernel before I started to clean out more pieces of the x86 architecture specific code.
I agree that this isn't very clear, but from comments I received on my first set, I assumed that this was closer to what people wanted to see in the kernel. I am happy to iterate and submit something better, but I am clearly misunderstanding what people would like to see.
Any suggestions on how this first-timer can help are greatly appreciated.
Joe
--
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
H. Peter Anvin
reviewerRe: [PATCH 1/2] x86: Add getter/setter static inlines for x86 descriptors
Quoted 25 lines · Joe Damato
Quoted 10 lines
This is a serious question. The x86 descriptors are so complex that it's not clear to me that this restriction makes the code any more clear. Especially not with things like:
(quoted diff) +1
-1- cpu->arch.gdt[i].b |= 0x00000100; + desc_set_hi(tmp, desc_get_hi(tmp) | 0x00000100);This isn't an improvement. If you're doing to so something like this, you need to actually implement the *intent* here.
Hi -
In my first patch set several months ago I replaced the structure
desc_structwith bit fields that exposed the fields for IDT/LDT/TSS entries. This patch set was rejected for several reasons, one of which was that Linux is trying to move away from bit fields. I also received other comments such as "take a smaller bite", etc.So instead of the bit fields, I implemented static inline getters/setters. I thought that this small change would be a good first step to re-test the waters of submitting patches to the kernel before I started to clean out more pieces of the x86 architecture specific code.
Quoted 7 lines
I agree that this isn't very clear, but from comments I received on my first set, I assumed that this was closer to what people wanted to see in the kernel. I am happy to iterate and submit something better, but I am clearly misunderstanding what people would like to see.
Any suggestions on how this first-timer can help are greatly appreciated.
If you're going to implement get/set then you need getters and setters that make sense. Now, doing something like that might be acceptable as part of a larger patchset (which is one way to "take smaller bites"), but the above is just gratuitous obfuscation, since it leaves in place the ugliest part of it all, which is the magic constant 0x00000100.
Personally, I would be just as happy treating the descriptor as an u64 and explicitly do shifts and masks with well-defined constants. It's the magic constants that suck.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.