Re: [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Fri, Jan 5, 2024 at 6:50 PM Julien Grall <julien@xxxxxxx> wrote:
>
>
>
> On 05/01/2024 16:52, Carlo Nonato wrote:
> > Hi Julien,
> >
> > On Thu, Jan 4, 2024 at 8:54 PM Julien Grall <julien@xxxxxxx> wrote:
> >>
> >> Hi Carlo,
> >>
> >> On 02/01/2024 09:51, Carlo Nonato wrote:
> >>> This commit allows the user to set the cache coloring configuration for
> >>> Dom0 via a command line parameter.
> >>> Since cache coloring and static memory are incompatible, direct mapping
> >>> Dom0 isn't possible when coloring is enabled.
> >>>
> >>> A common configuration syntax for cache colors is also introduced.
> >>>
> >>> 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>
> >>> ---
> >>> v5:
> >>> - Carlo Nonato as the new author
> >>> - moved dom0 colors parsing (parse_colors()) in this patch
> >>> - added dom0_set_llc_colors() to set dom0 colors after creation
> >>> - moved color allocation and checking in this patch
> >>> - error handling when allocating color arrays
> >>> - FIXME: copy pasted allocate_memory() cause it got moved
> >>> v4:
> >>> - dom0 colors are dynamically allocated as for any other domain
> >>>     (colors are duplicated in dom0_colors and in the new array, but logic
> >>>     is simpler)
> >>> ---
> >>>    docs/misc/arm/cache-coloring.rst        |  29 ++++++
> >>>    docs/misc/xen-command-line.pandoc       |   9 ++
> >>>    xen/arch/arm/domain_build.c             |  60 ++++++++++-
> >>>    xen/arch/arm/include/asm/llc-coloring.h |   1 +
> >>>    xen/arch/arm/llc-coloring.c             | 128 ++++++++++++++++++++++++
> >>>    5 files changed, 224 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/docs/misc/arm/cache-coloring.rst 
> >>> b/docs/misc/arm/cache-coloring.rst
> >>> index eabf8f5d1b..acf82c3df8 100644
> >>> --- a/docs/misc/arm/cache-coloring.rst
> >>> +++ b/docs/misc/arm/cache-coloring.rst
> >>> @@ -84,6 +84,35 @@ More specific documentation is available at 
> >>> `docs/misc/xen-command-line.pandoc`.
> >>>    +----------------------+-------------------------------+
> >>>    | ``llc-way-size``     | set the LLC way size          |
> >>>    +----------------------+-------------------------------+
> >>> +| ``dom0-llc-colors``  | Dom0 color configuration      |
> >>> ++----------------------+-------------------------------+
> >>> +
> >>> +Colors selection format
> >>> +***********************
> >>> +
> >>> +Regardless of the memory pool that has to be colored (Xen, Dom0/DomUs),
> >>> +the color selection can be expressed using the same syntax. In 
> >>> particular a
> >>> +comma-separated list of colors or ranges of colors is used.
> >>> +Ranges are hyphen-separated intervals (such as `0-4`) and are inclusive 
> >>> on both
> >>> +sides.
> >>> +
> >>> +Note that:
> >>> +
> >>> +- no spaces are allowed between values.
> >>> +- no overlapping ranges or duplicated colors are allowed.
> >>> +- values must be written in ascending order.
> >>> +
> >>> +Examples:
> >>> +
> >>> ++-------------------+-----------------------------+
> >>> +| **Configuration** | **Actual selection**        |
> >>> ++-------------------+-----------------------------+
> >>> +| 1-2,5-8           | [1, 2, 5, 6, 7, 8]          |
> >>> ++-------------------+-----------------------------+
> >>> +| 4-8,10,11,12      | [4, 5, 6, 7, 8, 10, 11, 12] |
> >>> ++-------------------+-----------------------------+
> >>> +| 0                 | [0]                         |
> >>> ++-------------------+-----------------------------+
> >>>
> >>>    Known issues and limitations
> >>>    ****************************
> >>> diff --git a/docs/misc/xen-command-line.pandoc 
> >>> b/docs/misc/xen-command-line.pandoc
> >>> index 22d2d5b6cf..51f6adf035 100644
> >>> --- a/docs/misc/xen-command-line.pandoc
> >>> +++ b/docs/misc/xen-command-line.pandoc
> >>> @@ -963,6 +963,15 @@ Controls for the dom0 IOMMU setup.
> >>>
> >>>    Specify a list of IO ports to be excluded from dom0 access.
> >>>
> >>> +### dom0-llc-colors (arm64)
> >>> +> `= List of [ <integer> | <integer>-<integer> ]`
> >>
> >> Someone reading only xen-command-line.pandoc would not know how each
> >> item of the list is separated. Can this be clarified?
> >
> > Isn't it already known that the list is comma separated? It's written at the
> > beginning of this file for the "List" type.
> > I can also point to cache-coloring documentation if needed.
>
> Ah I forgot that part. Please ignore this comment.
>
> [...]
>
> >>> +                return -EINVAL;
> >>> +            for ( color = start; color <= end; color++ )
> >>> +                colors[(*num_colors)++] = color;
> >>> +        }
> >>> +        else
> >>> +            s++;
> >>> +    }
> >>> +
> >>> +    return *s ? -EINVAL : 0;
> >>> +}
> >>> +
> >>> +static int __init parse_dom0_colors(const char *s)
> >>> +{
> >>> +    return parse_color_config(s, dom0_colors, &dom0_num_colors);
> >>> +}
> >>> +custom_param("dom0-llc-colors", parse_dom0_colors);
> >>> +
> >>>    /* Return the LLC way size by probing the hardware */
> >>>    static unsigned int __init get_llc_way_size(void)
> >>>    {
> >>> @@ -102,6 +159,28 @@ static void dump_coloring_info(unsigned char key)
> >>>        printk("Number of LLC colors supported: %u\n", nr_colors);
> >>>    }
> >>>
> >>> +static bool check_colors(unsigned int *colors, unsigned int num_colors)
> >>> +{
> >>> +    unsigned int i;
> >>> +
> >>> +    if ( num_colors > nr_colors )
> >>> +    {
> >>> +        printk(XENLOG_ERR "Number of LLC colors requested > %u\n", 
> >>> nr_colors);
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    for ( i = 0; i < num_colors; i++ )
> >>> +    {
> >>> +        if ( colors[i] >= nr_colors )
> >>> +        {
> >>> +            printk(XENLOG_ERR "LLC color %u >= %u\n", colors[i], 
> >>> nr_colors);
> >>> +            return false;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    return true;
> >>> +}
> >>> +
> >>>    bool __init llc_coloring_init(void)
> >>>    {
> >>>        if ( !llc_way_size && !(llc_way_size = get_llc_way_size()) )
> >>> @@ -150,6 +229,55 @@ void domain_dump_llc_colors(struct domain *d)
> >>>        print_colors(d->llc_colors, d->num_llc_colors);
> >>>    }
> >>>
> >>> +static int domain_alloc_colors(struct domain *d, unsigned int num_colors)
> >>> +{
> >>> +    d->num_llc_colors = num_colors;
> >>
> >> Shouldn't this be set *only* after the array was allocated?
> >
> > Yes, it works also like I did, but it's cleaner like you said.
>
> Actually, looking at the rest of the code. I think  d->num_llc_colors
> should be set outside of domain_alloc_colors().
>
> One of the reason is at least in the dom0less case, you will override
> the value afterwards.

In that case I need to allocate the array before parsing the string.
I allocate a full array then the string is parsed and the actual size is found
at the end of this phase. Knowing the actual size would require two parsing
stages. Yes I'm wasting a bit of memory by oversizing the array here. Is it
a problem?

Thanks.

> Cheers,
>
> --
> Julien Grall