> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha > <suresh.b.siddha at 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 at 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? > > YHNacked-by: Zachary Amsden <zach at vmware.com> 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 <zach at 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
On Tue, Jul 15, 2008 at 11:38 AM, Zachary Amsden <zach at vmware.com> wrote:> >> On Tue, Jul 15, 2008 at 10:33 AM, Suresh Siddha >> <suresh.b.siddha at 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 at 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 at 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
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 at 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=2Looks 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 11:51 AM, Suresh Siddha <suresh.b.siddha at intel.com> wrote:> On Tue, Jul 15, 2008 at 11:38:50AM -0700, Zachary Amsden wrote: >> >> Nacked-by: Zachary Amsden <zach at 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