Re: [PATCH v4 05/10] xen/domain: Add DOMCTL handler for claiming memory with NUMA awareness

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2026 17:17, Bernhard Kaindl wrote:
> Jan Beulich wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
> [...]
>>> +int claim_memory(struct domain *d, [...]
>>
>> static in domctl.c? Otherwise with Penny's work to make domctl optional this
>> would be unreachable code.
> 
> Thanks, done: Moved it to domctl.c to be not compiled without MGMT_HYPERCALLS 
> in v5/v6.
> 
>>> +    if ( uinfo->pad || uinfo->nr_claims != 1 || d->is_dying )
>>> +        return -EINVAL;
>>
>> As already alluded to in reply to patch 03, I can't help the impression that
>> usage of this sub-op with multiple entries would we quite different (i.e. it
>> would be not only the implementation in Xen that changes). I'm therefore
>> pretty uncertain whether taking it with this restriction is going to make
>> much sense.
> 
> I submitted this sub-op to support multiple entries with v5/v6 now.
> 
> In v5/v6 these checks are updated to support multiple claims in the claim set.
> For clarity, I renamed the .node of the individual claim entries to .target:
> 
> The target of a claim entry can also be a selector for a global claim
> or a legacy claim and the field have many bits for future use.
> 
> This wasn't needed but I think it's clearer that the claim entry specifies a
> target which is where the claim entry is aimed at, it's not just only a node.
> 
>> +    if ( claim.node == XEN_DOMCTL_CLAIM_MEMORY_NO_NODE )
>>> +        claim.node = NUMA_NO_NODE;
>>
>> What about the incoming claim.node being NUMA_NO_NODE? Imo the range checking
>> the previous patch adds to domain_set_outstanding_pages() wants to move here,
>> at which point the function's new parameter could be properly nodeid_t.
> 
> nodeid_t and NUMA_NO_NODE have (judging by the existing implementation) are 
> not
> exposed in the public API to the control domain.
> 
> This separation is probably a good thing because it allows to change Xen 
> internals
> like nodeit_t and NUMA_NO_NODE if so desired without changing the public API.
> 
> NUMA_NO_NODE is defined as 0xFF and nodeid_t is u8. But that is just an
> implementation detail of the Hypervisor itself. If needed, we could change
> the implementation like this series could do, if wanted.

You spell it all out here, but then you don't draw the conclusion that I was 
aiming
at: If someone passes in 0xff, that _should not_ be mistaken for NUMA_NO_NODE. 
Hence
for the time being you simply need to reject 0xff if you don't want to expose 
"no
specific node" exactly that way in the ABI. And indeed ...

> The public struct xen_sysctl_numainfo and xen_sysctl_physinfo define 
> num_nodes,
> nr_nodes and max_node_id as uint32_t, for example. For type consistency, I 
> opted
> to define this public API as uint32_t as well and not expose internal 
> types/values.

... the proper representation there would then likely be 0xffffffff.

Jan