lkml.org 
[lkml]   [2008]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 00/12] x86: Cleanup idt, gdt/ldt/tss structs
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


\
 
 \ /
  Last update: 2008-10-27 22:19    [W:0.155 / U:1.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site