Re: [PATCH v10 07/12] xen/arm: add support for cache coloring configuration via device-tree

[Michal Orzel <michal.orzel@xxxxxxx>]

On 19/11/2024 15:13, Carlo Nonato wrote:
> 
> 
> Add the "llc-colors" Device Tree attribute to express DomUs and Dom0less
NIT: s/attribute/property/

> color configurations.
> 
> Based on original work from: Luca Miccio <lucmiccio@xxxxxxxxx>
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # non-Arm
> ---
> v10:
> - no changes
> v9:
> - use best-effort allocation in domain_set_llc_colors_from_str()
> v8:
> - fixed memory leak on error path of domain_set_llc_colors_from_str()
> - realloc colors array after parsing from string to reduce memory usage
> v7:
> - removed alloc_colors() helper usage from domain_set_llc_colors_from_str()
> v6:
> - rewrote domain_set_llc_colors_from_str() to be more explicit
> v5:
> - static-mem check has been moved in a previous patch
> - added domain_set_llc_colors_from_str() to set colors after domain creation
> ---
>  docs/misc/arm/device-tree/booting.txt |  4 +++
>  docs/misc/cache-coloring.rst          | 48 +++++++++++++++++++++++++++
>  xen/arch/arm/dom0less-build.c         | 10 ++++++
>  xen/common/llc-coloring.c             | 41 +++++++++++++++++++++++
>  xen/include/xen/llc-coloring.h        |  1 +
>  xen/include/xen/xmalloc.h             | 12 +++++++
>  6 files changed, 116 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 3a04f5c57f..9085645433 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -162,6 +162,10 @@ with the following properties:
> 
>      An integer specifying the number of vcpus to allocate to the guest.
> 
> +- llc-colors
> +    A string specifying the LLC color configuration for the guest.
> +    Refer to docs/misc/cache_coloring.rst for syntax.
Shouldn't we make it clear that this property is Arm64 only?

> +
>  - vpl011
> 
>      An empty property to enable/disable a virtual pl011 for the guest to
> diff --git a/docs/misc/cache-coloring.rst b/docs/misc/cache-coloring.rst
> index c5fb33996c..c14b6ee2f4 100644
> --- a/docs/misc/cache-coloring.rst
> +++ b/docs/misc/cache-coloring.rst
> @@ -12,6 +12,7 @@ If needed, change the maximum number of colors with
>  ``CONFIG_LLC_COLORS_ORDER=<n>``.
> 
>  Runtime configuration is done via `Command line parameters`_.
> +For DomUs follow `DomUs configuration`_.
> 
>  Background
>  **********
> @@ -147,6 +148,53 @@ LLC specs can be manually set via the above command line 
> parameters. This
>  bypasses any auto-probing and it's used to overcome failing situations, such 
> as
>  flawed probing logic, or for debugging/testing purposes.
> 
> +DomUs configuration
> +*******************
> +
> +DomUs colors can be set either in the ``xl`` configuration file 
> (documentation
> +at `docs/man/xl.cfg.pod.5.in`) or via Device Tree, also for Dom0less
> +configurations (documentation at `docs/misc/arm/device-tree/booting.txt`) 
> using
NIT: I think you can remove the part "also for Dom0less configurations" if you 
already
mentioned "or via Device Tree".

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal