Re: [PATCH 12/36] xen/arm: initialize cache coloring data for Dom0/U

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> Initialize cache coloring configuration during domain creation. If no
> colors assignment is provided by the user, use the default one.
> The default configuration is the one assigned to Dom0. The latter is
> configured as a standard domain with default configuration.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/domain.c       | 53 +++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/domain_build.c |  5 +++-
>   2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 8110c1df86..33471b3c58 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -38,6 +38,7 @@
>   #include <asm/vfp.h>
>   #include <asm/vgic.h>
>   #include <asm/vtimer.h>
> +#include <asm/coloring.h>
>   
>   #include "vpci.h"
>   #include "vuart.h"
> @@ -782,6 +783,58 @@ int arch_domain_create(struct domain *d,
>       if ( (rc = domain_vpci_init(d)) != 0 )
>           goto fail;
>   
> +    d->max_colors = 0;
NIT: d is always zeroed when allocated. So it is not necessary to 
initialize the field again.
> +#ifdef CONFIG_COLORING
Please move this code in a separate helper. The new helper could be 
defined in coloring.c.
Furthermore, I would initialize the coloring information earlier in 
arch_domain_create(). This could be useful if we want to allocate 
internal structure from a color assigned to the domain.
> +    /* Setup domain colors */
> +    if ( !config->arch.colors.max_colors )
> +    {
> +        if ( !is_hardware_domain(d) )
> +            printk(XENLOG_INFO "Color configuration not found for dom%u, using 
> default\n",
This message and the other below wants to be ratelimited. I would use 
XENLOG_G_{INFO, ERROR}.
Please use %pd instead of dom%u. This remark is valid for all the other 
use below.
> +                   d->domain_id);
This would need to be changed to 'd'.

> +        d->colors = setup_default_colors(&d->max_colors);
Looking at setup_default_colors(), it using "dom0_col_num". This implies 
we are using the dom0 color. Shouldn't we return an error if d is not 
the hardware domain?
Also, AFAICT, you allocate the memory but never free it.

> +        if ( !d->colors )
> +        {
> +            rc = -ENOMEM;
> +            printk(XENLOG_ERR "Color array allocation failed for dom%u\n",
> +                   d->domain_id);
> +            goto fail;
> +        }
> +    }
> +    else
> +    {
> +        int i, k;
> +
> +        d->colors = xzalloc_array(uint32_t, config->arch.colors.max_colors);
Same here.

> +        if ( !d->colors )
> +        {
> +            rc = -ENOMEM;
> +            printk(XENLOG_ERR "Failed to alloc colors for dom%u\n",
> +                   d->domain_id);
> +            goto fail;
> +        }
> +
> +        d->max_colors = config->arch.colors.max_colors;
> +        for ( i = 0, k = 0;
> +              k < d->max_colors && i < sizeof(config->arch.colors.colors) * 8;
> +              i++ )
> +        {
> +            if ( config->arch.colors.colors[i / 32] & (1 << (i % 32)) )
> +                d->colors[k++] = i;
> +        }
> +    }
> +
> +    printk("Dom%u colors: [ ", d->domain_id);
> +    for ( int i = 0; i < d->max_colors; i++ ) > +        printk("%u ", 
> d->colors[i]);
> +    printk("]\n");
You will be able to get the same information using the debug-key. So I 
am not convinced this is warrant to print here. The more the 
configuration should always be the same as what the user requested.
> +
> +    if ( !check_domain_colors(d) )
> +    {
> +        rc = -EINVAL;
> +        printk(XENLOG_ERR "Failed to check colors for dom%u\n", d->domain_id);
> +        goto fail;
> +    }
> +#endif
>       return 0;
>   
>   fail:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de..9630d00066 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3344,7 +3344,10 @@ void __init create_dom0(void)
>           printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>       dom0_cfg.arch.tee_type = tee_get_type();
>       dom0_cfg.max_vcpus = dom0_max_vcpus();
> -
> +#ifdef CONFIG_COLORING
> +    /* Colors are set after domain_create */
Do you instead mean 'by'?

> +    dom0_cfg.arch.colors.max_colors = 0;
> +#endif
>       if ( iommu_enabled )
>           dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>   
Cheers,

--
Julien Grall