RE: [PATCH v7 5/6] xen/x86: move NUMA process nodes nodes code from x86 to common

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年11月9日 0:55
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 5/6] xen/x86: move NUMA process nodes nodes code
> from x86 to common
> 
> > mem_hotplug is accessed by common code if memory hotplug is
> > activated. Even if this is only supported by x86, export the
> > variable so that other architectures could support it in the future.
> >
> > As asm/acpi.h has been removed from common/numa.c, we have to
> > move NR_NODE_MEMBLKS from asm/acpi.h to xen/numa.h in this patch
> > as well.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> 
> There's just one remaining concern I have: I continue to consider ...
> 
> > @@ -341,159 +247,14 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> >             pxm &= 0xff;
> >     node = setup_node(pxm);
> >     if (node == NUMA_NO_NODE) {
> > -           bad_srat();
> > +           numa_fw_bad();
> >             return;
> > -                           }
> > -           } while (found && start < end);
> > -
> > -           if (start < end) {
> > -                   printk(KERN_ERR "NUMA: No NODE for RAM range: "
> > -                           "[%"PRIpaddr", %"PRIpaddr"]\n", start, end - 1);
> > -                   return 0;
> > -           }
> > -   }
> > -   return 1;
> > +   numa_fw_nid_name = "PXM";
> 
> ... this to be happening too late. Not because I can see a way for current
> code to use the variable earlier, but because of the risk of future code
> potentially doing so. Afaics srat_parse_regions() is called quite a bit
> earlier, so perhaps the field should (also?) be set there, presumably
> after acpi_table_parse() has succeeded. I've included "(also?)" because I
> think to be on the safe side the setting here may want keeping, albeit
> perhaps moving up in the function.
> 

When I was composing this patch, I also thought current place to call this
"PXM" setting would be a little late. But since there is only one function
that uses this prefix right now, I thought it was acceptable at the time.
But obviously your concerns make sense, I will move this call to
srat_parse_regions after acpi_table_parse has been done successfully.

Cheers,
Wei Chen

> Jan