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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2022 10:22, Wei Chen wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年7月12日 19:54
>>
>> 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.

But ACPI is possible to be enabled for Arm64, and hence hotplug could be
made work that way. Therefore I view the variable as potentially useful
on Arm as well, and hence introducing the variable might be less trouble
than introducing the per-arch helper.

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

Please separate "current implementation" from "abstract model". In the
latter it would seem quite reasonable to me to have per-node upper
bounds (or even per-node ranges). Therefore adding node (and, on top
of what I did suggest before, region start) to the parameters of the
new per-arch hook would seem a good move to me, even if at present
only / at most the "end" parameter would actually be used.

Jan