Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

["Yinghai Lu" <yhlu.kernel@gmail.com>]

On Thu, Mar 20, 2008 at 9:30 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> Ravikiran G Thirumalai wrote:
>  > - Fix the the build breakage when PARAVIRT is defined
>  >   but PCI is not
>  >   This fixes problem reported at:
>  >       http://marc.info/?l=linux-kernel&m=120525966600698&w=2
>  > - Make is_vsmp_box() available even when PARAVIRT is not defined.
>  >   This is needed to determine if tsc's are reliable as a time source
>  >   even when PARAVIRT is not defined.
>  > - split vsmp_init to use is_vsmp_box() and set_vsmp_pv_ops()
>  >   set_vsmp_pv_ops will do nothing if PCI is not enabled in the config.
>  >
>
>  I'm a bit confused by all the config dependencies.  I had assumed that
>  VSMP depended on both PCI and PARAVIRT.  Is this not actually true?  You
>  can have a VSMP system without either or both of PCI/PARAVIRT?
>
>  The structure of the code suggests that at the very least VSMP depends
>  on PCI (it never makes sense to enable VSMP on a non-PCI system), and
>  therefore a number of your config decisions can just be predicated on VSMP.
>
>
>  > Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
>  >
>  > Index: linux.git.trees/arch/x86/kernel/Makefile
>  > ===================================================================
>  > --- linux.git.trees.orig/arch/x86/kernel/Makefile     2008-03-19 13:39:29.000000000 -0700
>  > +++ linux.git.trees/arch/x86/kernel/Makefile  2008-03-19 13:46:14.400935607 -0700
>  > @@ -60,7 +60,7 @@ obj-$(CONFIG_KEXEC)         += relocate_kernel_
>  >  obj-$(CONFIG_CRASH_DUMP)     += crash_dump_$(BITS).o
>  >  obj-$(CONFIG_X86_NUMAQ)              += numaq_32.o
>  >  obj-$(CONFIG_X86_SUMMIT_NUMA)        += summit_32.o
>  > -obj-$(CONFIG_PARAVIRT)               += vsmp_64.o
>  > +obj-y                                += vsmp_64.o
>  >
>
>  Couldn't this be make obj-$(CONFIG_X86_VSMP)?
>
>
>
>  >  obj-$(CONFIG_KPROBES)                += kprobes.o
>  >  obj-$(CONFIG_MODULES)                += module_$(BITS).o
>  >  obj-$(CONFIG_ACPI_SRAT)      += srat_32.o
>  > Index: linux.git.trees/arch/x86/kernel/setup_64.c
>  > ===================================================================
>  > --- linux.git.trees.orig/arch/x86/kernel/setup_64.c   2008-03-19 13:39:29.000000000 -0700
>  > +++ linux.git.trees/arch/x86/kernel/setup_64.c        2008-03-19 13:57:43.951337318 -0700
>  > @@ -353,9 +353,7 @@ void __init setup_arch(char **cmdline_p)
>  >       if (efi_enabled)
>  >               efi_init();
>  >
>  > -#ifdef       CONFIG_PARAVIRT
>  >       vsmp_init();
>  > -#endif
>  >
>  >       dmi_scan_machine();
>  >
>  > Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
>  > ===================================================================
>  > --- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c    2008-03-19 13:39:50.000000000 -0700
>  > +++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700
>  > @@ -19,6 +19,7 @@
>  >  #include <asm/io.h>
>  >  #include <asm/paravirt.h>
>  >
>  > +#if defined CONFIG_PCI && defined CONFIG_PARAVIRT
>  >  /*
>  >   * Interrupt control on vSMPowered systems:
>  >   * ~AC is a shadow of IF.  If IF is 'on' AC should be 'off'
>  > @@ -72,39 +73,11 @@ static unsigned __init vsmp_patch(u8 typ
>  >
>  >  }
>  >
>  > -static int vsmp = -1;
>  > -
>  > -int is_vsmp_box(void)
>  > -{
>  > -     if (vsmp != -1)
>  > -             return vsmp;
>  > -
>  > -     vsmp = 0;
>  > -     if (!early_pci_allowed())
>  > -             return vsmp;
>  > -
>  > -     /* Check if we are running on a ScaleMP vSMP box */
>  > -     if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
>  > -          PCI_VENDOR_ID_SCALEMP) &&
>  > -         (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
>  > -         PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
>  > -             vsmp = 1;
>  > -
>  > -     return vsmp;
>  > -}
>  > -
>  > -void __init vsmp_init(void)
>  > +static void __init set_vsmp_pv_ops(void)
>  >  {
>  >       void *address;
>  >       unsigned int cap, ctl, cfg;
>  >
>  > -     if (!is_vsmp_box())
>  > -             return;
>  > -
>  > -     if (!early_pci_allowed())
>  > -             return;
>  > -
>  > -     /* If we are, use the distinguished irq functions */
>  >       pv_irq_ops.irq_disable = vsmp_irq_disable;
>  >       pv_irq_ops.irq_enable  = vsmp_irq_enable;
>  >       pv_irq_ops.save_fl  = vsmp_save_fl;
>  > @@ -127,5 +100,46 @@ void __init vsmp_init(void)
>  >       }
>  >
>  >       early_iounmap(address, 8);
>  > +}
>  > +#else
>  > +static void __init set_vsmp_pv_ops(void)
>  > +{
>  > +}
>  > +#endif
>  > +
>  > +#ifdef CONFIG_PCI
>  > +static int vsmp = -1;
>  > +
>  > +int is_vsmp_box(void)
>  > +{
>  > +     if (vsmp != -1)
>  > +             return vsmp;
>  > +
>  > +     vsmp = 0;
>  > +     if (!early_pci_allowed())
>  > +             return vsmp;
>  > +
>  > +     /* Check if we are running on a ScaleMP vSMP box */
>  > +     if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
>  > +          PCI_VENDOR_ID_SCALEMP) &&
>  > +         (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
>  > +         PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
>  > +             vsmp = 1;
>  > +
>  > +     return vsmp;
>  > +}
>  > +#else
>  > +int is_vsmp_box(void)
>  > +{
>  > +     return 0;
>  > +}
>  > +#endif
>  >
>
>  Rather than doing this, I'd propose putting
>
>  #ifndef CONFIG_X86_VSMP
>  static inline int is_vsmp_box(void)
>  {
>     return 0;
>  }
>  #endif
>
>  in a header, and then making the definition in vsmp_64.c unconditional
>  (or rather, the whole of vsmp_64.o conditional on CONFIG_X86_VSMP).
>
>
>  > +
>  > +void __init vsmp_init(void)
>  > +{
>  > +     if (!is_vsmp_box())
>  > +             return;
>  > +
>  > +     set_vsmp_pv_ops();
>  >       return;
>  >  }
>  >
>
>  Similarly with this.
>
>
>  > Index: linux.git.trees/include/asm-x86/apic.h
>  > ===================================================================
>  > --- linux.git.trees.orig/include/asm-x86/apic.h       2008-03-19 13:39:29.000000000 -0700
>  > +++ linux.git.trees/include/asm-x86/apic.h    2008-03-19 13:57:14.550893819 -0700
>  > @@ -51,19 +51,16 @@ extern unsigned boot_cpu_id;
>  >   */
>  >  #ifdef CONFIG_PARAVIRT
>  >  #include <asm/paravirt.h>
>  > -extern int is_vsmp_box(void);
>  >  #else
>  >  #define apic_write native_apic_write
>  >  #define apic_write_atomic native_apic_write_atomic
>  >  #define apic_read native_apic_read
>  >  #define setup_boot_clock setup_boot_APIC_clock
>  >  #define setup_secondary_clock setup_secondary_APIC_clock
>  > -static int inline is_vsmp_box(void)
>  > -{
>  > -     return 0;
>  > -}
>  >  #endif
>  >
>  > +extern int is_vsmp_box(void);
>  > +
>  >
>
>  This was almost right, except it should have been in a #ifdef
>  CONFIG_X86_VSMP block.
>
>
agreed

http://lkml.org/lkml/2008/3/18/76

YH