Re: [PATCH v3 5/6] arm/dom0less: assign dom0less guests to cpupools

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 18 Mar 2022, Luca Fancellu wrote:
> Introduce domain-cpupool property of a xen,domain device tree node,
> that specifies the cpupool device tree handle of a xen,cpupool node
> that identifies a cpupool created at boot time where the guest will
> be assigned on creation.
> 
> Add member to the xen_domctl_createdomain public interface so the
> XEN_DOMCTL_INTERFACE_VERSION version is bumped.
> 
> Add public function to retrieve a pool id from the device tree
> cpupool node.
> 
> Update documentation about the property.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> Changes in v3:
> - Use explicitely sized integer for struct xen_domctl_createdomain
>   cpupool_id member. (Stefano)
> - Changed code due to previous commit code changes
> Changes in v2:
> - Moved cpupool_id from arch specific to common part (Juergen)
> - Implemented functions to retrieve the cpupool id from the
>   cpupool dtb node.
> ---
>  docs/misc/arm/device-tree/booting.txt |  5 +++++
>  xen/arch/arm/domain_build.c           | 14 +++++++++++++-
>  xen/common/boot_cpupools.c            | 24 ++++++++++++++++++++++++
>  xen/common/domain.c                   |  2 +-
>  xen/include/public/domctl.h           |  4 +++-
>  xen/include/xen/sched.h               |  9 +++++++++
>  6 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index a94125394e35..7b4a29a2c293 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -188,6 +188,11 @@ with the following properties:
>      An empty property to request the memory of the domain to be
>      direct-map (guest physical address == physical address).
>  
> +- domain-cpupool
> +
> +    Optional. Handle to a xen,cpupool device tree node that identifies the
> +    cpupool where the guest will be started at boot.
> +
>  Under the "xen,domain" compatible node, one or more sub-nodes are present
>  for the DomU kernel and ramdisk.
>  
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de05..9c67a483d4a4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3172,7 +3172,8 @@ static int __init construct_domU(struct domain *d,
>  void __init create_domUs(void)
>  {
>      struct dt_device_node *node;
> -    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
> +    const struct dt_device_node *cpupool_node,
> +                                *chosen = dt_find_node_by_path("/chosen");
>  
>      BUG_ON(chosen == NULL);
>      dt_for_each_child_node(chosen, node)
> @@ -3241,6 +3242,17 @@ void __init create_domUs(void)
>                                           vpl011_virq - 32 + 1);
>          }
>  
> +        /* Get the optional property domain-cpupool */
> +        cpupool_node = dt_parse_phandle(node, "domain-cpupool", 0);
> +        if ( cpupool_node )
> +        {
> +            int pool_id = btcpupools_get_domain_pool_id(cpupool_node);
> +            if ( pool_id < 0 )
> +                panic("Error getting cpupool id from domain-cpupool (%d)\n",
> +                      pool_id);
> +            d_cfg.cpupool_id = pool_id;
> +        }
> +
>          /*
>           * The variable max_init_domid is initialized with zero, so here it's
>           * very important to use the pre-increment operator to call
> diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
> index f6f2fa8f2701..feba93a243fc 100644
> --- a/xen/common/boot_cpupools.c
> +++ b/xen/common/boot_cpupools.c
> @@ -23,6 +23,8 @@ static unsigned int __initdata next_pool_id;
>  
>  #define BTCPUPOOLS_DT_NODE_NO_REG     (-1)
>  #define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
> +#define BTCPUPOOLS_DT_WRONG_NODE      (-3)
> +#define BTCPUPOOLS_DT_CORRUPTED_NODE  (-4)
>  
>  static int __init get_logical_cpu_from_hw_id(unsigned int hwid)
>  {
> @@ -55,6 +57,28 @@ get_logical_cpu_from_cpu_node(const struct dt_device_node 
> *cpu_node)
>      return cpu_num;
>  }
>  
> +int __init btcpupools_get_domain_pool_id(const struct dt_device_node *node)
> +{
> +    const struct dt_device_node *phandle_node;
> +    int cpu_num;
> +
> +    if ( !dt_device_is_compatible(node, "xen,cpupool") )
> +        return BTCPUPOOLS_DT_WRONG_NODE;
> +    /*
> +     * Get first cpu listed in the cpupool, from its reg it's possible to
> +     * retrieve the cpupool id.
> +     */
> +    phandle_node = dt_parse_phandle(node, "cpupool-cpus", 0);
> +    if ( !phandle_node )
> +        return BTCPUPOOLS_DT_CORRUPTED_NODE;
> +
> +    cpu_num = get_logical_cpu_from_cpu_node(phandle_node);
> +    if ( cpu_num < 0 )
> +        return cpu_num;
> +
> +    return pool_cpu_map[cpu_num];
> +}
> +
>  static int __init check_and_get_sched_id(const char* scheduler_name)
>  {
>      int sched_id = sched_get_id_by_name(scheduler_name);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 351029f8b239..0827400f4f49 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -698,7 +698,7 @@ struct domain *domain_create(domid_t domid,
>          if ( !d->pbuf )
>              goto fail;
>  
> -        if ( (err = sched_init_domain(d, 0)) != 0 )
> +        if ( (err = sched_init_domain(d, config->cpupool_id)) != 0 )
>              goto fail;
>  
>          if ( (err = late_hwdom_init(d)) != 0 )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index b85e6170b0aa..2f4cf56f438d 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -106,6 +106,8 @@ struct xen_domctl_createdomain {
>      /* Per-vCPU buffer size in bytes.  0 to disable. */
>      uint32_t vmtrace_size;
>  
> +    uint32_t cpupool_id;
> +
>      struct xen_arch_domainconfig arch;
>  };
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 5d83465d3915..4e749a604f25 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1182,6 +1182,7 @@ unsigned int btcpupools_get_cpupool_id(unsigned int 
> cpu);
>  
>  #ifdef CONFIG_HAS_DEVICE_TREE
>  void btcpupools_dtb_parse(void);
> +int btcpupools_get_domain_pool_id(const struct dt_device_node *node);
>  #else
>  static inline void btcpupools_dtb_parse(void) {}
>  #endif
> @@ -1193,6 +1194,14 @@ static inline unsigned int 
> btcpupools_get_cpupool_id(unsigned int cpu)
>  {
>      return 0;
>  }
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +static inline int
> +btcpupools_get_domain_pool_id(const struct dt_device_node *node)
> +{
> +    return 0;
> +}
> +#endif

This is OK because in case !CONFIG_BOOT_TIME_CPUPOOLS, we have to handle
both the CONFIG_HAS_DEVICE_TREE and the !CONFIG_HAS_DEVICE_TREE.

It is the other case (CONFIG_BOOT_TIME_CPUPOOLS &&
!CONFIG_HAS_DEVICE_TREE) that is not possible.

This patch looks good to me.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>