Re: [PATCH v2] xen/dom0less: support for vcpu affinity

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 15/02/2025 01:17, Stefano Stabellini wrote:
> 
> 
> Add vcpu affinity to the dom0less bindings. Example:
> 
>                          dom1 {
>                                  ...
>                                  cpus = <4>;
>                                  vcpu0 {
>                                         compatible = "xen,vcpu-affinity";
>                                         id = <0>;
>                                         hard-affinity = "4-7";
>                                  };
>                                  vcpu1 {
>                                         compatible = "xen,vcpu-affinity";
>                                         id = <1>;
>                                         hard-affinity = "0-3";
>                                  };
>                                  vcpu2 {
>                                         compatible = "xen,vcpu-affinity";
>                                         id = <2>;
>                                         hard-affinity = "1,6";
>                                  };
>                                  ...
>                          }
What is this indentation for? It reads strangely.

> 
> Note that the property hard-affinity is optional. It is possible to add
If it's optional, then this node does not make sense anymore...

> other properties in the future not only to specify soft affinity, but
> also to provide more precise methods for configuring affinity. For
> instance, on ARM the MPIDR could be use to specify the pCPU. For now, it
> is left to the future.
> 
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
> Changes in v2:
> - code style
> - add binding description to docs/misc/arm/device-tree/booting.txt
> ---
> 
>  docs/misc/arm/device-tree/booting.txt | 21 +++++++++++
>  xen/arch/arm/dom0less-build.c         | 51 +++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 9c881baccc..6a2abbef4e 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -324,6 +324,27 @@ The ramdisk sub-node has the following properties:
>      property because it will be created by the UEFI stub on boot.
>      This option is needed only when UEFI boot is used.
> 
> +Under the "xen,domain" compatible node, it is possible optionally to add
> +one or more sub-nodes to configure vCPU affinity. The vCPU affinity
> +sub-node has the following properties:
> +
> +- compatible
> +
> +    "xen,vcpu-affinity"
> +
> +- id
> +
> +    A 32-bit integer that specifies the vCPU id. 0 is the first vCPU.
> +    The last vCPU is cpus-1, where "cpus" is the number of vCPUs
> +    specified with the "cpus" property under the "xen,domain" node.
> +
> +- hard-affinity
> +
> +    Optional. A string specifying the hard affinity configuration for the
> +    vCPU: a comma-separated list of pCPUs or ranges of pCPUs is used.
> +    Ranges are hyphen-separated intervals (such as `0-4`) and are inclusive
> +    on both sides.
I think users should know what this number refers to.

> +
> 
>  Example
>  =======
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 49d1f14d65..35d02689e7 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -818,6 +818,8 @@ void __init create_domUs(void)
>      const struct dt_device_node *cpupool_node,
>                                  *chosen = dt_find_node_by_path("/chosen");
>      const char *llc_colors_str = NULL;
> +    const char *hard_affinity_str = NULL;
> +    struct dt_device_node *np;
> 
>      BUG_ON(chosen == NULL);
>      dt_for_each_child_node(chosen, node)
> @@ -992,6 +994,55 @@ void __init create_domUs(void)
>          if ( rc )
>              panic("Could not set up domain %s (rc = %d)\n",
>                    dt_node_name(node), rc);
> +
> +        dt_for_each_child_node(node, np)
Can we please move this to a separate function? create_domUs starts to be
difficult to parse due to its length. It will also fix 80chars limit issues.

> +        {
> +            const char *s;
> +            struct vcpu *v;
> +            cpumask_t affinity;
> +
> +            if ( !dt_device_is_compatible(np, "xen,vcpu-affinity") )
> +                continue;
> +
> +            if ( !dt_property_read_u32(np, "id", &val) )
> +                continue;
empty line here please

> +            if ( val >= d->max_vcpus )
> +                panic("Invalid vcpu_id %u for domain %s\n", val, 
> dt_node_name(node));
> +
> +            v = d->vcpu[val];
> +            rc = dt_property_read_string(np, "hard-affinity", 
> &hard_affinity_str);
> +            if ( rc < 0 )
> +                continue;
> +
> +            s = hard_affinity_str;
> +            cpumask_clear(&affinity);
> +            while ( *s != '\0' )
> +            {
> +                unsigned int start, end;
> +
> +                start = simple_strtoul(s, &s, 0);
> +
> +                if ( *s == '-' )    /* Range */
> +                {
> +                    s++;
> +                    end = simple_strtoul(s, &s, 0);
> +                }
> +                else                /* Single value */
> +                    end = start;
> +
> +                for ( ; start <= end; start++ )
> +                    cpumask_set_cpu(start, &affinity);
What if the pCPU number is invalid? Then we rely on debug ASSERT. I think we
should panic here on invalid number.

> +
> +                if ( *s == ',' )
> +                    s++;
> +                else if ( *s != '\0' )
> +                    break;
> +            }
> +
> +            rc = vcpu_set_hard_affinity(v, &affinity);
> +            if ( rc )
> +                panic("vcpu%d: failed to set hard affinity\n", v->vcpu_id);
> +        }
>      }
>  }
> 
> --
> 2.25.1
> 

~Michal