RE: [XEN RFC PATCH 03/40] xen/x86: Initialize memnodemapsize while faking NUMA node

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2021年8月12日 23:33
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [XEN RFC PATCH 03/40] xen/x86: Initialize memnodemapsize
> while faking NUMA node
> 
> On 11.08.2021 12:23, Wei Chen wrote:
> > When system turns NUMA off or system lacks of NUMA support,
> > Xen will fake a NUMA node to make system works as a single
> > node NUMA system.
> >
> > In this case the memory node map doesn't need to be allocated
> > from boot pages. But we should set the memnodemapsize to the
> > array size of _memnodemap. Xen hadn't done it, and Xen should
> > assert in phys_to_nid. But because x86 was using an empty
> > macro "VIRTUAL_BUG_ON" to replace ASSERT, this bug will not
> > be triggered.
> 
> How about we promote VIRTUAL_BUG_ON() to expand to at least ASSERT()?
> 

That would be good. Frankly, we discovered this because we used ASSERT
in Arm and then noticed that x86 was using VIRTUAL_BUG_ON.

> > --- a/xen/arch/x86/numa.c
> > +++ b/xen/arch/x86/numa.c
> > @@ -270,6 +270,8 @@ void __init numa_initmem_init(unsigned long
> start_pfn, unsigned long end_pfn)
> >      /* setup dummy node covering all memory */
> >      memnode_shift = BITS_PER_LONG - 1;
> >      memnodemap = _memnodemap;
> > +    memnodemapsize = ARRAY_SIZE(_memnodemap);
> 
> But this doesn't reflect reality then, does it? We'd rather want to
> set the size to 1, I would think.
> 

Yes, you're right. Actually, we just only used 1 slot. But furthermore,
memnodemap[0] may be set in acpi_scan_nodes, but acpi_scan_nodes doesn't
reset memnodemap when it failed. I think maybe we can add:
    memnodemap[0] = 0;
    memnodemapsize = 1;
How do you think about it?

> Jan