> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha
> <[email protected]> wrote:
> > On Sun, Jul 13, 2008 at 10:19:35PM -0700, Yinghai Lu wrote:
> >>
> >> fix for pv.
> >>
> >> Signed-off-by: Yinghai Lu <[email protected]>
> >>
> >> ---
> >> 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 <[email protected]>
What are you doing here and why aren't you cc-ing the maintainers?
Thanks,
Zach
On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
>
> Nacked-by: Zachary Amsden <[email protected]>
>
> 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
On Tue, Jul 15, 2008 at 11:38 AM, Zachary Amsden <[email protected]> wrote:
>
>> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha
>> <[email protected]> wrote:
>> > On Sun, Jul 13, 2008 at 10:19:35PM -0700, Yinghai Lu wrote:
>> >>
>> >> fix for pv.
>> >>
>> >> Signed-off-by: Yinghai Lu <[email protected]>
>> >>
>> >> ---
>> >> 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 <[email protected]>
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
On Tue, 2008-07-15 at 11:52 -0700, Yinghai Lu wrote:
> > Nacked-by: Zachary Amsden <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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
On Tue, Jul 15, 2008 at 12:04 PM, Zachary Amsden <[email protected]> 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 <[email protected]>
>> >
>> > 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
On Tue, 2008-07-15 at 12:10 -0700, Yinghai Lu wrote:
> On Tue, Jul 15, 2008 at 12:04 PM, Zachary Amsden <[email protected]> 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 <[email protected]>
> >> >
> >> > 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 <[email protected]>
On Tue, Jul 15, 2008 at 11:51 AM, Suresh Siddha
<[email protected]> wrote:
> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
>>
>> Nacked-by: Zachary Amsden <[email protected]>
>>
>> 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
On Tue, 2008-07-15 at 12:22 -0700, Yinghai Lu wrote:
> On Tue, Jul 15, 2008 at 11:51 AM, Suresh Siddha
> <[email protected]> wrote:
> > On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote:
> >>
> >> Nacked-by: Zachary Amsden <[email protected]>
> >>
> >> 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