Re: [PATCH 02/12] xen/arm: add cache coloring initialization for domains

[Julien Grall <julien@xxxxxxx>]

On 26/09/2022 07:39, Wei Chen wrote:
> On 2022/8/26 20:51, Carlo Nonato wrote:
> > This commit adds array pointers to domains as well as to the hypercall
> > and configuration structure employed in domain creation. The latter is 
> > used
> > both by the toolstack and by Xen itself to pass configuration data to the
> > domain creation function, so the XEN_GUEST_HANDLE macro must be 
> > adopted to be
> > able to access guest memory in the first case. This implies special 
> > care for
> > the copy of the configuration data into the domain data, meaning that a
> > discrimination variable for the two possible code paths (coming from 
> > Xen or
> > from the toolstack) is needed.
> >
> > The initialization and free functions for colored domains are also added.
> > The former is responsible for allocating and populating the color array
> > of the domain and it also checks for configuration issues. One of those
> > issues is enabling both coloring and directmap for the domain because 
> > they
> > contradicts one another. Since that, Dom0 must not be created with the
> > directmap flag.
> > The latter instead frees allocated memory.
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > ---
> >   docs/misc/arm/cache-coloring.rst    |  7 ++--
> >   xen/arch/arm/coloring.c             | 56 +++++++++++++++++++++++++++++
> >   xen/arch/arm/domain.c               | 11 ++++++
> >   xen/arch/arm/domain_build.c         | 13 +++++--
> >   xen/arch/arm/include/asm/coloring.h |  7 ++++
> >   xen/arch/arm/include/asm/domain.h   |  4 +++
> >   xen/include/public/arch-arm.h       |  8 +++++
> >   7 files changed, 102 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/misc/arm/cache-coloring.rst 
> > b/docs/misc/arm/cache-coloring.rst
> > index c7adcb0f1f..345d97cb56 100644
> > --- a/docs/misc/arm/cache-coloring.rst
> > +++ b/docs/misc/arm/cache-coloring.rst
> > @@ -13,7 +13,7 @@ In order to enable and use it, few steps are needed.
> >     (refer to menuconfig help for value meaning and when it should be 
> > changed).
> >           CONFIG_MAX_CACHE_COLORS=<n>
> > -- Assign colors to Dom0 using the `Color selection format`_ (see
> > +- Assign colors to domains using the `Color selection format`_ (see
> >     `Coloring parameters`_ for more documentation pointers).
> >   Background
> > @@ -109,4 +109,7 @@ Coloring parameters
> >   LLC way size (as previously discussed) and Dom0 colors can be set 
> > using the
> >   appropriate command line parameters. See the relevant documentation in
> > -"docs/misc/xen-command-line.pandoc".
> > \ No newline at end of file
> > +"docs/misc/xen-command-line.pandoc".
> > +
> > +Note that if no color configuration is provided for domains, they 
> > fallback to
> > +the default one, which corresponds simply to all available colors.
> > \ No newline at end of file
> > diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> > index c010ebc01b..2b37cda067 100644
> > --- a/xen/arch/arm/coloring.c
> > +++ b/xen/arch/arm/coloring.c
> > @@ -22,6 +22,7 @@
> >    * along with this program.  If not, see 
> > <http://www.gnu.org/licenses/>.
> >    */
> >   #include <xen/errno.h>
> > +#include <xen/guest_access.h>
> >   #include <xen/keyhandler.h>
> >   #include <xen/param.h>
> >   #include <xen/types.h>
> > @@ -211,6 +212,61 @@ bool __init coloring_init(void)
> >       return true;
> >   }
> > +int domain_coloring_init(struct domain *d,
> > +                         const struct xen_arch_domainconfig *config)
> > +{
> > +    if ( is_domain_direct_mapped(d) )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "Can't enable coloring and directmap at the same time 
> > for %pd\n",
> > +               d);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( is_hardware_domain(d) )
> > +    {
> > +        d->arch.colors = dom0_colors;
> > +        d->arch.num_colors = dom0_num_colors;
> > +    }
> > +    else if ( config->num_colors == 0 )
> > +    {
> > +        printk(XENLOG_WARNING
> > +               "Color config not found for %pd. Using default\n", d);
> > +        d->arch.colors = xzalloc_array(unsigned int, max_colors);
> > +        d->arch.num_colors = set_default_domain_colors(d->arch.colors);
> > +    }
> > +    else
> > +    {
> > +        d->arch.colors = xzalloc_array(unsigned int, 
> > config->num_colors);
> > +        d->arch.num_colors = config->num_colors;
> > +        if ( config->from_guest )
> > +            copy_from_guest(d->arch.colors, config->colors, 
> > config->num_colors);
> > +        else
> > +            memcpy(d->arch.colors, config->colors.p,
> > +                   sizeof(unsigned int) * config->num_colors);
> > +    }
> > +
> > +    if ( !d->arch.colors )
> > +    {
> > +        printk(XENLOG_ERR "Colors allocation failed for %pd\n", d);
> > +        return -ENOMEM;
> > +    }
> > +
> > +    if ( !check_colors(d->arch.colors, d->arch.num_colors) )
> > +    {
> If we add xfree(d->arch.colors) here for non-hw domains, is it possible 
> to make this function have a complete fallback process? And I know 
> currently, this is handled in domain_coloring_free.
arch_domain_destroy() (and therefore domain_coloring_free()) will always 
be called by arch_domain_create(). So here you will want to use XFREE() 
to avoid a double free.
However, I would just rely on the free() in domain_coloring_free().

Cheers,

--
Julien Grall