Re: [PATCH v5 02/13] xen/arm: add cache coloring initialization

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Thu, Jan 11, 2024 at 11:44 AM Julien Grall <julien@xxxxxxx> wrote:
> On 11/01/2024 10:17, Carlo Nonato wrote:
> > Hi Julien,
>
> Hi Carlo,
>
> >>>> +bool __init llc_coloring_init(void)
> >>>> +{
> >>>> +    if ( !llc_way_size && !(llc_way_size = get_llc_way_size()) )
> >>>> +    {
> >>>> +        printk(XENLOG_ERR
> >>>> +               "Probed LLC way size is 0 and no custom value 
> >>>> provided\n");
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * The maximum number of colors must be a power of 2 in order to 
> >>>> correctly
> >>>> +     * map them to bits of an address, so also the LLC way size must be 
> >>>> so.
> >>>> +     */
> >>>> +    if ( llc_way_size & (llc_way_size - 1) )
> >>>> +    {
> >>>> +        printk(XENLOG_WARNING "LLC way size (%u) isn't a power of 2.\n",
> >>>> +               llc_way_size);
> >>>> +        llc_way_size = 1U << flsl(llc_way_size);
> >>>> +        printk(XENLOG_WARNING
> >>>> +               "Using %u instead. Performances will be suboptimal\n",
> >>>> +               llc_way_size);
> >>>> +    }
> >>>> +
> >>>> +    nr_colors = llc_way_size >> PAGE_SHIFT;
> >>>> +
> >>>> +    if ( nr_colors < 2 || nr_colors > CONFIG_NR_LLC_COLORS )
> >>>
> >>> I didn't find any documentation explaining why we need at least two
> >>> colors. I guess you want to make sure that there is a color for Xen and
> >>> domain. But I wonder what could wrong with just one color (other than
> >>> been pointless)?
> >>
> >> Yes, it would just be pointless. I'll change it to 1.
> >
> > Just wanted to correct myself here. Having just a single color introduces a
> > clear sharing of the cache between Xen and domains. So it's not just
> > pointless, but also inefficient. I would discourage such a configuration, 
> > so I
> > plan to better describe this with a range in the Kconfig option (see
> > discussion in #1).
>
> I understand this could be inneficient. But you are also allowing the
> user to not specify the color configuration (at least for dom0less
> domain). So the colors would end up to shared with everyone (including Xen).
>
> I don't particularly mind which way you want to go, but I think we need
> some coherency. If we want to avoid innefficiency, then we should
> prevent all the setups.

I definitely don't want to check for overlapping configurations since that
could also be a desired setup, but I do want sane defaults. You found some
inchoerency there cause Xen and the domains share one color. Maybe the best
solution would be to have a default configuration for domains that doesn't
overlap with the Xen one.

Back to the original point, having a single color for the whole platform is
just stupid, not only pointless. It defeats completely the coloring idea.
On the other hand the default configuration is something that could have an
application (still pretty "basic").

In patch #5 (dom0less) there is some contradiction where Xen panics when no
configuration is provided. I also misuderstood your comment on that patch
and it can't actually panic in that case, but instead use the default
configuration.

Thanks.

> Cheers,
>
> --
> Julien Grall