linux-kernel - Re: [git pull] "big box" x86 changes, PCI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <200804281334.16713.jbarnes@virtuousgeek.org>
Date:	Mon, 28 Apr 2008 13:34:15 -0700
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Yinghai Lu <yinghai.lu@....com>,
	Yinghai Lu <yhlu.kernel@...il.com>
Subject: Re: [git pull] "big box" x86 changes, PCI

On Saturday, April 26, 2008 2:55 pm Ingo Molnar wrote:
> @@ -184,51 +322,80 @@ static void __init pci_mmcfg_reject_broken(int type)
>  
>         cfg = &pci_mmcfg_config[0];
>  
> -       /*
> -        * Handle more broken MCFG tables on Asus etc.
> -        * They only contain a single entry for bus 0-0.
> -        */
> -       if (pci_mmcfg_config_num == 1 &&
> -           cfg->pci_segment == 0 &&
> -           (cfg->start_bus_number | cfg->end_bus_number) == 0) {
> -               printk(KERN_ERR "PCI: start and end of bus number is 0. "
> -                      "Rejected as broken MCFG.\n");
> -               goto reject;
> +       for (i = 0; i < pci_mmcfg_config_num; i++) {
> +               int valid = 0;
> +               u32 size = (cfg->end_bus_number + 1) << 20;
> +               cfg = &pci_mmcfg_config[i];
> +               printk(KERN_NOTICE "PCI: MCFG configuration %d: base %lx "
> +                      "segment %hu buses %u - %u\n",
> +                      i, (unsigned long)cfg->address, cfg->pci_segment,
> +                      (unsigned int)cfg->start_bus_number,
> +                      (unsigned int)cfg->end_bus_number);
> +
> +               if (!early &&
> +                   is_acpi_reserved(cfg->address, cfg->address + size -
> 1)) { +                       printk(KERN_NOTICE "PCI: MCFG area at %Lx
> reserved " +                              "in ACPI motherboard
> resources\n",
> +                              cfg->address);
> +                       valid = 1;
> +               }
> +
> +               if (valid)
> +                       continue;
> +
> +               if (!early)
> +                       printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %Lx is
> not" +                              " reserved in ACPI motherboard
> resources\n", +                              cfg->address);
> +               /* Don't try to do this check unless configuration
> +                  type 1 is available. how about type 2 ?*/
> +               if (raw_pci_ops && e820_all_mapped(cfg->address,
> +                                                 cfg->address + size - 1,
> +                                                 E820_RESERVED)) {
> +                       printk(KERN_NOTICE
> +                              "PCI: MCFG area at %Lx reserved in E820\n",
> +                              cfg->address);
> +                       valid = 1;
> +               }
> +
> +               if (!valid)
> +                       goto reject;
>         }

This loop is a bit messy, is there some way of making it clearer?  Maybe the 
early vs. late stuff should be split into separate routines entirely...

> @@ -842,11 +842,14 @@ static void set_pcie_port_type(struct pci_dev *pdev)
>   * reading the dword at 0x100 which must either be 0 or a valid extended
>   * capability header.
>   */
> -int pci_cfg_space_size(struct pci_dev *dev)
> +int pci_cfg_space_size_ext(struct pci_dev *dev, unsigned check_exp_pcix)
>  {
>         int pos;
>         u32 status;
>  
> +       if (!check_exp_pcix)
> +               goto skip;
> +

Rather than adding a flag to pci_cfg_space_size, you could either factor out 
the extended space probe into a separate routine and use it from both 
pci_cfg_space_size and the fixup code, or just make the fixup code do the 
probe & cfg_size setting by hand, moving the PCI_CFG_SPACE_SIZE and 
PCI_CFG_SPACE_EXP_SIZE to pci.h.

In both cases I'm just trying to avoid having a flag you pass to a routine 
that changes its behavior significantly enough that a new function would make 
things more readable.

Other than that, things look pretty good.  And the resulting kernel boots fine 
on my test box, which is nice...

Thanks,
Jesse