| 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
| ||
|
Message-ID: <468a6024-0431-1fec-7f27-36e440070a3f@linux.intel.com>
Date: Mon, 9 Dec 2024 15:35:25 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Yazen Ghannam <yazen.ghannam@....com>
cc: x86@...nel.org, Tony Luck <tony.luck@...el.com>,
Mario Limonciello <mario.limonciello@....com>,
Bjorn Helgaas <bhelgaas@...gle.com>, Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>, Clemens Ladisch <clemens@...isch.de>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Hans de Goede <hdegoede@...hat.com>,
Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>,
Suma Hegde <suma.hegde@....com>, LKML <linux-kernel@...r.kernel.org>,
linux-edac@...r.kernel.org, linux-pci@...r.kernel.org,
linux-hwmon@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2 10/16] x86/amd_nb: Move SMN access code to a new
amd_node driver
On Fri, 6 Dec 2024, Yazen Ghannam wrote:
> From: Mario Limonciello <mario.limonciello@....com>
>
> SMN access was bolted into amd_nb mostly as convenience. This has
> limitations though that require incurring tech debt to keep it working.
>
> Move SMN access to the newly introduced AMD Node driver.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> ---
>
> Notes:
> Link:
> https://lore.kernel.org/20241023172150.659002-11-yazen.ghannam@amd.com
>
> v1->v2:
> * Move code to AMD_NODE rather than create a new compilation unit.
>
> MAINTAINERS | 1 +
> arch/x86/include/asm/amd_nb.h | 3 -
> arch/x86/include/asm/amd_node.h | 3 +
> arch/x86/kernel/amd_nb.c | 89 ---------------------------
> arch/x86/kernel/amd_node.c | 90 ++++++++++++++++++++++++++++
> arch/x86/pci/fixup.c | 4 +-
> drivers/edac/Kconfig | 1 +
> drivers/edac/amd64_edac.c | 1 +
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/k10temp.c | 2 +-
> drivers/platform/x86/amd/pmc/Kconfig | 2 +-
> drivers/platform/x86/amd/pmc/pmc.c | 3 +-
> drivers/platform/x86/amd/pmf/Kconfig | 2 +-
> drivers/platform/x86/amd/pmf/core.c | 2 +-
For pdx86,
Acked-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
--
i.
> drivers/ras/amd/atl/Kconfig | 1 +
> drivers/ras/amd/atl/internal.h | 1 +
> 16 files changed, 107 insertions(+), 100 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d7617f196bda..ca383cb1539f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1122,6 +1122,7 @@ S: Supported
> F: drivers/i2c/busses/i2c-amd-asf-plat.c
>
> AMD NODE DRIVER
> +M: Mario Limonciello <mario.limonciello@....com>
> M: Yazen Ghannam <yazen.ghannam@....com>
> L: linux-kernel@...r.kernel.org
> S: Supported
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index 066dc3801430..4c4efb93045e 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -21,9 +21,6 @@ extern int amd_numa_init(void);
> extern int amd_get_subcaches(int);
> extern int amd_set_subcaches(int, unsigned long);
>
> -int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
> -int __must_check amd_smn_write(u16 node, u32 address, u32 value);
> -
> struct amd_l3_cache {
> unsigned indices;
> u8 subcaches[4];
> diff --git a/arch/x86/include/asm/amd_node.h b/arch/x86/include/asm/amd_node.h
> index 419a0ad13ef2..113ad3e8ee40 100644
> --- a/arch/x86/include/asm/amd_node.h
> +++ b/arch/x86/include/asm/amd_node.h
> @@ -30,4 +30,7 @@ static inline u16 amd_num_nodes(void)
> return topology_amd_nodes_per_pkg() * topology_max_packages();
> }
>
> +int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
> +int __must_check amd_smn_write(u16 node, u32 address, u32 value);
> +
> #endif /*_ASM_X86_AMD_NODE_H_*/
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 2ad67efe9032..2729e99806ec 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -15,9 +15,6 @@
> #include <linux/pci_ids.h>
> #include <asm/amd_nb.h>
>
> -/* Protect the PCI config register pairs used for SMN. */
> -static DEFINE_MUTEX(smn_mutex);
> -
> static u32 *flush_words;
>
> static const struct pci_device_id amd_nb_misc_ids[] = {
> @@ -59,92 +56,6 @@ struct amd_northbridge *node_to_amd_nb(int node)
> }
> EXPORT_SYMBOL_GPL(node_to_amd_nb);
>
> -/*
> - * SMN accesses may fail in ways that are difficult to detect here in the called
> - * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
> - * their own checking based on what behavior they expect.
> - *
> - * For SMN reads, the returned value may be zero if the register is Read-as-Zero.
> - * Or it may be a "PCI Error Response", e.g. all 0xFFs. The "PCI Error Response"
> - * can be checked here, and a proper error code can be returned.
> - *
> - * But the Read-as-Zero response cannot be verified here. A value of 0 may be
> - * correct in some cases, so callers must check that this correct is for the
> - * register/fields they need.
> - *
> - * For SMN writes, success can be determined through a "write and read back"
> - * However, this is not robust when done here.
> - *
> - * Possible issues:
> - *
> - * 1) Bits that are "Write-1-to-Clear". In this case, the read value should
> - * *not* match the write value.
> - *
> - * 2) Bits that are "Read-as-Zero"/"Writes-Ignored". This information cannot be
> - * known here.
> - *
> - * 3) Bits that are "Reserved / Set to 1". Ditto above.
> - *
> - * Callers of amd_smn_write() should do the "write and read back" check
> - * themselves, if needed.
> - *
> - * For #1, they can see if their target bits got cleared.
> - *
> - * For #2 and #3, they can check if their target bits got set as intended.
> - *
> - * This matches what is done for RDMSR/WRMSR. As long as there's no #GP, then
> - * the operation is considered a success, and the caller does their own
> - * checking.
> - */
> -static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)
> -{
> - struct pci_dev *root;
> - int err = -ENODEV;
> -
> - if (node >= amd_northbridges.num)
> - goto out;
> -
> - root = node_to_amd_nb(node)->root;
> - if (!root)
> - goto out;
> -
> - mutex_lock(&smn_mutex);
> -
> - err = pci_write_config_dword(root, 0x60, address);
> - if (err) {
> - pr_warn("Error programming SMN address 0x%x.\n", address);
> - goto out_unlock;
> - }
> -
> - err = (write ? pci_write_config_dword(root, 0x64, *value)
> - : pci_read_config_dword(root, 0x64, value));
> -
> -out_unlock:
> - mutex_unlock(&smn_mutex);
> -
> -out:
> - return err;
> -}
> -
> -int __must_check amd_smn_read(u16 node, u32 address, u32 *value)
> -{
> - int err = __amd_smn_rw(node, address, value, false);
> -
> - if (PCI_POSSIBLE_ERROR(*value)) {
> - err = -ENODEV;
> - *value = 0;
> - }
> -
> - return err;
> -}
> -EXPORT_SYMBOL_GPL(amd_smn_read);
> -
> -int __must_check amd_smn_write(u16 node, u32 address, u32 value)
> -{
> - return __amd_smn_rw(node, address, &value, true);
> -}
> -EXPORT_SYMBOL_GPL(amd_smn_write);
> -
> static int amd_cache_northbridges(void)
> {
> struct amd_northbridge *nb;
> diff --git a/arch/x86/kernel/amd_node.c b/arch/x86/kernel/amd_node.c
> index 4eea8c7d8090..95e5ca0acc90 100644
> --- a/arch/x86/kernel/amd_node.c
> +++ b/arch/x86/kernel/amd_node.c
> @@ -8,6 +8,7 @@
> * Author: Yazen Ghannam <Yazen.Ghannam@....com>
> */
>
> +#include <asm/amd_nb.h>
> #include <asm/amd_node.h>
>
> /*
> @@ -88,3 +89,92 @@ struct pci_dev *amd_node_get_root(u16 node)
> pci_dbg(root, "is root for AMD node %u\n", node);
> return root;
> }
> +
> +/* Protect the PCI config register pairs used for SMN. */
> +static DEFINE_MUTEX(smn_mutex);
> +
> +/*
> + * SMN accesses may fail in ways that are difficult to detect here in the called
> + * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
> + * their own checking based on what behavior they expect.
> + *
> + * For SMN reads, the returned value may be zero if the register is Read-as-Zero.
> + * Or it may be a "PCI Error Response", e.g. all 0xFFs. The "PCI Error Response"
> + * can be checked here, and a proper error code can be returned.
> + *
> + * But the Read-as-Zero response cannot be verified here. A value of 0 may be
> + * correct in some cases, so callers must check that this correct is for the
> + * register/fields they need.
> + *
> + * For SMN writes, success can be determined through a "write and read back"
> + * However, this is not robust when done here.
> + *
> + * Possible issues:
> + *
> + * 1) Bits that are "Write-1-to-Clear". In this case, the read value should
> + * *not* match the write value.
> + *
> + * 2) Bits that are "Read-as-Zero"/"Writes-Ignored". This information cannot be
> + * known here.
> + *
> + * 3) Bits that are "Reserved / Set to 1". Ditto above.
> + *
> + * Callers of amd_smn_write() should do the "write and read back" check
> + * themselves, if needed.
> + *
> + * For #1, they can see if their target bits got cleared.
> + *
> + * For #2 and #3, they can check if their target bits got set as intended.
> + *
> + * This matches what is done for RDMSR/WRMSR. As long as there's no #GP, then
> + * the operation is considered a success, and the caller does their own
> + * checking.
> + */
> +static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)
> +{
> + struct pci_dev *root;
> + int err = -ENODEV;
> +
> + if (node >= amd_nb_num())
> + goto out;
> +
> + root = node_to_amd_nb(node)->root;
> + if (!root)
> + goto out;
> +
> + mutex_lock(&smn_mutex);
> +
> + err = pci_write_config_dword(root, 0x60, address);
> + if (err) {
> + pr_warn("Error programming SMN address 0x%x.\n", address);
> + goto out_unlock;
> + }
> +
> + err = (write ? pci_write_config_dword(root, 0x64, *value)
> + : pci_read_config_dword(root, 0x64, value));
> +
> +out_unlock:
> + mutex_unlock(&smn_mutex);
> +
> +out:
> + return err;
> +}
> +
> +int __must_check amd_smn_read(u16 node, u32 address, u32 *value)
> +{
> + int err = __amd_smn_rw(node, address, value, false);
> +
> + if (PCI_POSSIBLE_ERROR(*value)) {
> + err = -ENODEV;
> + *value = 0;
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(amd_smn_read);
> +
> +int __must_check amd_smn_write(u16 node, u32 address, u32 value)
> +{
> + return __amd_smn_rw(node, address, &value, true);
> +}
> +EXPORT_SYMBOL_GPL(amd_smn_write);
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 0681ecfe3430..592fb9d97e77 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -9,7 +9,7 @@
> #include <linux/pci.h>
> #include <linux/suspend.h>
> #include <linux/vgaarb.h>
> -#include <asm/amd_nb.h>
> +#include <asm/amd_node.h>
> #include <asm/hpet.h>
> #include <asm/pci_x86.h>
>
> @@ -828,7 +828,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7910, rs690_fix_64bit_dma);
>
> #endif
>
> -#ifdef CONFIG_AMD_NB
> +#ifdef CONFIG_AMD_NODE
>
> #define AMD_15B8_RCC_DEV2_EPF0_STRAP2 0x10136008
> #define AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK 0x00000080L
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 06f7b43a6f78..cb97d7bdae31 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -78,6 +78,7 @@ config EDAC_GHES
> config EDAC_AMD64
> tristate "AMD64 (Opteron, Athlon64)"
> depends on AMD_NB && EDAC_DECODE_MCE
> + depends on AMD_NODE
> imply AMD_ATL
> help
> Support for error detection and correction of DRAM ECC errors on
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index ddfbdb66b794..29465088639c 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2,6 +2,7 @@
> #include <linux/ras.h>
> #include "amd64_edac.h"
> #include <asm/amd_nb.h>
> +#include <asm/amd_node.h>
>
> static struct edac_pci_ctl_info *pci_ctl;
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index dd376602f3f1..ea13ea482a63 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -324,7 +324,7 @@ config SENSORS_K8TEMP
>
> config SENSORS_K10TEMP
> tristate "AMD Family 10h+ temperature sensor"
> - depends on X86 && PCI && AMD_NB
> + depends on X86 && PCI && AMD_NODE
> help
> If you say yes here you get support for the temperature
> sensor(s) inside your CPU. Supported are later revisions of
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index cefa8cd184c8..d0b4cc9a5011 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -20,7 +20,7 @@
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/pci_ids.h>
> -#include <asm/amd_nb.h>
> +#include <asm/amd_node.h>
> #include <asm/processor.h>
>
> MODULE_DESCRIPTION("AMD Family 10h+ CPU core temperature monitor");
> diff --git a/drivers/platform/x86/amd/pmc/Kconfig b/drivers/platform/x86/amd/pmc/Kconfig
> index 94f9563d8be7..eeffdafd686e 100644
> --- a/drivers/platform/x86/amd/pmc/Kconfig
> +++ b/drivers/platform/x86/amd/pmc/Kconfig
> @@ -5,7 +5,7 @@
>
> config AMD_PMC
> tristate "AMD SoC PMC driver"
> - depends on ACPI && PCI && RTC_CLASS && AMD_NB
> + depends on ACPI && PCI && RTC_CLASS && AMD_NODE
> depends on SUSPEND
> select SERIO
> help
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 26b878ee5191..941b7753dd78 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -10,7 +10,6 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#include <asm/amd_nb.h>
> #include <linux/acpi.h>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> @@ -28,6 +27,8 @@
> #include <linux/seq_file.h>
> #include <linux/uaccess.h>
>
> +#include <asm/amd_node.h>
> +
> #include "pmc.h"
>
> /* SMU communication registers */
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 99d67cdbd91e..25b8f7ae3abd 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -7,7 +7,7 @@ config AMD_PMF
> tristate "AMD Platform Management Framework"
> depends on ACPI && PCI
> depends on POWER_SUPPLY
> - depends on AMD_NB
> + depends on AMD_NODE
> select ACPI_PLATFORM_PROFILE
> depends on TEE && AMDTEE
> depends on AMD_SFH_HID
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 06a97c533cb8..7f88f3121cf5 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -8,13 +8,13 @@
> * Author: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> */
>
> -#include <asm/amd_nb.h>
> #include <linux/debugfs.h>
> #include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> +#include <asm/amd_node.h>
> #include "pmf.h"
>
> /* PMF-SMU communication registers */
> diff --git a/drivers/ras/amd/atl/Kconfig b/drivers/ras/amd/atl/Kconfig
> index 551680073e43..6e03942cd7da 100644
> --- a/drivers/ras/amd/atl/Kconfig
> +++ b/drivers/ras/amd/atl/Kconfig
> @@ -10,6 +10,7 @@
> config AMD_ATL
> tristate "AMD Address Translation Library"
> depends on AMD_NB && X86_64 && RAS
> + depends on AMD_NODE
> depends on MEMORY_FAILURE
> default N
> help
> diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
> index 143d04c779a8..f9be26d25348 100644
> --- a/drivers/ras/amd/atl/internal.h
> +++ b/drivers/ras/amd/atl/internal.h
> @@ -18,6 +18,7 @@
> #include <linux/ras.h>
>
> #include <asm/amd_nb.h>
> +#include <asm/amd_node.h>
>
> #include "reg_fields.h"
>
>
Powered by blists - more mailing lists