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

[Joe Damato <ice799@gmail.com>]

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