[Xen-devel] Re: [PATCH] Fix xen hang on intel westmere-EP

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 22.08.11 at 15:55, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
> On Intel westmere-ep(w/ ICH10 chipset), the legacy USB logic will generate 
> SMI 
> consistently. And this will cause system hang under some specified 
> conditions. So we need to disable it during early boot.

NAK. See below.

> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> 
> diff -r 0f36c2eec2e1 xen/arch/x86/Makefile
> --- a/xen/arch/x86/Makefile     Thu Jul 28 15:40:54 2011 +0100
> +++ b/xen/arch/x86/Makefile     Mon Aug 22 16:23:16 2011 +0800
> @@ -57,6 +57,7 @@
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += xstate.o
> +obj-y += early_quirks.o
> 
>  obj-$(crash_debug) += gdbstub.o
> 
> diff -r 0f36c2eec2e1 xen/arch/x86/early_quirks.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/arch/x86/early_quirks.c       Mon Aug 22 16:23:16 2011 +0800
> @@ -0,0 +1,72 @@
> +/*
> + *  arch/x86/early_quirks.c
> + *
> + *  This code used to workaround the chipset bugs.
> + *
> + */
> +
> +#include <xen/init.h>
> +#include <xen/pci.h>
> +#include <xen/pci_regs.h>
> +#include <asm/io.h>
> +
> +/*
> + * For intel ich10 chipset, it has bug on deliver legacy usb
> + * SMI to CPU and will cause system hang. So we need to disable
> + * it when booting.
> + */
> +void intel_smi_quirk(
> +        unsigned int bus, unsigned int dev, unsigned int func) {
> +    unsigned int pmbase;
> +    unsigned int smi_ctrl_addr, smi_ctrl_reg;
> +
> +    /*
> +     * Smi control register reside at pmbase +30h. We need to find
> +     * the pmbase address firstly which located at offset 0x40.
> +     */
> +    pmbase = pci_conf_read32(bus, dev, func, 0x40);
> +
> +    smi_ctrl_addr = (pmbase & 0xff80) + 0x30;
> +    smi_ctrl_reg = inl(smi_ctrl_addr);
> +
> +    /*
> +     *mask bit 3 and bit 17 to disable legacy usb to generate SMI.
> +     */
> +    smi_ctrl_reg &= ~0x20008;
> +    outl(smi_ctrl_reg, smi_ctrl_addr);
> +}
> +
> +struct chipset {
> +    unsigned int vendor;
> +    unsigned int device;
> +    void (*f)(unsigned int bus, unsigned int dev, unsigned int func); 
> +};
> +
> +static struct chipset early_quirk[] = {
> +    {PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_ICH10_LPC, intel_smi_quirk},

I think we discussed (off-list) quite extensively that this should not be
keyed to the PCI device IDs. It ought to hang off the BIOS and/or
board manufacturer. I actually have a patch that does just so, but
am waiting for possibly more fine grained identification information
from you (Intel).

Also, are you certain this problem exists only with this single ICH
variant?

> +    {}
> +};
> +
> +static void check_quirk(unsigned int bus, unsigned int dev, unsigned 
> +int func) {
> +    unsigned int vendor_id, device_id;
> +    int i;
> +
> +    vendor_id = pci_conf_read16(bus, dev, func, PCI_VENDOR_ID);
> +    device_id = pci_conf_read16(bus, dev, func, PCI_DEVICE_ID);
> +
> +    for (i = 0; early_quirk[i].f != NULL; i++)
> +        if (early_quirk[i].vendor == vendor_id &&
> +            early_quirk[i].device == device_id)
> +                early_quirk[i].f(bus, dev, func); }
> +
> +void  __init early_quirks(void)
> +{
> +    unsigned int dev, func;
> +
> +    for (dev = 0; dev < 32; dev++)
> +        for (func = 0; func < 8; func++)
> +            check_quirk(0, dev, func);

Further I'm opposed to introducing further instances of legacy brute-
force PCI bus scans.

And I don't think you got something along these lines accepted into
Linux, did you? It ought to be DMI based there, too.

Jan

> +}
> diff -r 0f36c2eec2e1 xen/arch/x86/setup.c
> --- a/xen/arch/x86/setup.c      Thu Jul 28 15:40:54 2011 +0100
> +++ b/xen/arch/x86/setup.c      Mon Aug 22 16:23:16 2011 +0800
> @@ -1255,6 +1255,8 @@
>      if ( opt_nosmp )
>          max_cpus = 0;
> 
> +    early_quirks();
> +
>      iommu_setup();    /* setup iommu if available */
> 
>      smp_prepare_cpus(max_cpus);
> diff -r 0f36c2eec2e1 xen/include/asm-x86/setup.h
> --- a/xen/include/asm-x86/setup.h       Thu Jul 28 15:40:54 2011 +0100
> +++ b/xen/include/asm-x86/setup.h       Mon Aug 22 16:23:16 2011 +0800
> @@ -12,6 +12,7 @@
>  void early_cpu_init(void);
>  void early_time_init(void);
>  void early_page_fault(void);
> +void early_quirks(void);
> 
>  int intel_cpu_init(void);
>  int amd_init_cpu(void);
> diff -r 0f36c2eec2e1 xen/include/xen/pci_regs.h
> --- a/xen/include/xen/pci_regs.h        Thu Jul 28 15:40:54 2011 +0100
> +++ b/xen/include/xen/pci_regs.h        Mon Aug 22 16:23:16 2011 +0800
> @@ -22,6 +22,9 @@
>  #ifndef LINUX_PCI_REGS_H
>  #define LINUX_PCI_REGS_H
> 
> +#define PCI_VENDOR_ID_INTEL         0x8086
> +#define PCI_DEVICE_ID_ICH10_LPC     0x3a16
> +
>  /*
>   * Under PCI, each device has 256 bytes of configuration address space,
>   * of which the first 64 bytes are standardized as follows:



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel