Patch from LKML

Patch from LKML

[Zachary Amsden]

> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha
> <suresh.b.siddha@intel.com> wrote:
> > On Sun, Jul 13, 2008 at 10:19:35PM -0700, Yinghai Lu wrote:
> >>
> >> fix for pv.
> >>
> >> Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
> >>
> >> ---
> >>  arch/x86/kernel/paravirt.c |    5 ----
> >>  arch/x86/kernel/vmi_32.c   |   51 ++++++++++++++++++++++++++++++++++++++++++---
> >>  arch/x86/xen/enlighten.c   |   19 +++++++---------
> >>  include/asm-x86/paravirt.h |   29 -------------------------
> >>  4 files changed, 57 insertions(+), 47 deletions(-)
> >>
> >> Index: linux-2.6/arch/x86/kernel/paravirt.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/kernel/paravirt.c
> >> +++ linux-2.6/arch/x86/kernel/paravirt.c
> >> @@ -373,11 +373,6 @@ struct pv_cpu_ops pv_cpu_ops = {
> >>
> >>  struct pv_apic_ops pv_apic_ops = {
> >>  #ifdef CONFIG_X86_LOCAL_APIC
> >> -#ifndef CONFIG_X86_64
> >> -       .apic_write = native_apic_mem_write,
> >> -       .apic_write_atomic = native_apic_mem_write_atomic,
> >> -       .apic_read = native_apic_mem_read,
> >> -#endif
> >>         .setup_boot_clock = setup_boot_APIC_clock,
> >>         .setup_secondary_clock = setup_secondary_APIC_clock,
> >>         .startup_ipi_hook = paravirt_nop,
> >> Index: linux-2.6/arch/x86/kernel/vmi_32.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/kernel/vmi_32.c
> >> +++ linux-2.6/arch/x86/kernel/vmi_32.c
> >> @@ -676,6 +676,50 @@ static inline int __init probe_vmi_rom(v
> >>         return 0;
> >>  }
> >>
> >> +#ifdef CONFIG_X86_LOCAL_APIC
> >> +static u32 vmi_apic_read(u32 reg)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +static void vmi_apic_write(u32 reg, u32 val)
> >> +{
> >> +       /* Warn to see if there's any stray references */
> >> +       WARN_ON(1);
> >> +}
> >> +
> >> +static u64 vmi_apic_icr_read(void)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +static void vmi_apic_icr_write(u32 low, u32 id)
> >> +{
> >> +       /* Warn to see if there's any stray references */
> >> +       WARN_ON(1);
> >> +}
> >> +
> >> +static void vmi_apic_wait_icr_idle(void)
> >> +{
> >> +       return;
> >> +}
> >> +
> >> +static u32 vmi_safe_apic_wait_icr_idle(void)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >> +static struct apic_ops vmi_basic_apic_ops = {
> >> +        .read = vmi_apic_read,
> >> +        .write = vmi_apic_write,
> >> +        .write_atomic = vmi_apic_write,
> >> +        .icr_read = vmi_apic_icr_read,
> >> +        .icr_write = vmi_apic_icr_write,
> >> +        .wait_icr_idle = vmi_apic_wait_icr_idle,
> >> +        .safe_wait_icr_idle = vmi_safe_apic_wait_icr_idle,
> >> +};
> >> +#endif
> >> +
> >>  /*
> >>   * VMI setup common to all processors
> >>   */
> >> @@ -904,9 +948,10 @@ static inline int __init activate_vmi(vo
> >>  #endif
> >>
> >>  #ifdef CONFIG_X86_LOCAL_APIC
> >> -       para_fill(pv_apic_ops.apic_read, APICRead);
> >> -       para_fill(pv_apic_ops.apic_write, APICWrite);
> >> -       para_fill(pv_apic_ops.apic_write_atomic, APICWrite);
> >> +       para_fill(vmi_basic_apic_ops.read, APICRead);
> >> +       para_fill(vmi_basic_apic_ops.write, APICWrite);
> >> +       para_fill(vmi_basic_apic_ops.write_atomic, APICWrite);
> >> +       apic_ops = &vmi_basic_apic_ops;
> >
> > Yinghai, Looking more closely at this, based on my understanding this might be
> > wrong for VMI. Correct patch should be as follows. Any comments?
> 
> so you mean icr related will still use default native member?
> 
> YH

Nacked-by: Zachary Amsden <zach@vmware.com>

What are you doing here and why aren't you cc-ing the maintainers?

Thanks,

Zach

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Suresh Siddha]

On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
> 
> Nacked-by: Zachary Amsden <zach@vmware.com>
> 
> What are you doing here and why aren't you cc-ing the maintainers?

Sorry. I was about to bring you into the loop.

Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix 
for VMI case aswell.

Based on my understanding, tip/x86/x2apic git commit
94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
something like
http://marc.info/?l=linux-kernel&m=121614328831237&w=2

Can you please check and Ack/Nack this fix?

thanks,
suresh

Re: Patch from LKML

[Yinghai Lu]

On Tue, Jul 15, 2008 at 11:38 AM, Zachary Amsden <zach@vmware.com> wrote:
>
>> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha
>> <suresh.b.siddha@intel.com> wrote:
>> > On Sun, Jul 13, 2008 at 10:19:35PM -0700, Yinghai Lu wrote:
>> >>
>> >> fix for pv.
>> >>
>> >> Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
>> >>
>> >> ---
>> >>  arch/x86/kernel/paravirt.c |    5 ----
>> >>  arch/x86/kernel/vmi_32.c   |   51 ++++++++++++++++++++++++++++++++++++++++++---
>> >>  arch/x86/xen/enlighten.c   |   19 +++++++---------
>> >>  include/asm-x86/paravirt.h |   29 -------------------------
>> >>  4 files changed, 57 insertions(+), 47 deletions(-)
>> >>
>> >> Index: linux-2.6/arch/x86/kernel/paravirt.c
>> >> ===================================================================
>> >> --- linux-2.6.orig/arch/x86/kernel/paravirt.c
>> >> +++ linux-2.6/arch/x86/kernel/paravirt.c
>> >> @@ -373,11 +373,6 @@ struct pv_cpu_ops pv_cpu_ops = {
>> >>
>> >>  struct pv_apic_ops pv_apic_ops = {
>> >>  #ifdef CONFIG_X86_LOCAL_APIC
>> >> -#ifndef CONFIG_X86_64
>> >> -       .apic_write = native_apic_mem_write,
>> >> -       .apic_write_atomic = native_apic_mem_write_atomic,
>> >> -       .apic_read = native_apic_mem_read,
>> >> -#endif
>> >>         .setup_boot_clock = setup_boot_APIC_clock,
>> >>         .setup_secondary_clock = setup_secondary_APIC_clock,
>> >>         .startup_ipi_hook = paravirt_nop,
>> >> Index: linux-2.6/arch/x86/kernel/vmi_32.c
>> >> ===================================================================
>> >> --- linux-2.6.orig/arch/x86/kernel/vmi_32.c
>> >> +++ linux-2.6/arch/x86/kernel/vmi_32.c
>> >> @@ -676,6 +676,50 @@ static inline int __init probe_vmi_rom(v
>> >>         return 0;
>> >>  }
>> >>
>> >> +#ifdef CONFIG_X86_LOCAL_APIC
>> >> +static u32 vmi_apic_read(u32 reg)
>> >> +{
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static void vmi_apic_write(u32 reg, u32 val)
>> >> +{
>> >> +       /* Warn to see if there's any stray references */
>> >> +       WARN_ON(1);
>> >> +}
>> >> +
>> >> +static u64 vmi_apic_icr_read(void)
>> >> +{
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static void vmi_apic_icr_write(u32 low, u32 id)
>> >> +{
>> >> +       /* Warn to see if there's any stray references */
>> >> +       WARN_ON(1);
>> >> +}
>> >> +
>> >> +static void vmi_apic_wait_icr_idle(void)
>> >> +{
>> >> +       return;
>> >> +}
>> >> +
>> >> +static u32 vmi_safe_apic_wait_icr_idle(void)
>> >> +{
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static struct apic_ops vmi_basic_apic_ops = {
>> >> +        .read = vmi_apic_read,
>> >> +        .write = vmi_apic_write,
>> >> +        .write_atomic = vmi_apic_write,
>> >> +        .icr_read = vmi_apic_icr_read,
>> >> +        .icr_write = vmi_apic_icr_write,
>> >> +        .wait_icr_idle = vmi_apic_wait_icr_idle,
>> >> +        .safe_wait_icr_idle = vmi_safe_apic_wait_icr_idle,
>> >> +};
>> >> +#endif
>> >> +
>> >>  /*
>> >>   * VMI setup common to all processors
>> >>   */
>> >> @@ -904,9 +948,10 @@ static inline int __init activate_vmi(vo
>> >>  #endif
>> >>
>> >>  #ifdef CONFIG_X86_LOCAL_APIC
>> >> -       para_fill(pv_apic_ops.apic_read, APICRead);
>> >> -       para_fill(pv_apic_ops.apic_write, APICWrite);
>> >> -       para_fill(pv_apic_ops.apic_write_atomic, APICWrite);
>> >> +       para_fill(vmi_basic_apic_ops.read, APICRead);
>> >> +       para_fill(vmi_basic_apic_ops.write, APICWrite);
>> >> +       para_fill(vmi_basic_apic_ops.write_atomic, APICWrite);
>> >> +       apic_ops = &vmi_basic_apic_ops;
>> >
>> > Yinghai, Looking more closely at this, based on my understanding this might be
>> > wrong for VMI. Correct patch should be as follows. Any comments?
>>
>> so you mean icr related will still use default native member?
>>
>> YH
>
> Nacked-by: Zachary Amsden <zach@vmware.com>

because of not ccing you?

>
> What are you doing here and why aren't you cc-ing the maintainers?

did you checking tip tree for x86 changing?

YH

Re: Patch from LKML

[Zachary Amsden]

On Tue, 2008-07-15 at 11:52 -0700, Yinghai Lu wrote:

> > Nacked-by: Zachary Amsden <zach@vmware.com>
> 
> because of not ccing you?

Because it's wrong.

> >
> > What are you doing here and why aren't you cc-ing the maintainers?
> 
> did you checking tip tree for x86 changing?

No, this was brought to my attention by someone who spotted it.

Zach

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Zachary Amsden]

On Tue, 2008-07-15 at 11:51 -0700, Suresh Siddha wrote:
> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
> >
> > Nacked-by: Zachary Amsden <zach@vmware.com>
> >
> > What are you doing here and why aren't you cc-ing the maintainers?
> 
> Sorry. I was about to bring you into the loop.
> 
> Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
> is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix
> for VMI case aswell.
> 
> Based on my understanding, tip/x86/x2apic git commit
> 94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
> something like
> http://marc.info/?l=linux-kernel&m=121614328831237&w=2

Looks better, but I need to read more context to find out where the
apic_ops variable comes from; I'll read the list for patches.

You are correct in that we will want to use the same wait_icr_idle
routine as native hardware; it's not clear from just this patch how that
happens.

Also, the VMI operations are sensitive to parameter order because they
interface with an ABI at the other end.  I need to check the parameter
order for apic read / write is still consistent with the ABI.

Zach

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Yinghai Lu]

On Tue, Jul 15, 2008 at 12:04 PM, Zachary Amsden <zach@vmware.com> wrote:
> On Tue, 2008-07-15 at 11:51 -0700, Suresh Siddha wrote:
>> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
>> >
>> > Nacked-by: Zachary Amsden <zach@vmware.com>
>> >
>> > What are you doing here and why aren't you cc-ing the maintainers?
>>
>> Sorry. I was about to bring you into the loop.
>>
>> Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
>> is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix
>> for VMI case aswell.
>>
>> Based on my understanding, tip/x86/x2apic git commit
>> 94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
>> something like
>> http://marc.info/?l=linux-kernel&m=121614328831237&w=2
>
> Looks better, but I need to read more context to find out where the
> apic_ops variable comes from; I'll read the list for patches.
>
> You are correct in that we will want to use the same wait_icr_idle
> routine as native hardware; it's not clear from just this patch how that
> happens.
>
> Also, the VMI operations are sensitive to parameter order because they
> interface with an ABI at the other end.  I need to check the parameter
> order for apic read / write is still consistent with the ABI.

http://people.redhat.com/mingo/tip.git/readme.txt

mkdir linux-2.6
cd linux-2.6

git init

git remote add linus
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

git remote add tip
git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git

git remote update

git checkout -b tip-latest tip/master

git merge tip/x86/x2apic

YH

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Zachary Amsden]

On Tue, 2008-07-15 at 12:10 -0700, Yinghai Lu wrote:
> On Tue, Jul 15, 2008 at 12:04 PM, Zachary Amsden <zach@vmware.com> wrote:
> > On Tue, 2008-07-15 at 11:51 -0700, Suresh Siddha wrote:
> >> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
> >> >
> >> > Nacked-by: Zachary Amsden <zach@vmware.com>
> >> >
> >> > What are you doing here and why aren't you cc-ing the maintainers?
> >>
> >> Sorry. I was about to bring you into the loop.
> >>
> >> Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
> >> is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix
> >> for VMI case aswell.
> >>
> >> Based on my understanding, tip/x86/x2apic git commit
> >> 94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
> >> something like
> >> http://marc.info/?l=linux-kernel&m=121614328831237&w=2


Suresh's patch looks good.  Thanks Suresh!

Acked-by: Zachary Amsden <zach@vmware.com>

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Yinghai Lu]

On Tue, Jul 15, 2008 at 11:51 AM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
>>
>> Nacked-by: Zachary Amsden <zach@vmware.com>
>>
>> What are you doing here and why aren't you cc-ing the maintainers?
>
> Sorry. I was about to bring you into the loop.
>
> Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
> is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix
> for VMI case aswell.
>
> Based on my understanding, tip/x86/x2apic git commit
> 94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
> something like
> http://marc.info/?l=linux-kernel&m=121614328831237&w=2
>
> Can you please check and Ack/Nack this fix?

how about xen pv with icr related?

YH

Re: [PATCH] x86: let 32bit use apic_ops too - fix

[Zachary Amsden]

On Tue, 2008-07-15 at 12:22 -0700, Yinghai Lu wrote:
> On Tue, Jul 15, 2008 at 11:51 AM, Suresh Siddha
> <suresh.b.siddha@intel.com> wrote:
> > On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
> >>
> >> Nacked-by: Zachary Amsden <zach@vmware.com>
> >>
> >> What are you doing here and why aren't you cc-ing the maintainers?
> >
> > Sorry. I was about to bring you into the loop.
> >
> > Yinghai posted 32bit native apic_ops(similar to my 64bit apic ops patch, which
> > is different from pv_apic_ops) which is in tip/x86/x2apic and proposed a fix
> > for VMI case aswell.
> >
> > Based on my understanding, tip/x86/x2apic git commit
> > 94a8c3c2437c8946f1b6c8e0b2c560a7db8ed3c6 is wrong and it should be fixed with
> > something like
> > http://marc.info/?l=linux-kernel&m=121614328831237&w=2
> >
> > Can you please check and Ack/Nack this fix?
> 
> how about xen pv with icr related?

Xen doesn't have an APIC; they should be fine as is I think.

Zach