Re: [PATCH 1/2] x86/amd_node: Fix AMD root device caching

Borislav Petkov
2025-10-22T11:14:00Z

On Tue, Sep 30, 2025 at 04:45:45PM +0000, Yazen Ghannam wrote:
> 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.

<---

Please add here something along the lines of:

"And even if one could say, they both have bus 0x0 containing the root
devices, this is not true on the other AMD nodes besides 0."

> static int amd_cache_roots(void)
> {
> - u16 node, num_nodes = amd_num_nodes();
> + u16 count = 0, num_roots = 0, roots_per_node, node = 0, num_nodes = amd_num_nodes();
> + struct pci_dev *root = NULL;
>
> amd_roots = kcalloc(num_nodes, sizeof(*amd_roots), GFP_KERNEL);
> if (!amd_roots)
> return -ENOMEM;
>
> - for (node = 0; node < num_nodes; node++)
> - amd_roots[node] = amd_node_get_root(node);
> + while ((root = get_next_root(root))) {
> + pci_dbg(root, "is an AMD root device\n");
> + num_roots++;
> + }
> +
> + pr_debug("Found %d AMD root devices\n", num_roots);
> +
> + roots_per_node = num_roots / num_nodes;

What happens if num_roots = 0? IOW, you need to handle that here.

> +
> + while ((root = get_next_root(root)) && node < num_nodes) {
> + /* Use one root for each node and skip the rest. */
> + if (count++ % roots_per_node)
> + continue;
> +
> + pci_dbg(root, "is root for AMD node %u\n", node);
> + amd_roots[node++] = root;
> + }

If I squint my eyes hard enough, I can see you getting rid of the *three*
while loops here. So please try again.

Thx.

--
Regards/Gruss,
Boris.