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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Feb 1, 2024 at 3:19 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 29.01.2024 18:18, Carlo Nonato wrote:
> > @@ -950,6 +951,11 @@ void __init create_domUs(void)
> >  #endif
> >          }
> >
> > +        dt_property_read_string(node, "llc-colors", &llc_colors_str);
> > +        if ( !llc_coloring_enabled && llc_colors_str)
> > +            printk(XENLOG_WARNING
> > +                   "'llc-colors' found, but LLC coloring is disabled\n");
>
> Why's this just a warning, when ...

This informs the user that this configuration will be ignored, but the DomU can
be constructed anyway...

> > @@ -960,6 +966,11 @@ void __init create_domUs(void)
> >              panic("Error creating domain %s (rc = %ld)\n",
> >                    dt_node_name(node), PTR_ERR(d));
> >
> > +        if ( llc_coloring_enabled &&
> > +             (rc = domain_set_llc_colors_from_str(d, llc_colors_str)) )
> > +            panic("Error initializing LLC coloring for domain %s (rc = 
> > %d)\n",
> > +                  dt_node_name(node), rc);
>
> ... this results in panic()?

... while here we can't continue because there's some error in the
configuration and the DomU can't be constructed. Domains must have a valid
coloring configuration.

> > --- a/xen/common/llc-coloring.c
> > +++ b/xen/common/llc-coloring.c
> > @@ -254,6 +254,29 @@ int domain_set_llc_colors_domctl(struct domain *d,
> >      return domain_check_colors(d);
> >  }
> >
> > +int domain_set_llc_colors_from_str(struct domain *d, const char *str)
>
> __init ?

Yes.

Thanks.

> > +{
> > +    int err;
> > +    unsigned int *colors;
> > +
> > +    if ( !str )
> > +        return domain_set_default_colors(d);
> > +
> > +    colors = alloc_colors(max_nr_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    err = parse_color_config(str, colors, max_nr_colors, 
> > &d->num_llc_colors);
> > +    if ( err )
> > +    {
> > +        printk(XENLOG_ERR "Error parsing LLC color configuration.");
>
> Nit: No full stop at the end of log messages please.
>
> > +        return err;
> > +    }
> > +    d->llc_colors = colors;
> > +
> > +    return domain_check_colors(d);
>
> Same ordering issue as in the earlier patch, I think.
>
> Jan