[PATCH 0/8] Integrate system.h - Kernel

This is a discussion on [PATCH 0/8] Integrate system.h - Kernel ; Hi, In the same lines of the msr.h integration, here it goes a series for system.h. Again, after the headers are turned into one, the paravirt pieces related to system.h comes for free. -- To unsubscribe from this list: send ...

+ Reply to Thread
Results 1 to 12 of 12

Thread: [PATCH 0/8] Integrate system.h

  1. [PATCH 0/8] Integrate system.h

    Hi,

    In the same lines of the msr.h integration, here it goes a series for
    system.h. Again, after the headers are turned into one, the paravirt
    pieces related to system.h comes for free.



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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.

    (you're likely not getting an error since it's a static inline, so the
    asm is never emitted)

    --
    Any sufficiently difficult bug is indistinguishable from a feature.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Tue, Dec 04, 2007 at 09:18:33PM +0200, Avi Kivity 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.
    >
    > (you're likely not getting an error since it's a static inline, so the asm
    > is never emitted)


    Linux never uses that register. The only user is suspend save/restore,
    but that' bogus because it wasn't ever initialized by Linux in the first
    place. It could be probably all safely removed.

    -Andi

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Tuesday 04 December 2007 11:41, Glauber de Oliveira Costa wrote:
    > On Dec 4, 2007 5:18 PM, Avi Kivity wrote:
    > > 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.


    If future changes will mistakenly make 32-bit x86 call native_read_cr8(),
    you will get no warning. (Hmmm. Maybe as will complain, I'm not sure).
    If it explicitly ifdefed out for 32 bits, it's easier to detect misuse.
    --
    vda
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Tue 2007-12-04 20:34:32, Andi Kleen wrote:
    > On Tue, Dec 04, 2007 at 09:18:33PM +0200, Avi Kivity 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.
    > >
    > > (you're likely not getting an error since it's a static inline, so the asm
    > > is never emitted)

    >
    > Linux never uses that register. The only user is suspend save/restore,
    > but that' bogus because it wasn't ever initialized by Linux in the first
    > place. It could be probably all safely removed.


    It probably is safe to remove... but we currently support '2.8.95
    kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    cr8.

    So please keep it if it is not a big problem.

    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    * Pavel Machek wrote:

    > > Linux never uses that register. The only user is suspend
    > > save/restore, but that' bogus because it wasn't ever initialized by
    > > Linux in the first place. It could be probably all safely removed.

    >
    > It probably is safe to remove... but we currently support '2.8.95
    > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > cr8.
    >
    > So please keep it if it is not a big problem.


    hm, so __save_processor_state() is in essence an ABI? Could you please
    also send a patch that documents this prominently, in the structure
    itself?

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > It probably is safe to remove... but we currently support '2.8.95
    > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > cr8.


    No it won't. 2.8 would just restore some random useless value.
    If 2.8 wants to use CR8 it would have to re-initialize it

    -Andi
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Pavel Machek wrote:
    >
    > It probably is safe to remove... but we currently support '2.8.95
    > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > cr8.
    >
    > So please keep it if it is not a big problem.
    >


    Note that CR8 is an alias for the TPR in the APIC.

    -hpa
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Sat 2007-12-15 14:26:38, Andi Kleen wrote:
    > > It probably is safe to remove... but we currently support '2.8.95
    > > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > > cr8.

    >
    > No it won't. 2.8 would just restore some random useless value.


    Restoring random value seems wrong. Putting random values into cpu
    registers can break stuff, right?

    Even if 2.6.24 image being restored did not set %cr8 itself, it may
    depend on %cr8 to have "sane" value.

    > If 2.8 wants to use CR8 it would have to re-initialize it


    We are talking "2.8 restores 2.6 image" here.

    Pavel
    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Saturday, 15 of December 2007, Ingo Molnar wrote:
    >
    > * Pavel Machek wrote:
    >
    > > > Linux never uses that register. The only user is suspend
    > > > save/restore, but that' bogus because it wasn't ever initialized by
    > > > Linux in the first place. It could be probably all safely removed.

    > >
    > > It probably is safe to remove... but we currently support '2.8.95
    > > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > > cr8.
    > >
    > > So please keep it if it is not a big problem.

    >
    > hm, so __save_processor_state() is in essence an ABI? Could you please
    > also send a patch that documents this prominently, in the structure
    > itself?


    Hmm, I'm not sure if it really is an ABI part. It doesn't communicate anything
    outside of the kernel in which it is defined.

    The problem is, though, that if kernel A is used for resuming kernel B, and
    kernel B doesn't save/restore everything it will need after the resume, then
    things will break if kernel A modifies that. So, yes, we'll need to document
    that explicitly.

    Greetings,
    Rafael
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon 2007-12-17 01:27:29, Rafael J. Wysocki wrote:
    > On Saturday, 15 of December 2007, Ingo Molnar wrote:
    > >
    > > * Pavel Machek wrote:
    > >
    > > > > Linux never uses that register. The only user is suspend
    > > > > save/restore, but that' bogus because it wasn't ever initialized by
    > > > > Linux in the first place. It could be probably all safely removed.
    > > >
    > > > It probably is safe to remove... but we currently support '2.8.95
    > > > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > > > cr8.
    > > >
    > > > So please keep it if it is not a big problem.

    > >
    > > hm, so __save_processor_state() is in essence an ABI? Could you please
    > > also send a patch that documents this prominently, in the structure
    > > itself?

    >
    > Hmm, I'm not sure if it really is an ABI part. It doesn't communicate anything
    > outside of the kernel in which it is defined.


    Well, it is not "application binary interface", but it is
    "kernel-to-kernel binary interface"...

    > The problem is, though, that if kernel A is used for resuming kernel B, and
    > kernel B doesn't save/restore everything it will need after the resume, then
    > things will break if kernel A modifies that. So, yes, we'll need to document
    > that explicitly.


    Agreed.
    Pavel
    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Monday, 17 of December 2007, Pavel Machek wrote:
    > On Mon 2007-12-17 01:27:29, Rafael J. Wysocki wrote:
    > > On Saturday, 15 of December 2007, Ingo Molnar wrote:
    > > >
    > > > * Pavel Machek wrote:
    > > >
    > > > > > Linux never uses that register. The only user is suspend
    > > > > > save/restore, but that' bogus because it wasn't ever initialized by
    > > > > > Linux in the first place. It could be probably all safely removed.
    > > > >
    > > > > It probably is safe to remove... but we currently support '2.8.95
    > > > > kernel loads/resumes 2.6.24 image'... which would break if 2.8 uses
    > > > > cr8.
    > > > >
    > > > > So please keep it if it is not a big problem.
    > > >
    > > > hm, so __save_processor_state() is in essence an ABI? Could you please
    > > > also send a patch that documents this prominently, in the structure
    > > > itself?

    > >
    > > Hmm, I'm not sure if it really is an ABI part. It doesn't communicate anything
    > > outside of the kernel in which it is defined.

    >
    > Well, it is not "application binary interface", but it is
    > "kernel-to-kernel binary interface"...


    Hm, rather a kernel-to-itself interface. ;-)

    Rafael
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread