linux-kernel - Re: [PATCH v8 07/16] x86/pci: Add Hygon Dhyana support to PCI and north bridge

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: <20180924152448.GE20187@zn.tnic>
Date:   Mon, 24 Sep 2018 17:24:48 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Pu Wen <puwen@...on.cn>
Cc:     bhelgaas@...gle.com, tglx@...utronix.de, mingo@...hat.com,
        hpa@...or.com, x86@...nel.org, thomas.lendacky@....com,
        helgaas@...nel.org, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v8 07/16] x86/pci: Add Hygon Dhyana support to PCI and
 north bridge

On Sun, Sep 23, 2018 at 05:35:13PM +0800, Pu Wen wrote:
> As Hygon registered its PCI Vendor ID as a new one 0x1d94, and there
> are PCI Devices 0x1450/0x1463/0x1464 for Host bridge on Hygon Dhyana
> platform, so add Hygon Dhyana support to the PCI and north bridge
> subsystem by using the code path of AMD family 17h.
> 
> To prevent further checking PCI device ids which cannot happen both on
> Hygon and Intel platform, the function amd_gart_present should return
> if it's not a AMD CPU.
> 
> Signed-off-by: Pu Wen <puwen@...on.cn>
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>	# pci_ids.h
> Reviewed-by: Borislav Petkov <bp@...e.de>
> ---
>  arch/x86/include/asm/amd_nb.h |  3 +++
>  arch/x86/kernel/amd_nb.c      | 47 +++++++++++++++++++++++++++++++++++++------
>  arch/x86/pci/amd_bus.c        |  6 ++++--
>  include/linux/pci_ids.h       |  2 ++
>  4 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index fddb6d2..1ae4e57 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -103,6 +103,9 @@ static inline u16 amd_pci_dev_to_node_id(struct pci_dev *pdev)
>  
>  static inline bool amd_gart_present(void)
>  {
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +		return false;
> +

What is that for?

Hygon doesn't have F15h so that function will return false there too...

... or is that because the qemu script you got from the 0day bot guys
uses -cpu kvm64 which is family 0xf:

[    0.214353] smpboot: CPU0: AMD Common KVM processor (family: 0xf, model: 0x6, stepping: 0x1)

?

and that makes amd_gart_present() say yes.

In that case, please make a *prepatch* which adds the vendor check to
both amd_gart_present() and early_is_amd_nb() and send it as a reply to
this message.

*Then*, do this patch ontop and also as a reply.

>  	/* GART present only on Fam15h, upto model 0fh */
>  	if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
>  	    (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index b481b95..dcc4130 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -61,6 +61,21 @@ static const struct pci_device_id amd_nb_link_ids[] = {
>  	{}
>  };
>  
> +static const struct pci_device_id hygon_root_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_ROOT) },
> +	{}
> +};
> +
> +const struct pci_device_id hygon_nb_misc_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_DF_F3) },
> +	{}
> +};
> +
> +static const struct pci_device_id hygon_nb_link_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_DF_F4) },
> +	{}
> +};
> +
>  const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[] __initconst = {
>  	{ 0x00, 0x18, 0x20 },
>  	{ 0xff, 0x00, 0x20 },
> @@ -197,12 +212,25 @@ int amd_cache_northbridges(void)
>  	u16 i = 0;
>  	struct amd_northbridge *nb;
>  	struct pci_dev *root, *misc, *link;
> +	const struct pci_device_id *root_ids = NULL;
> +	const struct pci_device_id *misc_ids = NULL;
> +	const struct pci_device_id *link_ids = NULL;
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> +		root_ids = hygon_root_ids;
> +		misc_ids = hygon_nb_misc_ids;
> +		link_ids = hygon_nb_link_ids;
> +	} else {
> +		root_ids = amd_root_ids;
> +		misc_ids = amd_nb_misc_ids;
> +		link_ids = amd_nb_link_ids;
> +	}

Also, you can make this assignment differently:

	const struct pci_device_id *root_ids = amd_root_ids;
	const struct pci_device_id *misc_ids = amd_nb_misc_ids;
	const struct pci_device_id *link_ids = amd_nb_link_ids;


	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
		root_ids = hygon_root_ids;
		misc_ids = hygon_nb_misc_ids;
		link_ids = hygon_nb_link_ids;
	}

This way the change is obvious and it is only for Hygon without
affecting the other vendors.

Ditto for the other assignment.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.