Skip to content

Commit 0a4b61d

Browse files
yghannambp3tk0v
authored andcommitted
x86/amd_node: Fix AMD root device caching
Recent AMD node rework removed the "search and count" method of caching AMD root devices. This depended on the value from a Data Fabric register that was expected to hold the PCI bus of one of the root devices attached to that fabric. However, this expectation is incorrect. The register, when read from PCI config space, returns the bitwise-OR of the buses of all attached root devices. This behavior is benign on AMD reference design boards, since the bus numbers are aligned. This results in a bitwise-OR value matching one of the buses. For example, 0x00 | 0x40 | 0xA0 | 0xE0 = 0xE0. This behavior breaks on boards where the bus numbers are not exactly aligned. For example, 0x00 | 0x07 | 0xE0 | 0x15 = 0x1F. The examples above are for AMD node 0. The first root device on other nodes will not be 0x00. The first root device for other nodes will depend on the total number of root devices, the system topology, and the specific PCI bus number assignment. For example, a system with 2 AMD nodes could have this: Node 0 : 0x00 0x07 0x0e 0x15 Node 1 : 0x1c 0x23 0x2a 0x31 The bus numbering style in the reference boards is not a requirement. The numbering found in other boards is not incorrect. Therefore, the root device caching method needs to be adjusted. Go back to the "search and count" method used before the recent rework. Search for root devices using PCI class code rather than fixed PCI IDs. This keeps the goal of the rework (remove dependency on PCI IDs) while being able to support various board designs. Merge helper functions to reduce code duplication. [ bp: Reflow comment. ] Fixes: 40a5f6f ("x86/amd_nb: Simplify root device search") Signed-off-by: Yazen Ghannam <[email protected]> Signed-off-by: Borislav Petkov (AMD) <[email protected]> Cc: [email protected] Link: https://patch.msgid.link/all/[email protected]
1 parent 6146a0f commit 0a4b61d

File tree

2 files changed

+51
-100
lines changed

2 files changed

+51
-100
lines changed

arch/x86/include/asm/amd/node.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
#define AMD_NODE0_PCI_SLOT 0x18
2424

2525
struct pci_dev *amd_node_get_func(u16 node, u8 func);
26-
struct pci_dev *amd_node_get_root(u16 node);
2726

2827
static inline u16 amd_num_nodes(void)
2928
{

arch/x86/kernel/amd_node.c

Lines changed: 51 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -34,62 +34,6 @@ struct pci_dev *amd_node_get_func(u16 node, u8 func)
3434
return pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(AMD_NODE0_PCI_SLOT + node, func));
3535
}
3636

37-
#define DF_BLK_INST_CNT 0x040
38-
#define DF_CFG_ADDR_CNTL_LEGACY 0x084
39-
#define DF_CFG_ADDR_CNTL_DF4 0xC04
40-
41-
#define DF_MAJOR_REVISION GENMASK(27, 24)
42-
43-
static u16 get_cfg_addr_cntl_offset(struct pci_dev *df_f0)
44-
{
45-
u32 reg;
46-
47-
/*
48-
* Revision fields added for DF4 and later.
49-
*
50-
* Major revision of '0' is found pre-DF4. Field is Read-as-Zero.
51-
*/
52-
if (pci_read_config_dword(df_f0, DF_BLK_INST_CNT, &reg))
53-
return 0;
54-
55-
if (reg & DF_MAJOR_REVISION)
56-
return DF_CFG_ADDR_CNTL_DF4;
57-
58-
return DF_CFG_ADDR_CNTL_LEGACY;
59-
}
60-
61-
struct pci_dev *amd_node_get_root(u16 node)
62-
{
63-
struct pci_dev *root;
64-
u16 cntl_off;
65-
u8 bus;
66-
67-
if (!cpu_feature_enabled(X86_FEATURE_ZEN))
68-
return NULL;
69-
70-
/*
71-
* D18F0xXXX [Config Address Control] (DF::CfgAddressCntl)
72-
* Bits [7:0] (SecBusNum) holds the bus number of the root device for
73-
* this Data Fabric instance. The segment, device, and function will be 0.
74-
*/
75-
struct pci_dev *df_f0 __free(pci_dev_put) = amd_node_get_func(node, 0);
76-
if (!df_f0)
77-
return NULL;
78-
79-
cntl_off = get_cfg_addr_cntl_offset(df_f0);
80-
if (!cntl_off)
81-
return NULL;
82-
83-
if (pci_read_config_byte(df_f0, cntl_off, &bus))
84-
return NULL;
85-
86-
/* Grab the pointer for the actual root device instance. */
87-
root = pci_get_domain_bus_and_slot(0, bus, 0);
88-
89-
pci_dbg(root, "is root for AMD node %u\n", node);
90-
return root;
91-
}
92-
9337
static struct pci_dev **amd_roots;
9438

9539
/* Protect the PCI config register pairs used for SMN. */
@@ -274,51 +218,21 @@ DEFINE_SHOW_STORE_ATTRIBUTE(smn_node);
274218
DEFINE_SHOW_STORE_ATTRIBUTE(smn_address);
275219
DEFINE_SHOW_STORE_ATTRIBUTE(smn_value);
276220

277-
static int amd_cache_roots(void)
278-
{
279-
u16 node, num_nodes = amd_num_nodes();
280-
281-
amd_roots = kcalloc(num_nodes, sizeof(*amd_roots), GFP_KERNEL);
282-
if (!amd_roots)
283-
return -ENOMEM;
284-
285-
for (node = 0; node < num_nodes; node++)
286-
amd_roots[node] = amd_node_get_root(node);
287-
288-
return 0;
289-
}
290-
291-
static int reserve_root_config_spaces(void)
221+
static struct pci_dev *get_next_root(struct pci_dev *root)
292222
{
293-
struct pci_dev *root = NULL;
294-
struct pci_bus *bus = NULL;
295-
296-
while ((bus = pci_find_next_bus(bus))) {
297-
/* Root device is Device 0 Function 0 on each Primary Bus. */
298-
root = pci_get_slot(bus, 0);
299-
if (!root)
223+
while ((root = pci_get_class(PCI_CLASS_BRIDGE_HOST << 8, root))) {
224+
/* Root device is Device 0 Function 0. */
225+
if (root->devfn)
300226
continue;
301227

302228
if (root->vendor != PCI_VENDOR_ID_AMD &&
303229
root->vendor != PCI_VENDOR_ID_HYGON)
304230
continue;
305231

306-
pci_dbg(root, "Reserving PCI config space\n");
307-
308-
/*
309-
* There are a few SMN index/data pairs and other registers
310-
* that shouldn't be accessed by user space.
311-
* So reserve the entire PCI config space for simplicity rather
312-
* than covering specific registers piecemeal.
313-
*/
314-
if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
315-
pci_err(root, "Failed to reserve config space\n");
316-
return -EEXIST;
317-
}
232+
break;
318233
}
319234

320-
smn_exclusive = true;
321-
return 0;
235+
return root;
322236
}
323237

324238
static bool enable_dfs;
@@ -332,7 +246,8 @@ __setup("amd_smn_debugfs_enable", amd_smn_enable_dfs);
332246

333247
static int __init amd_smn_init(void)
334248
{
335-
int err;
249+
u16 count, num_roots, roots_per_node, node, num_nodes;
250+
struct pci_dev *root;
336251

337252
if (!cpu_feature_enabled(X86_FEATURE_ZEN))
338253
return 0;
@@ -342,13 +257,48 @@ static int __init amd_smn_init(void)
342257
if (amd_roots)
343258
return 0;
344259

345-
err = amd_cache_roots();
346-
if (err)
347-
return err;
260+
num_roots = 0;
261+
root = NULL;
262+
while ((root = get_next_root(root))) {
263+
pci_dbg(root, "Reserving PCI config space\n");
348264

349-
err = reserve_root_config_spaces();
350-
if (err)
351-
return err;
265+
/*
266+
* There are a few SMN index/data pairs and other registers
267+
* that shouldn't be accessed by user space. So reserve the
268+
* entire PCI config space for simplicity rather than covering
269+
* specific registers piecemeal.
270+
*/
271+
if (!pci_request_config_region_exclusive(root, 0, PCI_CFG_SPACE_SIZE, NULL)) {
272+
pci_err(root, "Failed to reserve config space\n");
273+
return -EEXIST;
274+
}
275+
276+
num_roots++;
277+
}
278+
279+
pr_debug("Found %d AMD root devices\n", num_roots);
280+
281+
if (!num_roots)
282+
return -ENODEV;
283+
284+
num_nodes = amd_num_nodes();
285+
amd_roots = kcalloc(num_nodes, sizeof(*amd_roots), GFP_KERNEL);
286+
if (!amd_roots)
287+
return -ENOMEM;
288+
289+
roots_per_node = num_roots / num_nodes;
290+
291+
count = 0;
292+
node = 0;
293+
root = NULL;
294+
while (node < num_nodes && (root = get_next_root(root))) {
295+
/* Use one root for each node and skip the rest. */
296+
if (count++ % roots_per_node)
297+
continue;
298+
299+
pci_dbg(root, "is root for AMD node %u\n", node);
300+
amd_roots[node++] = root;
301+
}
352302

353303
if (enable_dfs) {
354304
debugfs_dir = debugfs_create_dir("amd_smn", arch_debugfs_dir);
@@ -358,6 +308,8 @@ static int __init amd_smn_init(void)
358308
debugfs_create_file("value", 0600, debugfs_dir, NULL, &smn_value_fops);
359309
}
360310

311+
smn_exclusive = true;
312+
361313
return 0;
362314
}
363315

0 commit comments

Comments
 (0)