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日 17:30
> 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
> 
> On 09.11.2022 09:51, Wei Chen wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 2022年11月9日 0:55
> >>
> >>> @@ -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.
> 
> As said - perhaps not move, but copy. There is an (extremely unlikely)
> case
> where srat_parse_regions() would not be called at all.
> 

Got it, understand now.

Cheers,
Wei Chen

> Jan