Re: [PATCH] kexec: force x86_64 arches to boot kdump kernels on boot cpu

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

On Dec 10, 2007 8:48 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
>
> Almost there.
>
>
>
> > On Mon, Dec 10, 2007 at 06:08:03PM -0700, Eric W. Biederman wrote:
> >> Neil Horman <nhorman@tuxdriver.com> writes:
> >>
> > <snip>
> >>
> >> Ok.  This test is broken.  Please remove the == 1.  You are looking
> >> for == (1 << 18).  So just saying: "if (htcfg & (1 << 18))" should be clearer.
> >>
> > Fixed.  Thanks!
> >
> >> > + printk(KERN_INFO "Detected use of extended apic ids on hypertransport
> > bus\n");
> >> > +          if ((htcfg & (1 << 17)) == 0) {
> >> > + printk(KERN_INFO "Enabling hypertransport extended apic interrupt
> >> > broadcast\n");
> >> > +                  htcfg |= (1 << 17);
> >> > +                  write_pci_config(num, slot, func, 0x68, htcfg);
> >> > +          }
> >> > +  }
> >> > +
> >> > +}
> >>
> >> The rest of this quirk looks fine, include the fact it is only intended
> >> to be applied to PCI_VENDOR_ID_AMD PCI_DEVICE_ID_AMD_K8_NB.
> >>
> > Copy that.
> >
> >>
> >> For what is below I don't like the way the infrastructure has been
> >> extended as what you are doing quickly devolves into a big mess.
> >>
> >> Please extend struct chipset to be something like:
> >> struct chipset {
> >>      u16 vendor;
> >>      u16 device;
> >>         u32 class, class_mask;
> >>      void (*f)(void);
> >> };
> >>
> >> And then the test for matching the chipset can be something like:
> >>      if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> >>          (id->device == PCI_ANY_ID || id->device == dev->device) &&
> >>          !((id->class ^ dev->class) & id->class_mask))
> >>
> >> Essentially a subset of pci_match_one_device from drivers/pci/pci.h
> >>
> >> That way you don't need to increase the number of tables or the
> >> number of passes through the pci busses, just update the early_qrk
> >> table with a few more bits of information.
> >>
> > copy that.  Fixed.  Thanks!
> >
> >> The extended form should be much more maintainable in the long
> >> run.  Given that we may want this before we enable the timer
> >> which is very early doing this in the pci early quirks seems
> >> to make sense.
> >>
> >> Eric
> >
> >
> > New patch attached, with suggestions incorporated.
> >
> > Thanks & regards
> > Neil
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >
> >
> >  early-quirks.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 73 insertions(+), 9 deletions(-)
> >
> >
> >
> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index 88bb83e..4b0cee1 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -44,6 +44,50 @@ static int __init nvidia_hpet_check(struct acpi_table_header
> > *header)
> >  #endif /* CONFIG_X86_IO_APIC */
> >  #endif /* CONFIG_ACPI */
> >
> > +static void __init fix_hypertransport_config(int num, int slot, int func)
> > +{
> > +     u32 htcfg;
> > +     /*
> > +      *we found a hypertransport bus
> > +      *make sure that are broadcasting
> > +      *interrupts to all cpus on the ht bus
> > +      *if we're using extended apic ids
> > +      */
> > +     htcfg = read_pci_config(num, slot, func, 0x68);
> > +     if (htcfg & (1 << 18)) {
> > + printk(KERN_INFO "Detected use of extended apic ids on hypertransport bus\n");
> > +             if ((htcfg & (1 << 17)) == 0) {
> > + printk(KERN_INFO "Enabling hypertransport extended apic interrupt
> > broadcast\n");
> > +                     htcfg |= (1 << 17);
> > +                     write_pci_config(num, slot, func, 0x68, htcfg);
> > +             }
> > +     }
> > +
> > +}
> > +
> > +static void __init check_hypertransport_config()
> > +{
> > +     int num, slot, func;
> > +     u32 device, vendor;
> > +     func = 0;
> > +     for (num = 0; num < 32; num++) {
> > +             for (slot = 0; slot < 32; slot++) {
> > +                     vendor = read_pci_config(num,slot,func,
> > +                                             PCI_VENDOR_ID);
> > +                     device = read_pci_config(num,slot,func,
> > +                                             PCI_DEVICE_ID);
> > +                     vendor &= 0x0000ffff;
> > +                     device >>= 16;
> > +                     if ((vendor == PCI_VENDOR_ID_AMD) &&
> > +                         (device == PCI_DEVICE_ID_AMD_K8_NB))
> > +                             fix_hypertransport_config(num,slot,func);
> > +             }
> > +     }
> > +
> > +     return;
> > +
> > +}
>
> We should not need check_hypertransport_config as the generic loop
> now does the work for us.
> > +
> >  static void __init nvidia_bugs(void)
> >  {
> >  #ifdef CONFIG_ACPI
> > @@ -83,15 +127,25 @@ static void __init ati_bugs(void)
> >  #endif
> >  }
> >
> > +static void __init amd_host_bugs(void)
> > +{
> > +     printk(KERN_CRIT "IN AMD_HOST_BUGS\n");
> > +     check_hypertransport_config();
> > +}
>
> Likewise this function is unneeded and the printk is likely confusing
> for users.
>
> >  struct chipset {
> >       u16 vendor;
> > +     u16 device;
> > +     u32 class;
> > +     u32 class_mask;
> >       void (*f)(void);
> >  };
> >
> >  static struct chipset early_qrk[] __initdata = {
> > -     { PCI_VENDOR_ID_NVIDIA, nvidia_bugs },
> > -     { PCI_VENDOR_ID_VIA, via_bugs },
> > -     { PCI_VENDOR_ID_ATI, ati_bugs },
> > + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID,
> > nvidia_bugs },
> > + { PCI_VENDOR_ID_VIA, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, via_bugs },
> > + { PCI_VENDOR_ID_ATI, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, ati_bugs },
> > + { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB, PCI_CLASS_BRIDGE_HOST,
> > PCI_ANY_ID, amd_host_bugs },
> >       {}
> So make that fix_hypertransport_config and we should be good.

Agreed.

struct chipset {
     u16 vendor;
    u16 device;
    u32 class;
+     u32 class_mask;
      void (*f)(void); ============> int (*f) (int num, int slot, int func);
 };

> >  };
> >
> > @@ -108,25 +162,35 @@ void __init early_quirks(void)
> >                       for (func = 0; func < 8; func++) {
> >                               u32 class;
> >                               u32 vendor;
> > +                             u32 device;
> >                               u8 type;
> >                               int i;
> > +
> >                               class = read_pci_config(num,slot,func,
> >                                                       PCI_CLASS_REVISION);
> >                               if (class == 0xffffffff)
> >                                       break;
> >
> > -                             if ((class >> 16) != PCI_CLASS_BRIDGE_PCI)
> > -                                     continue;
> > +                             class >>= 16;
>
> >
> >                               vendor = read_pci_config(num, slot, func,
> >                                                        PCI_VENDOR_ID);
> >                               vendor &= 0xffff;
> >
> > -                             for (i = 0; early_qrk[i].f; i++)
> > -                                     if (early_qrk[i].vendor == vendor) {
> > -                                             early_qrk[i].f();
> > -                                             return;
> > +                             device = read_pci_config(num, slot, func,
> > +                                                     PCI_DEVICE_ID);
> > +                             device >>= 16;
>
> We don't need to shift device.  Although we can do:
> device_vendor = read_pci_config(num, slot, func, PCI_VENDOR_ID);
> device = device_vendor >> 16;
> vendor = device_vendor & 0xffff;
>
> Assuming the early read_pci_config is limited to 32bit reads.
>
>
> > +                             for(i=0;early_qrk[i].f != NULL;i++) {
> > + if (((early_qrk[i].vendor == PCI_ANY_ID) ||
> > +                                         (early_qrk[i].vendor == vendor)) &&
> > + ((early_qrk[i].device == PCI_ANY_ID) ||
> > +                                         (early_qrk[i].device == device)) &&
> > +                                        ((early_qrk[i].class ^ class) &
> > +                                          early_qrk[i].class_mask)) {
> > +                                             early_qrk[i].f();
> >                                       }

> > +                                             early_qrk[i].f();
===>
int status;
status = early_qrk[i].f(num, slot, bus);
if (status == 1)
       break;
else if (status == 2)
       return;


YH