Ingo Molnar wrote:
* Joe Damato <ice799@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-x86_64" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
