Re: [PATCH 4/8] unify paravirt parts of system.h

["Glauber de Oliveira Costa" <glommer@gmail.com>]

On Dec 4, 2007 5:18 PM, Avi Kivity <avi@qumranet.com> wrote:
> Glauber de Oliveira Costa wrote:
> > This patch moves the i386 control registers manipulation functions,
> > wbinvd, and clts functions to system.h. They are essentially the same
> > as in x86_64, except for the cr8 register, which we add.
> >
> > +
> > +static inline unsigned long native_read_cr8(void)
> > +{
> > +     unsigned long cr8;
> > +     asm volatile("mov %%cr8,%0" : "=r" (cr8), "=m" (__force_order));
> > +     return cr8;
> > +}
> > +
> >
>
> There is no cr8 register on i386.  This had better be protected by an
> #ifdef.

Sure. I mentioned it in the changelog. I, however, am not sure If I
agree it should be enclosed
in ifdefs. Me and Jeremy discussed it a while ago, and we seem to
agree that for those functions
that are exclusive of one architecture, there were no need for ifdefs.
Any usage by the other arch
is a bug.

But I admit that I'm not particularly biased here, and I can change
it, if there's agreement that
an ifdef here is the way to go.

> (you're likely not getting an error since it's a static inline, so the
> asm is never emitted)
>
Which also means it does not affect the binary in anyway. No bigger
code, no nothing.
My current approach is to save the ifdefs for pieces that in fact can
save us some resources
in the final image. But again... I can change it.



-- 
Glauber de Oliveira Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."