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

Yazen Ghannam
2025-10-22T13:19:12Z

On Wed, Oct 22, 2025 at 01:13:42PM +0200, Borislav Petkov wrote:
> 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."
>

Okay, will do.

> > 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.

Yes, will do.

>
> > +
> > + 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.
>

I don't follow. Do you mean to combine the other loops into this one? Or
that this loop should be expanded into three loops explicitly doing one
thing each?

Thanks,
Yazen