RE: [PATCH v2 1/9] xen/x86: introduce a helper to update memory hotplug end

[Wei Chen <Wei.Chen@arm.com>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 2022年7月12日 19:54
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau
> Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 1/9] xen/x86: introduce a helper to update memory
> hotplug end
> 
> On 08.07.2022 16:54, Wei Chen wrote:
> > x86 provides a mem_hotplug variable to maintain the memory hotplug
> > end address. We want to move some codes from x86 to common, so that
> > it can be reused by other architectures. But not all architectures
> > have supported memory hotplug. So in this patch, we introduce this
> > helper to replace mem_hotplug direct access in these codes. This
> > will give the ability of stubbing this helper to those architectures
> > without memory hotplug support.
> 
> What remains unclear to me is why Arm can't simply gain the necessary
> variable. Sooner or later you'll want to support hotplug there anyway,

I am not sure my limited memory hotplug knowledge can answer your question
or not. As memory hotplug depends on hardware and firmware support. Now
for Arm, we only have ACPI firmware that can notify OS through GED event
(not very sure). But ACPI and device tree couldn't be enabled at the same
time from OS booting. In device tree based NUMA, we will enable device
tree, ACPI service will not be initialized, that means we can not get
notification of memory hotplug events from ACPI firmware. 

Of course, adding GED device nodes to the device tree, and getting these
events through GED interrupts should not be a big technical problem. But
there may be other reasons, and no one is planning to add memory hotplug
support to the device tree (perhaps need an ACPI-like specification or a
firmware abstraction interface). So if we want to implement Xen Arm memory
hotplug, it should also start from ACPI. So currently even if we gain the
variable in Arm, it will not be used. Device tree does not have property
to indicate a memory block can be hotplug or not.

> I suppose. Mechanically the change is fine. Albeit maybe "top" instead
> of "boundary", and maybe also pass "node" even if x86 doesn't need it?
> 

Sorry, I am not very clear about these comments:
It makes sense to use mem_hotplug_update_top instead
of mem_hotplug_update_boundary. But how can I understand pass "node"?
did you mean mem_hotplug_update_top(node, end)? But mem_hotplug is
a global top for memory hotplug ranges. I don't think pass "node"
to this function can be useful.

Cheers,
Wei Chen

> Jan
> 
> > Signed-off-by: Wei Chen <wei.chen@arm.com>
> > ---
> > v1 -> v2:
> > 1. Refine the commit message.
> > 2. Merge v1 patch#9,10 into one patch. Introduce the new functions
> >    in the same patch that this patch will be used first time.
> > 3. Fold if ( end > mem_hotplug ) to mem_hotplug_update_boundary,
> >    in this case, we can drop mem_hotplug_boundary.
> > ---
> >  xen/arch/x86/include/asm/mm.h | 6 ++++++
> >  xen/arch/x86/srat.c           | 3 +--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/mm.h
> b/xen/arch/x86/include/asm/mm.h
> > index 07b59c982b..b3dfbdb52b 100644
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -476,6 +476,12 @@ static inline int get_page_and_type(struct
> page_info *page,
> >
> >  extern paddr_t mem_hotplug;
> >
> > +static inline void mem_hotplug_update_boundary(paddr_t end)
> > +{
> > +    if ( end > mem_hotplug )
> > +        mem_hotplug = end;
> > +}
> > +
> >
> /*************************************************************************
> *****
> >   * With shadow pagetables, the different kinds of address start
> >   * to get get confusing.
> > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> > index b62a152911..f53431f5e8 100644
> > --- a/xen/arch/x86/srat.c
> > +++ b/xen/arch/x86/srat.c
> > @@ -418,8 +418,7 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> >  	memblk_nodeid[num_node_memblks] = node;
> >  	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> >  		__set_bit(num_node_memblks, memblk_hotplug);
> > -		if (end > mem_hotplug)
> > -			mem_hotplug = end;
> > +		mem_hotplug_update_boundary(end);
> >  	}
> >  	num_node_memblks++;
> >  }