Re: [PATCH 08/11] xen/memory: Enable parsing NUMA node argument in XENMEM_claim_pages

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 14, 2025 at 05:24:59PM +0000, Alejandro Vallejo wrote:
> Extract a NUMA node from mem_flags. This _must_ be an exact node, and
> has the semantics of making a claim on a specific node.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/memory.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 1ab0bac4e7da..28f9db89ba62 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1659,6 +1659,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>  
>      case XENMEM_claim_pages:
> +    {
> +        nodeid_t node;
> +
>          if ( unlikely(start_extent) )
>              return -EINVAL;
>  
> @@ -1671,9 +1674,23 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( reservation.extent_order != 0 )
>              return -EINVAL;
>  
> -        if ( reservation.mem_flags != 0 )
> +        /* Only allow NUMA-related memory flags to claim on exact nodes */
> +        if ( (reservation.mem_flags &
> +              !XENMEMF_exact_node(XENMEMF_node_mask - 1)) != 0 )

Are you sure the above is correct?

!XENMEMF_exact_node(XENMEMF_node_mask - 1) resolves unconditionally to
false, and hence you are doing:

(reservation.mem_flags & 0) != 0

Which can never be true?

I think you want ~ instead of !.

>              return -EINVAL;
>  
> +        node = XENMEMF_get_node(reservation.mem_flags);
> +        if ( node != NUMA_NO_NODE )
> +        {
> +            /* Disallow advisory nodes. If present, must be exact */
> +            if ( !(reservation.mem_flags & XENMEMF_exact_node_request) )
> +                return -EINVAL;
> +
> +            /* Disallow nodes that would overflow the in-hypervisor arrays */
> +            if ( node >= MAX_NUMNODES )
> +                return -EINVAL;

You can likely join all checks in a single condition.

Thanks, Roger.