* [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-24 16:37 ` Jan Beulich
2023-01-23 15:47 ` [PATCH v4 02/11] xen/arm: add cache coloring initialization Carlo Nonato
` (10 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Marco Solieri
This commit adds the Last Level Cache (LLC) coloring common header,
Kconfig options and stub functions for domain coloring. Since this is an
arch specific feature, actual implementation is postponed to later patches
and Kconfig options are placed under xen/arch.
LLC colors are represented as dynamic arrays plus their size and since
they have to be passed during domain creation, domain_create() is replaced
by domain_create_llc_colored(). domain_create() is then turned into a
wrapper of the colored version to let all the original call sites remain
untouched.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- Kconfig options moved to xen/arch
- removed range for CONFIG_NR_LLC_COLORS
- added "llc_coloring_enabled" global to later implement the boot-time
switch
- added domain_create_llc_colored() to be able to pass colors
- added is_domain_llc_colored() macro
---
xen/arch/Kconfig | 17 +++++++++++
xen/common/Kconfig | 3 ++
xen/common/domain.c | 23 +++++++++++++--
xen/common/keyhandler.c | 4 +++
xen/include/xen/llc_coloring.h | 54 ++++++++++++++++++++++++++++++++++
xen/include/xen/sched.h | 9 ++++++
6 files changed, 107 insertions(+), 3 deletions(-)
create mode 100644 xen/include/xen/llc_coloring.h
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 7028f7b74f..39c23f2528 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -28,3 +28,20 @@ config NR_NUMA_NODES
associated with multiple-nodes management. It is the upper bound of
the number of NUMA nodes that the scheduler, memory allocation and
other NUMA-aware components can handle.
+
+config LLC_COLORING
+ bool "Last Level Cache (LLC) coloring" if EXPERT
+ depends on HAS_LLC_COLORING
+
+config NR_LLC_COLORS
+ int "Maximum number of LLC colors"
+ default 128
+ depends on LLC_COLORING
+ help
+ Controls the build-time size of various arrays associated with LLC
+ coloring. Refer to the documentation for how to compute the number
+ of colors supported by the platform.
+ The default value corresponds to an 8 MiB 16-ways LLC, which should be
+ more than what needed in the general case.
+ Note that if, at any time, a color configuration with more colors than
+ the maximum is employed, an error is produced.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f1ea3199c8..c796c633f1 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -49,6 +49,9 @@ config HAS_IOPORTS
config HAS_KEXEC
bool
+config HAS_LLC_COLORING
+ bool
+
config HAS_PDX
bool
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 626debbae0..87aae86081 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -7,6 +7,7 @@
#include <xen/compat.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/llc_coloring.h>
#include <xen/ctype.h>
#include <xen/err.h>
#include <xen/param.h>
@@ -549,9 +550,11 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
return arch_sanitise_domain_config(config);
}
-struct domain *domain_create(domid_t domid,
- struct xen_domctl_createdomain *config,
- unsigned int flags)
+struct domain *domain_create_llc_colored(domid_t domid,
+ struct xen_domctl_createdomain *config,
+ unsigned int flags,
+ unsigned int *llc_colors,
+ unsigned int num_llc_colors)
{
struct domain *d, **pd, *old_hwdom = NULL;
enum { INIT_watchdog = 1u<<1,
@@ -663,6 +666,10 @@ struct domain *domain_create(domid_t domid,
d->nr_pirqs = min(d->nr_pirqs, nr_irqs);
radix_tree_init(&d->pirq_tree);
+
+ if ( llc_coloring_enabled &&
+ (err = domain_llc_coloring_init(d, llc_colors, num_llc_colors)) )
+ return ERR_PTR(err);
}
if ( (err = arch_domain_create(d, config, flags)) != 0 )
@@ -769,6 +776,13 @@ struct domain *domain_create(domid_t domid,
return ERR_PTR(err);
}
+struct domain *domain_create(domid_t domid,
+ struct xen_domctl_createdomain *config,
+ unsigned int flags)
+{
+ return domain_create_llc_colored(domid, config, flags, 0, 0);
+}
+
void __init setup_system_domains(void)
{
/*
@@ -1103,6 +1117,9 @@ static void cf_check complete_domain_destroy(struct rcu_head *head)
struct vcpu *v;
int i;
+ if ( is_domain_llc_colored(d) )
+ domain_llc_coloring_free(d);
+
/*
* Flush all state for the vCPU previously having run on the current CPU.
* This is in particular relevant for x86 HVM ones on VMX, so that this
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 0a551033c4..56f7731595 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -6,6 +6,7 @@
#include <xen/debugger.h>
#include <xen/delay.h>
#include <xen/keyhandler.h>
+#include <xen/llc_coloring.h>
#include <xen/param.h>
#include <xen/shutdown.h>
#include <xen/event.h>
@@ -307,6 +308,9 @@ static void cf_check dump_domains(unsigned char key)
arch_dump_domain_info(d);
+ if ( is_domain_llc_colored(d) )
+ domain_dump_llc_colors(d);
+
rangeset_domain_printk(d);
dump_pageframe_info(d);
diff --git a/xen/include/xen/llc_coloring.h b/xen/include/xen/llc_coloring.h
new file mode 100644
index 0000000000..625930d378
--- /dev/null
+++ b/xen/include/xen/llc_coloring.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Last Level Cache (LLC) coloring common header
+ *
+ * Copyright (C) 2022 Xilinx Inc.
+ *
+ * Authors:
+ * Carlo Nonato <carlo.nonato@minervasys.tech>
+ */
+#ifndef __COLORING_H__
+#define __COLORING_H__
+
+#include <xen/sched.h>
+#include <public/domctl.h>
+
+#ifdef CONFIG_HAS_LLC_COLORING
+
+#include <asm/llc_coloring.h>
+
+extern bool llc_coloring_enabled;
+
+int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
+ unsigned int num_colors);
+void domain_llc_coloring_free(struct domain *d);
+void domain_dump_llc_colors(struct domain *d);
+
+#else
+
+#define llc_coloring_enabled (false)
+
+static inline int domain_llc_coloring_init(struct domain *d,
+ unsigned int *colors,
+ unsigned int num_colors)
+{
+ return 0;
+}
+static inline void domain_llc_coloring_free(struct domain *d) {}
+static inline void domain_dump_llc_colors(struct domain *d) {}
+
+#endif /* CONFIG_HAS_LLC_COLORING */
+
+#define is_domain_llc_colored(d) (llc_coloring_enabled)
+
+#endif /* __COLORING_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
\ No newline at end of file
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 12be794002..754f6cb1da 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -602,6 +602,9 @@ struct domain
/* Holding CDF_* constant. Internal flags for domain creation. */
unsigned int cdf;
+
+ unsigned int *llc_colors;
+ unsigned int num_llc_colors;
};
static inline struct page_list_head *page_to_list(
@@ -685,6 +688,12 @@ static inline void domain_update_node_affinity(struct domain *d)
*/
int arch_sanitise_domain_config(struct xen_domctl_createdomain *config);
+struct domain *domain_create_llc_colored(domid_t domid,
+ struct xen_domctl_createdomain *config,
+ unsigned int flags,
+ unsigned int *colors,
+ unsigned int num_colors);
+
/*
* Create a domain: the configuration is only necessary for real domain
* (domid < DOMID_FIRST_RESERVED).
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-23 15:47 ` [PATCH v4 01/11] xen/common: add cache coloring common code Carlo Nonato
@ 2023-01-24 16:37 ` Jan Beulich
2023-01-25 11:18 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-24 16:37 UTC (permalink / raw)
To: Carlo Nonato
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Marco Solieri, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> @@ -769,6 +776,13 @@ struct domain *domain_create(domid_t domid,
> return ERR_PTR(err);
> }
>
> +struct domain *domain_create(domid_t domid,
> + struct xen_domctl_createdomain *config,
> + unsigned int flags)
> +{
> + return domain_create_llc_colored(domid, config, flags, 0, 0);
Please can you use NULL when you mean a null pointer?
> --- /dev/null
> +++ b/xen/include/xen/llc_coloring.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Last Level Cache (LLC) coloring common header
> + *
> + * Copyright (C) 2022 Xilinx Inc.
> + *
> + * Authors:
> + * Carlo Nonato <carlo.nonato@minervasys.tech>
> + */
> +#ifndef __COLORING_H__
> +#define __COLORING_H__
> +
> +#include <xen/sched.h>
> +#include <public/domctl.h>
> +
> +#ifdef CONFIG_HAS_LLC_COLORING
> +
> +#include <asm/llc_coloring.h>
> +
> +extern bool llc_coloring_enabled;
> +
> +int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
> + unsigned int num_colors);
> +void domain_llc_coloring_free(struct domain *d);
> +void domain_dump_llc_colors(struct domain *d);
> +
> +#else
> +
> +#define llc_coloring_enabled (false)
While I agree this is needed, ...
> +static inline int domain_llc_coloring_init(struct domain *d,
> + unsigned int *colors,
> + unsigned int num_colors)
> +{
> + return 0;
> +}
> +static inline void domain_llc_coloring_free(struct domain *d) {}
> +static inline void domain_dump_llc_colors(struct domain *d) {}
... I don't think you need any of these. Instead the declarations above
simply need to be visible unconditionally (to be visible to the compiler
when processing consuming code). We rely on DCE to remove such references
in many other places.
> +#endif /* CONFIG_HAS_LLC_COLORING */
> +
> +#define is_domain_llc_colored(d) (llc_coloring_enabled)
> +
> +#endif /* __COLORING_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> \ No newline at end of file
This wants taking care of.
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -602,6 +602,9 @@ struct domain
>
> /* Holding CDF_* constant. Internal flags for domain creation. */
> unsigned int cdf;
> +
> + unsigned int *llc_colors;
> + unsigned int num_llc_colors;
> };
Why outside of any #ifdef, and why not in struct arch_domain?
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-24 16:37 ` Jan Beulich
@ 2023-01-25 11:18 ` Carlo Nonato
2023-01-25 13:10 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-25 11:18 UTC (permalink / raw)
To: Jan Beulich, Julien Grall
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel
Hi Jan, Julien
On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > @@ -769,6 +776,13 @@ struct domain *domain_create(domid_t domid,
> > return ERR_PTR(err);
> > }
> >
> > +struct domain *domain_create(domid_t domid,
> > + struct xen_domctl_createdomain *config,
> > + unsigned int flags)
> > +{
> > + return domain_create_llc_colored(domid, config, flags, 0, 0);
>
> Please can you use NULL when you mean a null pointer?
>
> > --- /dev/null
> > +++ b/xen/include/xen/llc_coloring.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Last Level Cache (LLC) coloring common header
> > + *
> > + * Copyright (C) 2022 Xilinx Inc.
> > + *
> > + * Authors:
> > + * Carlo Nonato <carlo.nonato@minervasys.tech>
> > + */
> > +#ifndef __COLORING_H__
> > +#define __COLORING_H__
> > +
> > +#include <xen/sched.h>
> > +#include <public/domctl.h>
> > +
> > +#ifdef CONFIG_HAS_LLC_COLORING
> > +
> > +#include <asm/llc_coloring.h>
> > +
> > +extern bool llc_coloring_enabled;
> > +
> > +int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
> > + unsigned int num_colors);
> > +void domain_llc_coloring_free(struct domain *d);
> > +void domain_dump_llc_colors(struct domain *d);
> > +
> > +#else
> > +
> > +#define llc_coloring_enabled (false)
>
> While I agree this is needed, ...
>
> > +static inline int domain_llc_coloring_init(struct domain *d,
> > + unsigned int *colors,
> > + unsigned int num_colors)
> > +{
> > + return 0;
> > +}
> > +static inline void domain_llc_coloring_free(struct domain *d) {}
> > +static inline void domain_dump_llc_colors(struct domain *d) {}
>
> ... I don't think you need any of these. Instead the declarations above
> simply need to be visible unconditionally (to be visible to the compiler
> when processing consuming code). We rely on DCE to remove such references
> in many other places.
So this is true for any other stub function that I used in the series, right?
Since all of them are guarded by the same kind of if statement: checking for
llc_coloring_enabled value which, in case of coloring disabled from Kconfig,
is always false and then DCE comes in. Sorry for being so verbose, but I just
want to be sure I understood.
> > +#endif /* CONFIG_HAS_LLC_COLORING */
> > +
> > +#define is_domain_llc_colored(d) (llc_coloring_enabled)
> > +
> > +#endif /* __COLORING_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > \ No newline at end of file
>
> This wants taking care of.
>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -602,6 +602,9 @@ struct domain
> >
> > /* Holding CDF_* constant. Internal flags for domain creation. */
> > unsigned int cdf;
> > +
> > + unsigned int *llc_colors;
> > + unsigned int num_llc_colors;
> > };
>
> Why outside of any #ifdef, and why not in struct arch_domain?
Moving this in sched.h seemed like the natural continuation of the common +
arch specific split. Notice that this split is also because Julien pointed
out (as you did in some earlier revision) that cache coloring can be used
by other arch in the future (even if x86 is excluded). Having two maintainers
saying the same thing sounded like a good reason to do that.
The missing #ifdef comes from a discussion I had with Julien in v2 about
domctl interface where he suggested removing it
(https://marc.info/?l=xen-devel&m=166151802002263). We were talking about
a different struct, but I thought the principle was the same. Anyway I would
like the #ifdef too.
So @Jan, @Julien, can you help me fix this once for all?
> Jan
Thanks.
- Carlo Nonato
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-25 11:18 ` Carlo Nonato
@ 2023-01-25 13:10 ` Jan Beulich
2023-01-25 16:18 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-25 13:10 UTC (permalink / raw)
To: Carlo Nonato
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel, Julien Grall
On 25.01.2023 12:18, Carlo Nonato wrote:
> On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/llc_coloring.h
>>> @@ -0,0 +1,54 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Last Level Cache (LLC) coloring common header
>>> + *
>>> + * Copyright (C) 2022 Xilinx Inc.
>>> + *
>>> + * Authors:
>>> + * Carlo Nonato <carlo.nonato@minervasys.tech>
>>> + */
>>> +#ifndef __COLORING_H__
>>> +#define __COLORING_H__
>>> +
>>> +#include <xen/sched.h>
>>> +#include <public/domctl.h>
>>> +
>>> +#ifdef CONFIG_HAS_LLC_COLORING
>>> +
>>> +#include <asm/llc_coloring.h>
>>> +
>>> +extern bool llc_coloring_enabled;
>>> +
>>> +int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
>>> + unsigned int num_colors);
>>> +void domain_llc_coloring_free(struct domain *d);
>>> +void domain_dump_llc_colors(struct domain *d);
>>> +
>>> +#else
>>> +
>>> +#define llc_coloring_enabled (false)
>>
>> While I agree this is needed, ...
>>
>>> +static inline int domain_llc_coloring_init(struct domain *d,
>>> + unsigned int *colors,
>>> + unsigned int num_colors)
>>> +{
>>> + return 0;
>>> +}
>>> +static inline void domain_llc_coloring_free(struct domain *d) {}
>>> +static inline void domain_dump_llc_colors(struct domain *d) {}
>>
>> ... I don't think you need any of these. Instead the declarations above
>> simply need to be visible unconditionally (to be visible to the compiler
>> when processing consuming code). We rely on DCE to remove such references
>> in many other places.
>
> So this is true for any other stub function that I used in the series, right?
Likely. I didn't look at most of the Arm-only pieces.
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -602,6 +602,9 @@ struct domain
>>>
>>> /* Holding CDF_* constant. Internal flags for domain creation. */
>>> unsigned int cdf;
>>> +
>>> + unsigned int *llc_colors;
>>> + unsigned int num_llc_colors;
>>> };
>>
>> Why outside of any #ifdef, and why not in struct arch_domain?
>
> Moving this in sched.h seemed like the natural continuation of the common +
> arch specific split. Notice that this split is also because Julien pointed
> out (as you did in some earlier revision) that cache coloring can be used
> by other arch in the future (even if x86 is excluded). Having two maintainers
> saying the same thing sounded like a good reason to do that.
If you mean this to be usable by other arch-es as well (which I would
welcome, as I think I had expressed on an earlier version), then I think
more pieces want to be in common code. But putting the fields here and all
users of them in arch-specific code (which I think is the way I saw it)
doesn't look very logical to me. IOW to me there exist only two possible
approaches: As much as possible in common code, or common code being
disturbed as little as possible.
> The missing #ifdef comes from a discussion I had with Julien in v2 about
> domctl interface where he suggested removing it
> (https://marc.info/?l=xen-devel&m=166151802002263).
I went about five levels deep in the replies, without finding any such reply
from Julien. Can you please be more specific with the link, so readers don't
need to endlessly dig?
Jan
> We were talking about
> a different struct, but I thought the principle was the same. Anyway I would
> like the #ifdef too.
>
> So @Jan, @Julien, can you help me fix this once for all?
>
> Thanks.
>
> - Carlo Nonato
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-25 13:10 ` Jan Beulich
@ 2023-01-25 16:18 ` Carlo Nonato
2023-01-26 8:06 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-25 16:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel, Julien Grall
On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.01.2023 12:18, Carlo Nonato wrote:
> > On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 23.01.2023 16:47, Carlo Nonato wrote:
> >>> --- /dev/null
> >>> +++ b/xen/include/xen/llc_coloring.h
> >>> @@ -0,0 +1,54 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +/*
> >>> + * Last Level Cache (LLC) coloring common header
> >>> + *
> >>> + * Copyright (C) 2022 Xilinx Inc.
> >>> + *
> >>> + * Authors:
> >>> + * Carlo Nonato <carlo.nonato@minervasys.tech>
> >>> + */
> >>> +#ifndef __COLORING_H__
> >>> +#define __COLORING_H__
> >>> +
> >>> +#include <xen/sched.h>
> >>> +#include <public/domctl.h>
> >>> +
> >>> +#ifdef CONFIG_HAS_LLC_COLORING
> >>> +
> >>> +#include <asm/llc_coloring.h>
> >>> +
> >>> +extern bool llc_coloring_enabled;
> >>> +
> >>> +int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
> >>> + unsigned int num_colors);
> >>> +void domain_llc_coloring_free(struct domain *d);
> >>> +void domain_dump_llc_colors(struct domain *d);
> >>> +
> >>> +#else
> >>> +
> >>> +#define llc_coloring_enabled (false)
> >>
> >> While I agree this is needed, ...
> >>
> >>> +static inline int domain_llc_coloring_init(struct domain *d,
> >>> + unsigned int *colors,
> >>> + unsigned int num_colors)
> >>> +{
> >>> + return 0;
> >>> +}
> >>> +static inline void domain_llc_coloring_free(struct domain *d) {}
> >>> +static inline void domain_dump_llc_colors(struct domain *d) {}
> >>
> >> ... I don't think you need any of these. Instead the declarations above
> >> simply need to be visible unconditionally (to be visible to the compiler
> >> when processing consuming code). We rely on DCE to remove such references
> >> in many other places.
> >
> > So this is true for any other stub function that I used in the series, right?
>
> Likely. I didn't look at most of the Arm-only pieces.
>
> >>> --- a/xen/include/xen/sched.h
> >>> +++ b/xen/include/xen/sched.h
> >>> @@ -602,6 +602,9 @@ struct domain
> >>>
> >>> /* Holding CDF_* constant. Internal flags for domain creation. */
> >>> unsigned int cdf;
> >>> +
> >>> + unsigned int *llc_colors;
> >>> + unsigned int num_llc_colors;
> >>> };
> >>
> >> Why outside of any #ifdef, and why not in struct arch_domain?
> >
> > Moving this in sched.h seemed like the natural continuation of the common +
> > arch specific split. Notice that this split is also because Julien pointed
> > out (as you did in some earlier revision) that cache coloring can be used
> > by other arch in the future (even if x86 is excluded). Having two maintainers
> > saying the same thing sounded like a good reason to do that.
>
> If you mean this to be usable by other arch-es as well (which I would
> welcome, as I think I had expressed on an earlier version), then I think
> more pieces want to be in common code. But putting the fields here and all
> users of them in arch-specific code (which I think is the way I saw it)
> doesn't look very logical to me. IOW to me there exist only two possible
> approaches: As much as possible in common code, or common code being
> disturbed as little as possible.
This means having a llc-coloring.c in common where to put the common
implementation, right?
Anyway right now there is also another user of such fields in common:
page_alloc.c.
> > The missing #ifdef comes from a discussion I had with Julien in v2 about
> > domctl interface where he suggested removing it
> > (https://marc.info/?l=xen-devel&m=166151802002263).
>
> I went about five levels deep in the replies, without finding any such reply
> from Julien. Can you please be more specific with the link, so readers don't
> need to endlessly dig?
https://marc.info/?l=xen-devel&m=166669617917298
quote (me and then Julien):
>> We can also think of moving the coloring fields from this
>> struct to the common one (xen_domctl_createdomain) protecting them with
>> the proper #ifdef (but we are targeting only arm64...).
> Your code is targeting arm64 but fundamentally this is an arm64 specific
> feature. IOW, this could be used in the future on other arch. So I think
> it would make sense to define it in common without the #ifdef.
> Jan
>
> > We were talking about
> > a different struct, but I thought the principle was the same. Anyway I would
> > like the #ifdef too.
> >
> > So @Jan, @Julien, can you help me fix this once for all?
> >
> > Thanks.
> >
> > - Carlo Nonato
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-25 16:18 ` Carlo Nonato
@ 2023-01-26 8:06 ` Jan Beulich
2023-01-26 10:15 ` Julien Grall
0 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-26 8:06 UTC (permalink / raw)
To: Carlo Nonato
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel, Julien Grall
On 25.01.2023 17:18, Carlo Nonato wrote:
> On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 25.01.2023 12:18, Carlo Nonato wrote:
>>> On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>>>> --- a/xen/include/xen/sched.h
>>>>> +++ b/xen/include/xen/sched.h
>>>>> @@ -602,6 +602,9 @@ struct domain
>>>>>
>>>>> /* Holding CDF_* constant. Internal flags for domain creation. */
>>>>> unsigned int cdf;
>>>>> +
>>>>> + unsigned int *llc_colors;
>>>>> + unsigned int num_llc_colors;
>>>>> };
>>>>
>>>> Why outside of any #ifdef, and why not in struct arch_domain?
>>>
>>> Moving this in sched.h seemed like the natural continuation of the common +
>>> arch specific split. Notice that this split is also because Julien pointed
>>> out (as you did in some earlier revision) that cache coloring can be used
>>> by other arch in the future (even if x86 is excluded). Having two maintainers
>>> saying the same thing sounded like a good reason to do that.
>>
>> If you mean this to be usable by other arch-es as well (which I would
>> welcome, as I think I had expressed on an earlier version), then I think
>> more pieces want to be in common code. But putting the fields here and all
>> users of them in arch-specific code (which I think is the way I saw it)
>> doesn't look very logical to me. IOW to me there exist only two possible
>> approaches: As much as possible in common code, or common code being
>> disturbed as little as possible.
>
> This means having a llc-coloring.c in common where to put the common
> implementation, right?
Likely, yes.
> Anyway right now there is also another user of such fields in common:
> page_alloc.c.
Yet hopefully all inside suitable #ifdef.
>>> The missing #ifdef comes from a discussion I had with Julien in v2 about
>>> domctl interface where he suggested removing it
>>> (https://marc.info/?l=xen-devel&m=166151802002263).
>>
>> I went about five levels deep in the replies, without finding any such reply
>> from Julien. Can you please be more specific with the link, so readers don't
>> need to endlessly dig?
>
> https://marc.info/?l=xen-devel&m=166669617917298
>
> quote (me and then Julien):
>>> We can also think of moving the coloring fields from this
>>> struct to the common one (xen_domctl_createdomain) protecting them with
>>> the proper #ifdef (but we are targeting only arm64...).
>
>> Your code is targeting arm64 but fundamentally this is an arm64 specific
>> feature. IOW, this could be used in the future on other arch. So I think
>> it would make sense to define it in common without the #ifdef.
I'm inclined to read this as a dislike for "#ifdef CONFIG_ARM64", not for
"#ifdef CONFIG_LLC_COLORING" (or whatever the name of the option was). But
I guess only Julien can clarify this ...
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-26 8:06 ` Jan Beulich
@ 2023-01-26 10:15 ` Julien Grall
2023-01-26 11:03 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2023-01-26 10:15 UTC (permalink / raw)
To: Jan Beulich, Carlo Nonato
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel
Hi Jan,
On 26/01/2023 08:06, Jan Beulich wrote:
> On 25.01.2023 17:18, Carlo Nonato wrote:
>> On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> On 25.01.2023 12:18, Carlo Nonato wrote:
>>>> On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -602,6 +602,9 @@ struct domain
>>>>>>
>>>>>> /* Holding CDF_* constant. Internal flags for domain creation. */
>>>>>> unsigned int cdf;
>>>>>> +
>>>>>> + unsigned int *llc_colors;
>>>>>> + unsigned int num_llc_colors;
>>>>>> };
>>>>>
>>>>> Why outside of any #ifdef, and why not in struct arch_domain?
>>>>
>>>> Moving this in sched.h seemed like the natural continuation of the common +
>>>> arch specific split. Notice that this split is also because Julien pointed
>>>> out (as you did in some earlier revision) that cache coloring can be used
>>>> by other arch in the future (even if x86 is excluded). Having two maintainers
>>>> saying the same thing sounded like a good reason to do that.
>>>
>>> If you mean this to be usable by other arch-es as well (which I would
>>> welcome, as I think I had expressed on an earlier version), then I think
>>> more pieces want to be in common code. But putting the fields here and all
>>> users of them in arch-specific code (which I think is the way I saw it)
>>> doesn't look very logical to me. IOW to me there exist only two possible
>>> approaches: As much as possible in common code, or common code being
>>> disturbed as little as possible.
>>
>> This means having a llc-coloring.c in common where to put the common
>> implementation, right?
>
> Likely, yes.
>
>> Anyway right now there is also another user of such fields in common:
>> page_alloc.c.
>
> Yet hopefully all inside suitable #ifdef.
>
>>>> The missing #ifdef comes from a discussion I had with Julien in v2 about
>>>> domctl interface where he suggested removing it
>>>> (https://marc.info/?l=xen-devel&m=166151802002263).
>>>
>>> I went about five levels deep in the replies, without finding any such reply
>>> from Julien. Can you please be more specific with the link, so readers don't
>>> need to endlessly dig?
>>
>> https://marc.info/?l=xen-devel&m=166669617917298
>>
>> quote (me and then Julien):
>>>> We can also think of moving the coloring fields from this
>>>> struct to the common one (xen_domctl_createdomain) protecting them with
>>>> the proper #ifdef (but we are targeting only arm64...).
>>
>>> Your code is targeting arm64 but fundamentally this is an arm64 specific
>>> feature. IOW, this could be used in the future on other arch. So I think
>>> it would make sense to define it in common without the #ifdef.
>
> I'm inclined to read this as a dislike for "#ifdef CONFIG_ARM64", not for
> "#ifdef CONFIG_LLC_COLORING" (or whatever the name of the option was). But
> I guess only Julien can clarify this ...
Your interpretation is correct. I would prefer if the fields are
protected with #ifdef CONFIG_LLC_COLORING.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 01/11] xen/common: add cache coloring common code
2023-01-26 10:15 ` Julien Grall
@ 2023-01-26 11:03 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 11:03 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Marco Solieri, xen-devel
Hi Julien and Jan,
On Thu, Jan 26, 2023 at 11:16 AM Julien Grall <julien@xen.org> wrote:
>
> Hi Jan,
>
> On 26/01/2023 08:06, Jan Beulich wrote:
> > On 25.01.2023 17:18, Carlo Nonato wrote:
> >> On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>> On 25.01.2023 12:18, Carlo Nonato wrote:
> >>>> On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 23.01.2023 16:47, Carlo Nonato wrote:
> >>>>>> --- a/xen/include/xen/sched.h
> >>>>>> +++ b/xen/include/xen/sched.h
> >>>>>> @@ -602,6 +602,9 @@ struct domain
> >>>>>>
> >>>>>> /* Holding CDF_* constant. Internal flags for domain creation. */
> >>>>>> unsigned int cdf;
> >>>>>> +
> >>>>>> + unsigned int *llc_colors;
> >>>>>> + unsigned int num_llc_colors;
> >>>>>> };
> >>>>>
> >>>>> Why outside of any #ifdef, and why not in struct arch_domain?
> >>>>
> >>>> Moving this in sched.h seemed like the natural continuation of the common +
> >>>> arch specific split. Notice that this split is also because Julien pointed
> >>>> out (as you did in some earlier revision) that cache coloring can be used
> >>>> by other arch in the future (even if x86 is excluded). Having two maintainers
> >>>> saying the same thing sounded like a good reason to do that.
> >>>
> >>> If you mean this to be usable by other arch-es as well (which I would
> >>> welcome, as I think I had expressed on an earlier version), then I think
> >>> more pieces want to be in common code. But putting the fields here and all
> >>> users of them in arch-specific code (which I think is the way I saw it)
> >>> doesn't look very logical to me. IOW to me there exist only two possible
> >>> approaches: As much as possible in common code, or common code being
> >>> disturbed as little as possible.
> >>
> >> This means having a llc-coloring.c in common where to put the common
> >> implementation, right?
> >
> > Likely, yes.
> >
> >> Anyway right now there is also another user of such fields in common:
> >> page_alloc.c.
> >
> > Yet hopefully all inside suitable #ifdef.
> >
> >>>> The missing #ifdef comes from a discussion I had with Julien in v2 about
> >>>> domctl interface where he suggested removing it
> >>>> (https://marc.info/?l=xen-devel&m=166151802002263).
> >>>
> >>> I went about five levels deep in the replies, without finding any such reply
> >>> from Julien. Can you please be more specific with the link, so readers don't
> >>> need to endlessly dig?
> >>
> >> https://marc.info/?l=xen-devel&m=166669617917298
> >>
> >> quote (me and then Julien):
> >>>> We can also think of moving the coloring fields from this
> >>>> struct to the common one (xen_domctl_createdomain) protecting them with
> >>>> the proper #ifdef (but we are targeting only arm64...).
> >>
> >>> Your code is targeting arm64 but fundamentally this is an arm64 specific
> >>> feature. IOW, this could be used in the future on other arch. So I think
> >>> it would make sense to define it in common without the #ifdef.
> >
> > I'm inclined to read this as a dislike for "#ifdef CONFIG_ARM64", not for
> > "#ifdef CONFIG_LLC_COLORING" (or whatever the name of the option was). But
> > I guess only Julien can clarify this ...
> Your interpretation is correct. I would prefer if the fields are
> protected with #ifdef CONFIG_LLC_COLORING.
Understood. Thanks to both.
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 02/11] xen/arm: add cache coloring initialization
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 01/11] xen/common: add cache coloring common code Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-24 16:20 ` Jan Beulich
2023-01-23 15:47 ` [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support Carlo Nonato
` (9 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Marco Solieri
This commit implements functions declared in the LLC coloring common header
for arm64 and adds documentation. It also adds two command line options: a
runtime switch for the cache coloring feature and the LLC way size
parameter.
The feature init function consists of an auto probing of the cache layout
necessary to retrieve the LLC way size which is used to compute the number
of platform colors. It also adds a debug-key to dump general cache coloring
info.
The domain init function, instead, allocates default colors if needed and
checks the provided configuration for errors.
Note that until this patch, there are no implemented methods for actually
configuring cache colors for domains and all the configurations fall back
to the default one.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- added "llc-coloring" cmdline option for the boot-time switch
- dom0 colors are now checked during domain init as for any other domain
- fixed processor.h masks bit width
- check for overflow in parse_color_config()
- check_colors() now checks also that colors are sorted and unique
---
docs/misc/arm/cache-coloring.rst | 105 +++++++++
docs/misc/xen-command-line.pandoc | 37 ++++
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/Makefile | 1 +
xen/arch/arm/include/asm/llc_coloring.h | 36 ++++
xen/arch/arm/include/asm/processor.h | 16 ++
xen/arch/arm/llc_coloring.c | 272 ++++++++++++++++++++++++
xen/arch/arm/setup.c | 7 +
8 files changed, 475 insertions(+)
create mode 100644 docs/misc/arm/cache-coloring.rst
create mode 100644 xen/arch/arm/include/asm/llc_coloring.h
create mode 100644 xen/arch/arm/llc_coloring.c
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
new file mode 100644
index 0000000000..0244d2f606
--- /dev/null
+++ b/docs/misc/arm/cache-coloring.rst
@@ -0,0 +1,105 @@
+Xen cache coloring user guide
+=============================
+
+The cache coloring support in Xen allows to reserve Last Level Cache (LLC)
+partitions for Dom0, DomUs and Xen itself. Currently only ARM64 is supported.
+
+In order to enable and use it, few steps are needed.
+
+In Kconfig:
+
+- Enable LLC coloring.
+
+ CONFIG_LLC_COLORING=y
+- If needed, change the maximum number of colors (refer to menuconfig help for
+ value meaning and when it should be changed).
+
+ CONFIG_NR_LLC_COLORS=<n>
+
+Compile Xen and the toolstack and then:
+
+- Set the `llc-coloring=on` command line option.
+- Set `Coloring parameters and domain configurations`_.
+
+Background
+**********
+
+Cache hierarchy of a modern multi-core CPU typically has first levels dedicated
+to each core (hence using multiple cache units), while the last level is shared
+among all of them. Such configuration implies that memory operations on one
+core (e.g. running a DomU) are able to generate interference on another core
+(e.g .hosting another DomU). Cache coloring allows eliminating this
+mutual interference, and thus guaranteeing higher and more predictable
+performances for memory accesses.
+The key concept underlying cache coloring is a fragmentation of the memory
+space into a set of sub-spaces called colors that are mapped to disjoint cache
+partitions. Technically, the whole memory space is first divided into a number
+of subsequent regions. Then each region is in turn divided into a number of
+subsequent sub-colors. The generic i-th color is then obtained by all the
+i-th sub-colors in each region.
+
+.. raw:: html
+
+ <pre>
+ Region j Region j+1
+ ..................... ............
+ . . .
+ . .
+ _ _ _______________ _ _____________________ _ _
+ | | | | | | |
+ | c_0 | c_1 | | c_n | c_0 | c_1 |
+ _ _ _|_____|_____|_ _ _|_____|_____|_____|_ _ _
+ : :
+ : :... ... .
+ : color 0
+ :........................... ... .
+ :
+ . . ..................................:
+ </pre>
+
+There are two pragmatic lesson to be learnt.
+
+1. If one wants to avoid cache interference between two domains, different
+ colors needs to be used for their memory.
+
+2. Color assignment must privilege contiguity in the partitioning. E.g.,
+ assigning colors (0,1) to domain I and (2,3) to domain J is better than
+ assigning colors (0,2) to I and (1,3) to J.
+
+How to compute the number of colors
+***********************************
+
+To compute the number of available colors for a specific platform, the size of
+an LLC way and the page size used by Xen must be known. The first parameter can
+be found in the processor manual or can be also computed dividing the total
+cache size by the number of its ways. The second parameter is the minimum
+amount of memory that can be mapped by the hypervisor, thus dividing the way
+size by the page size, the number of total cache partitions is found. So for
+example, an Arm Cortex-A53 with a 16-ways associative 1 MiB LLC, can isolate up
+to 16 colors when pages are 4 KiB in size.
+
+Cache layout is probed automatically by Xen itself, but a possibility to
+manually set the way size it's left for the user to overcome failing situations
+or for debugging/testing purposes. See `Coloring parameters and domain
+configurations`_ section for more information on that.
+
+Coloring parameters and domain configurations
+*********************************************
+
+LLC way size (as previously discussed) can be set using the appropriate command
+line parameter. See the relevant documentation in
+"docs/misc/xen-command-line.pandoc".
+
+**Note:** If no color configuration is provided for a domain, the default one,
+which corresponds to all available colors, is used instead.
+
+Known issues and limitations
+****************************
+
+"xen,static-mem" isn't supported when coloring is enabled
+#########################################################
+
+In the domain configuration, "xen,static-mem" allows memory to be statically
+allocated to the domain. This isn't possibile when LLC coloring is enabled,
+because that memory can't be guaranteed to use only colors assigned to the
+domain.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 923910f553..eb105c03af 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -908,6 +908,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> ]`
+
+> Default: `All available LLC colors`
+
+Specify dom0 LLC color configuration. This options is available only when
+`CONFIG_LLC_COLORING` is enabled. If the parameter is not set, all available
+colors are chosen and the user is warned on Xen serial console.
+
### dom0_max_vcpus
Either:
@@ -1645,6 +1654,34 @@ This option is intended for debugging purposes only. Enable MSR_DEBUGCTL.LBR
in hypervisor context to be able to dump the Last Interrupt/Exception To/From
record with other registers.
+### llc-coloring (arm64)
+> `= <boolean>`
+
+> Default: `false`
+
+Flag to enable or disable LLC coloring support at runtime. This options is
+available only when `CONFIG_LLC_COLORING` is enabled. See the general
+cache coloring documentation for more info.
+
+### llc-way-size (arm64)
+> `= <size>`
+
+> Default: `Obtained from the hardware`
+
+Specify the way size of the Last Level Cache. This options is available only
+when `CONFIG_LLC_COLORING` is enabled. It is an optional, expert-only parameter
+and it is used to calculate the number of available LLC colors on the platform.
+It can be obtained by dividing the total LLC size by the number of its
+associative ways.
+By default, the value is automatically computed by probing the hardware, but in
+case of specific needs, it can be manually set. Those include failing probing
+and debugging/testing purposes so that it's possibile to emulate platforms with
+different number of supported colors.
+An important detail to highlight is that the current implementation of the
+cache coloring technique requires the number of colors to be a power of 2, and
+consequently, also the LLC way size must be so. A value that doesn't match this
+requirement is aligned down to the previous power of 2.
+
### lock-depth-size
> `= <integer>`
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..97eac24ee3 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
select 64BIT
select ARM_EFI
select HAS_FAST_MULTIPLY
+ select HAS_LLC_COLORING
config ARM
def_bool y
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 4d076b278b..7f5cb8ef26 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
obj-y += irq.o
obj-y += kernel.init.o
obj-$(CONFIG_LIVEPATCH) += livepatch.o
+obj-$(CONFIG_LLC_COLORING) += llc_coloring.o
obj-y += mem_access.o
obj-y += mm.o
obj-y += monitor.o
diff --git a/xen/arch/arm/include/asm/llc_coloring.h b/xen/arch/arm/include/asm/llc_coloring.h
new file mode 100644
index 0000000000..c7985c8fd0
--- /dev/null
+++ b/xen/arch/arm/include/asm/llc_coloring.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Last Level Cache (LLC) coloring support for ARM
+ *
+ * Copyright (C) 2022 Xilinx Inc.
+ *
+ * Authors:
+ * Luca Miccio <lucmiccio@gmail.com>
+ * Carlo Nonato <carlo.nonato@minervasys.tech>
+ */
+#ifndef __ASM_ARM_COLORING_H__
+#define __ASM_ARM_COLORING_H__
+
+#include <xen/init.h>
+
+#ifdef CONFIG_LLC_COLORING
+
+bool __init llc_coloring_init(void);
+
+#else /* !CONFIG_LLC_COLORING */
+
+static inline bool __init llc_coloring_init(void) { return true; }
+
+#endif /* CONFIG_LLC_COLORING */
+
+#endif /* __ASM_ARM_COLORING_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
\ No newline at end of file
diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 1dd81d7d52..1c5f6d6e7a 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -18,6 +18,22 @@
#define CTR_IDC_SHIFT 28
#define CTR_DIC_SHIFT 29
+/* CCSIDR Current Cache Size ID Register */
+#define CCSIDR_LINESIZE_MASK _AC(0x7, ULL)
+#define CCSIDR_NUMSETS_SHIFT 13
+#define CCSIDR_NUMSETS_MASK _AC(0x3fff, ULL)
+#define CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX 32
+#define CCSIDR_NUMSETS_MASK_FEAT_CCIDX _AC(0xffffff, ULL)
+
+/* CCSELR Cache Size Selection Register */
+#define CCSELR_LEVEL_MASK _AC(0x7, UL)
+#define CCSELR_LEVEL_SHIFT 1
+
+/* CLIDR Cache Level ID Register */
+#define CLIDR_CTYPEn_SHIFT(n) (3 * (n - 1))
+#define CLIDR_CTYPEn_MASK _AC(0x7, UL)
+#define CLIDR_CTYPEn_LEVELS 7
+
#define ICACHE_POLICY_VPIPT 0
#define ICACHE_POLICY_AIVIVT 1
#define ICACHE_POLICY_VIPT 2
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
new file mode 100644
index 0000000000..44b601915e
--- /dev/null
+++ b/xen/arch/arm/llc_coloring.c
@@ -0,0 +1,272 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Last Level Cache (LLC) coloring support for ARM
+ *
+ * Copyright (C) 2022 Xilinx Inc.
+ *
+ * Authors:
+ * Luca Miccio <lucmiccio@gmail.com>
+ * Carlo Nonato <carlo.nonato@minervasys.tech>
+ */
+#include <xen/bitops.h>
+#include <xen/errno.h>
+#include <xen/keyhandler.h>
+#include <xen/llc_coloring.h>
+#include <xen/param.h>
+#include <xen/types.h>
+
+#include <asm/processor.h>
+#include <asm/sysregs.h>
+
+bool llc_coloring_enabled;
+boolean_param("llc-coloring", llc_coloring_enabled);
+
+/* Size of an LLC way */
+static unsigned int __ro_after_init llc_way_size;
+size_param("llc-way-size", llc_way_size);
+/* Number of colors available in the LLC */
+static unsigned int __ro_after_init nr_colors = CONFIG_NR_LLC_COLORS;
+/* Mask to extract coloring relevant bits */
+static paddr_t __ro_after_init addr_col_mask;
+
+static unsigned int __ro_after_init dom0_colors[CONFIG_NR_LLC_COLORS];
+static unsigned int __ro_after_init dom0_num_colors;
+
+/*
+ * Parse the coloring configuration given in the buf string, following the
+ * syntax below.
+ *
+ * COLOR_CONFIGURATION ::= COLOR | RANGE,...,COLOR | RANGE
+ * RANGE ::= COLOR-COLOR
+ *
+ * Example: "0,2-6,15-16" represents the set of colors: 0,2,3,4,5,6,15,16.
+ */
+static int parse_color_config(const char *buf, unsigned int *colors,
+ unsigned int *num_colors)
+{
+ const char *s = buf;
+
+ if ( !colors || !num_colors )
+ return -EINVAL;
+
+ *num_colors = 0;
+
+ while ( *s != '\0' )
+ {
+ if ( *s != ',' )
+ {
+ unsigned int color, start, end;
+
+ start = simple_strtoul(s, &s, 0);
+
+ if ( *s == '-' ) /* Range */
+ {
+ s++;
+ end = simple_strtoul(s, &s, 0);
+ }
+ else /* Single value */
+ end = start;
+
+ if ( start > end || (end - start) > UINT_MAX - *num_colors ||
+ *num_colors + (end - start) >= nr_colors )
+ 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)
+{
+ register_t ccsidr_el1;
+ register_t clidr_el1 = READ_SYSREG(CLIDR_EL1);
+ register_t csselr_el1 = READ_SYSREG(CSSELR_EL1);
+ register_t id_aa64mmfr2_el1 = READ_SYSREG(ID_AA64MMFR2_EL1);
+ uint32_t ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT;
+ uint32_t ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK;
+ unsigned int n, line_size, num_sets;
+
+ for ( n = CLIDR_CTYPEn_LEVELS;
+ n != 0 && !((clidr_el1 >> CLIDR_CTYPEn_SHIFT(n)) & CLIDR_CTYPEn_MASK);
+ n-- );
+
+ if ( n == 0 )
+ return 0;
+
+ WRITE_SYSREG(((n - 1) & CCSELR_LEVEL_MASK) << CCSELR_LEVEL_SHIFT,
+ CSSELR_EL1);
+ isb();
+
+ ccsidr_el1 = READ_SYSREG(CCSIDR_EL1);
+
+ /* Arm ARM: (Log2(Number of bytes in cache line)) - 4 */
+ line_size = 1 << ((ccsidr_el1 & CCSIDR_LINESIZE_MASK) + 4);
+
+ /* If FEAT_CCIDX is enabled, CCSIDR_EL1 has a different bit layout */
+ if ( (id_aa64mmfr2_el1 >> ID_AA64MMFR2_CCIDX_SHIFT) & 0x7 )
+ {
+ ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX;
+ ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK_FEAT_CCIDX;
+ }
+ /* Arm ARM: (Number of sets in cache) - 1 */
+ num_sets = ((ccsidr_el1 >> ccsidr_numsets_shift) & ccsidr_numsets_mask) + 1;
+
+ printk(XENLOG_INFO "LLC found: L%u (line size: %u bytes, sets num: %u)\n",
+ n, line_size, num_sets);
+
+ /* Restore value in CSSELR_EL1 */
+ WRITE_SYSREG(csselr_el1, CSSELR_EL1);
+ isb();
+
+ return line_size * num_sets;
+}
+
+static bool check_colors(unsigned int *colors, unsigned int num_colors)
+{
+ unsigned int i;
+
+ if ( num_colors > nr_colors )
+ return false;
+
+ for ( i = 0; i < num_colors; i++ )
+ if ( colors[i] >= nr_colors ||
+ (i != num_colors - 1 && colors[i] >= colors[i + 1]) )
+ return false;
+
+ return true;
+}
+
+static void print_colors(unsigned int *colors, unsigned int num_colors)
+{
+ unsigned int i;
+
+ printk("[ ");
+ for ( i = 0; i < num_colors; i++ )
+ printk("%u ", colors[i]);
+ printk("]\n");
+}
+
+static void dump_coloring_info(unsigned char key)
+{
+ printk("'%c' pressed -> dumping LLC coloring general info\n", key);
+ printk("LLC way size: %u KiB\n", llc_way_size >> 10);
+ printk("Number of LLC colors supported: %u\n", nr_colors);
+ printk("Address to LLC color mask: 0x%lx\n", addr_col_mask);
+}
+
+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 )
+ {
+ printk(XENLOG_ERR "Number of LLC colors (%u) not in range [2, %u]\n",
+ nr_colors, CONFIG_NR_LLC_COLORS);
+ return false;
+ }
+
+ addr_col_mask = (nr_colors - 1) << PAGE_SHIFT;
+
+ register_keyhandler('K', dump_coloring_info, "dump LLC coloring info", 1);
+
+ return true;
+}
+
+static unsigned int *alloc_colors(unsigned int num_colors)
+{
+ unsigned int *colors = xmalloc_array(unsigned int, num_colors);
+
+ if ( !colors )
+ panic("Unable to allocate LLC colors\n");
+
+ return colors;
+}
+
+int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
+ unsigned int num_colors)
+{
+ unsigned int i;
+
+ if ( is_domain_direct_mapped(d) )
+ {
+ printk(XENLOG_ERR
+ "LLC coloring and direct mapping are incompatible (%pd)\n", d);
+ return -EINVAL;
+ }
+
+ if ( !colors || num_colors == 0 )
+ {
+ printk(XENLOG_WARNING
+ "LLC color config not found for %pd. Using default\n", d);
+ colors = alloc_colors(nr_colors);
+ num_colors = nr_colors;
+ for ( i = 0; i < nr_colors; i++ )
+ colors[i] = i;
+ }
+
+ d->llc_colors = colors;
+ d->num_llc_colors = num_colors;
+
+ if ( !check_colors(d->llc_colors, d->num_llc_colors) )
+ {
+ /* d->llc_colors will be freed in domain_destroy() */
+ printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
+ print_colors(d->llc_colors, d->num_llc_colors);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+void domain_llc_coloring_free(struct domain *d)
+{
+ xfree(d->llc_colors);
+}
+
+void domain_dump_llc_colors(struct domain *d)
+{
+ printk("Domain %pd has %u LLC colors: ", d, d->num_llc_colors);
+ print_colors(d->llc_colors, d->num_llc_colors);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90..c04e5012f0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -12,6 +12,7 @@
#include <xen/device_tree.h>
#include <xen/domain_page.h>
#include <xen/grant_table.h>
+#include <xen/llc_coloring.h>
#include <xen/types.h>
#include <xen/string.h>
#include <xen/serial.h>
@@ -1026,6 +1027,12 @@ void __init start_xen(unsigned long boot_phys_offset,
printk("Command line: %s\n", cmdline);
cmdline_parse(cmdline);
+ if ( llc_coloring_enabled )
+ {
+ if ( !llc_coloring_init() )
+ panic("Xen LLC coloring support: setup failed\n");
+ }
+
setup_mm();
/* Parse the ACPI tables for possible boot-time configuration */
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 02/11] xen/arm: add cache coloring initialization
2023-01-23 15:47 ` [PATCH v4 02/11] xen/arm: add cache coloring initialization Carlo Nonato
@ 2023-01-24 16:20 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2023-01-24 16:20 UTC (permalink / raw)
To: Carlo Nonato
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> +static unsigned int *alloc_colors(unsigned int num_colors)
> +{
> + unsigned int *colors = xmalloc_array(unsigned int, num_colors);
> +
> + if ( !colors )
> + panic("Unable to allocate LLC colors\n");
Already for Dom0 creation I view this as an unacceptable form of error
"handling". Later you even hook this up to a domctl for DomU creation,
at which point panic()ing is entirely unacceptable.
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 01/11] xen/common: add cache coloring common code Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 02/11] xen/arm: add cache coloring initialization Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2024-01-12 9:24 ` Michal Orzel
2023-01-23 15:47 ` [PATCH v4 04/11] xen: extend domctl interface for cache coloring Carlo Nonato
` (8 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri, Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
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.
Here is also introduced a common configuration syntax for cache colors.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
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 | 32 ++++++++++++++++++++++---
xen/arch/arm/domain_build.c | 17 +++++++++++--
xen/arch/arm/include/asm/llc_coloring.h | 4 ++++
xen/arch/arm/llc_coloring.c | 14 +++++++++++
4 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index 0244d2f606..c2e0e87426 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -83,12 +83,38 @@ manually set the way size it's left for the user to overcome failing situations
or for debugging/testing purposes. See `Coloring parameters and domain
configurations`_ section for more information on that.
+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] |
++---------------------+-----------------------------------+
+
Coloring parameters and domain configurations
*********************************************
-LLC way size (as previously discussed) can be set using the appropriate command
-line parameter. See the relevant documentation in
-"docs/misc/xen-command-line.pandoc".
+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".
**Note:** If no color configuration is provided for a domain, the default one,
which corresponds to all available colors, is used instead.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f35f4d2456..093d4ad6f6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2,6 +2,7 @@
#include <xen/init.h>
#include <xen/compile.h>
#include <xen/lib.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/param.h>
#include <xen/domain_page.h>
@@ -4014,7 +4015,10 @@ static int __init construct_dom0(struct domain *d)
/* type must be set before allocate_memory */
d->arch.type = kinfo.type;
#endif
- allocate_memory_11(d, &kinfo);
+ if ( is_domain_llc_colored(d) )
+ allocate_memory(d, &kinfo);
+ else
+ allocate_memory_11(d, &kinfo);
find_gnttab_region(d, &kinfo);
#ifdef CONFIG_STATIC_SHM
@@ -4060,6 +4064,8 @@ void __init create_dom0(void)
.max_maptrack_frames = -1,
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
};
+ unsigned int *llc_colors = NULL;
+ unsigned int num_llc_colors = 0, flags = CDF_privileged;
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
@@ -4076,7 +4082,14 @@ void __init create_dom0(void)
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
- dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
+ if ( llc_coloring_enabled )
+ llc_colors = dom0_llc_colors(&num_llc_colors);
+ else
+ flags |= CDF_directmap;
+
+ dom0 = domain_create_llc_colored(0, &dom0_cfg, flags, llc_colors,
+ num_llc_colors);
+
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
panic("Error creating domain 0\n");
diff --git a/xen/arch/arm/include/asm/llc_coloring.h b/xen/arch/arm/include/asm/llc_coloring.h
index c7985c8fd0..382ff7de47 100644
--- a/xen/arch/arm/include/asm/llc_coloring.h
+++ b/xen/arch/arm/include/asm/llc_coloring.h
@@ -17,9 +17,13 @@
bool __init llc_coloring_init(void);
+unsigned int *dom0_llc_colors(unsigned int *num_colors);
+
#else /* !CONFIG_LLC_COLORING */
static inline bool __init llc_coloring_init(void) { return true; }
+static inline unsigned int *dom0_llc_colors(
+ unsigned int *num_colors) { return NULL; }
#endif /* CONFIG_LLC_COLORING */
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 44b601915e..51f057d7c9 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -261,6 +261,20 @@ void domain_dump_llc_colors(struct domain *d)
print_colors(d->llc_colors, d->num_llc_colors);
}
+unsigned int *dom0_llc_colors(unsigned int *num_colors)
+{
+ unsigned int *colors;
+
+ if ( !dom0_num_colors )
+ return NULL;
+
+ colors = alloc_colors(dom0_num_colors);
+ memcpy(colors, dom0_colors, sizeof(unsigned int) * dom0_num_colors);
+ *num_colors = dom0_num_colors;
+
+ return colors;
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support
2023-01-23 15:47 ` [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support Carlo Nonato
@ 2024-01-12 9:24 ` Michal Orzel
2024-01-12 10:39 ` Michal Orzel
2024-01-18 0:23 ` Stefano Stabellini
0 siblings, 2 replies; 43+ messages in thread
From: Michal Orzel @ 2024-01-12 9:24 UTC (permalink / raw)
To: Carlo Nonato, xen-devel
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
Hi Carlo,
On 23/01/2023 16:47, Carlo Nonato wrote:
>
>
> From: Luca Miccio <lucmiccio@gmail.com>
>
> 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.
>
> Here is also introduced a common configuration syntax for cache colors.
>
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> ---
> 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 | 32 ++++++++++++++++++++++---
> xen/arch/arm/domain_build.c | 17 +++++++++++--
> xen/arch/arm/include/asm/llc_coloring.h | 4 ++++
> xen/arch/arm/llc_coloring.c | 14 +++++++++++
> 4 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
> index 0244d2f606..c2e0e87426 100644
> --- a/docs/misc/arm/cache-coloring.rst
> +++ b/docs/misc/arm/cache-coloring.rst
> @@ -83,12 +83,38 @@ manually set the way size it's left for the user to overcome failing situations
> or for debugging/testing purposes. See `Coloring parameters and domain
> configurations`_ section for more information on that.
>
> +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] |
> ++---------------------+-----------------------------------+
> +
> Coloring parameters and domain configurations
> *********************************************
>
> -LLC way size (as previously discussed) can be set using the appropriate command
> -line parameter. See the relevant documentation in
> -"docs/misc/xen-command-line.pandoc".
> +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".
>
> **Note:** If no color configuration is provided for a domain, the default one,
> which corresponds to all available colors, is used instead.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f35f4d2456..093d4ad6f6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2,6 +2,7 @@
> #include <xen/init.h>
> #include <xen/compile.h>
> #include <xen/lib.h>
> +#include <xen/llc_coloring.h>
> #include <xen/mm.h>
> #include <xen/param.h>
> #include <xen/domain_page.h>
> @@ -4014,7 +4015,10 @@ static int __init construct_dom0(struct domain *d)
> /* type must be set before allocate_memory */
> d->arch.type = kinfo.type;
> #endif
> - allocate_memory_11(d, &kinfo);
> + if ( is_domain_llc_colored(d) )
> + allocate_memory(d, &kinfo);
While doing some checks, I realized that the issue from previous series is still present.
Given that dom0 is hwdom, how are you going to prevent conflicts between allocated RAM and HW resources
that are to be mapped to dom0?
~Michal
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support
2024-01-12 9:24 ` Michal Orzel
@ 2024-01-12 10:39 ` Michal Orzel
2024-01-18 0:23 ` Stefano Stabellini
1 sibling, 0 replies; 43+ messages in thread
From: Michal Orzel @ 2024-01-12 10:39 UTC (permalink / raw)
To: Carlo Nonato, xen-devel
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
On 12/01/2024 10:24, Michal Orzel wrote:
>
>
> Hi Carlo,
>
> On 23/01/2023 16:47, Carlo Nonato wrote:
>>
>>
>> From: Luca Miccio <lucmiccio@gmail.com>
>>
>> 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.
>>
>> Here is also introduced a common configuration syntax for cache colors.
>>
>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
>> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
>> ---
>> 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 | 32 ++++++++++++++++++++++---
>> xen/arch/arm/domain_build.c | 17 +++++++++++--
>> xen/arch/arm/include/asm/llc_coloring.h | 4 ++++
>> xen/arch/arm/llc_coloring.c | 14 +++++++++++
>> 4 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
>> index 0244d2f606..c2e0e87426 100644
>> --- a/docs/misc/arm/cache-coloring.rst
>> +++ b/docs/misc/arm/cache-coloring.rst
>> @@ -83,12 +83,38 @@ manually set the way size it's left for the user to overcome failing situations
>> or for debugging/testing purposes. See `Coloring parameters and domain
>> configurations`_ section for more information on that.
>>
>> +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] |
>> ++---------------------+-----------------------------------+
>> +
>> Coloring parameters and domain configurations
>> *********************************************
>>
>> -LLC way size (as previously discussed) can be set using the appropriate command
>> -line parameter. See the relevant documentation in
>> -"docs/misc/xen-command-line.pandoc".
>> +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".
>>
>> **Note:** If no color configuration is provided for a domain, the default one,
>> which corresponds to all available colors, is used instead.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f35f4d2456..093d4ad6f6 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2,6 +2,7 @@
>> #include <xen/init.h>
>> #include <xen/compile.h>
>> #include <xen/lib.h>
>> +#include <xen/llc_coloring.h>
>> #include <xen/mm.h>
>> #include <xen/param.h>
>> #include <xen/domain_page.h>
>> @@ -4014,7 +4015,10 @@ static int __init construct_dom0(struct domain *d)
>> /* type must be set before allocate_memory */
>> d->arch.type = kinfo.type;
>> #endif
>> - allocate_memory_11(d, &kinfo);
>> + if ( is_domain_llc_colored(d) )
>> + allocate_memory(d, &kinfo);
> While doing some checks, I realized that the issue from previous series is still present.
> Given that dom0 is hwdom, how are you going to prevent conflicts between allocated RAM and HW resources
> that are to be mapped to dom0?
Sorry. I answered to the wrong revision :)
Anyway, the remark still applies to v5.
~Michal
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support
2024-01-12 9:24 ` Michal Orzel
2024-01-12 10:39 ` Michal Orzel
@ 2024-01-18 0:23 ` Stefano Stabellini
2024-01-18 7:40 ` Michal Orzel
1 sibling, 1 reply; 43+ messages in thread
From: Stefano Stabellini @ 2024-01-18 0:23 UTC (permalink / raw)
To: Michal Orzel
Cc: Carlo Nonato, xen-devel, Luca Miccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Marco Solieri
On Fri, 12 Jan 2024, Michal Orzel wrote:
> Hi Carlo,
>
> On 23/01/2023 16:47, Carlo Nonato wrote:
> >
> >
> > From: Luca Miccio <lucmiccio@gmail.com>
> >
> > 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.
> >
> > Here is also introduced a common configuration syntax for cache colors.
> >
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> > ---
> > 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 | 32 ++++++++++++++++++++++---
> > xen/arch/arm/domain_build.c | 17 +++++++++++--
> > xen/arch/arm/include/asm/llc_coloring.h | 4 ++++
> > xen/arch/arm/llc_coloring.c | 14 +++++++++++
> > 4 files changed, 62 insertions(+), 5 deletions(-)
> >
> > diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
> > index 0244d2f606..c2e0e87426 100644
> > --- a/docs/misc/arm/cache-coloring.rst
> > +++ b/docs/misc/arm/cache-coloring.rst
> > @@ -83,12 +83,38 @@ manually set the way size it's left for the user to overcome failing situations
> > or for debugging/testing purposes. See `Coloring parameters and domain
> > configurations`_ section for more information on that.
> >
> > +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] |
> > ++---------------------+-----------------------------------+
> > +
> > Coloring parameters and domain configurations
> > *********************************************
> >
> > -LLC way size (as previously discussed) can be set using the appropriate command
> > -line parameter. See the relevant documentation in
> > -"docs/misc/xen-command-line.pandoc".
> > +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".
> >
> > **Note:** If no color configuration is provided for a domain, the default one,
> > which corresponds to all available colors, is used instead.
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index f35f4d2456..093d4ad6f6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2,6 +2,7 @@
> > #include <xen/init.h>
> > #include <xen/compile.h>
> > #include <xen/lib.h>
> > +#include <xen/llc_coloring.h>
> > #include <xen/mm.h>
> > #include <xen/param.h>
> > #include <xen/domain_page.h>
> > @@ -4014,7 +4015,10 @@ static int __init construct_dom0(struct domain *d)
> > /* type must be set before allocate_memory */
> > d->arch.type = kinfo.type;
> > #endif
> > - allocate_memory_11(d, &kinfo);
> > + if ( is_domain_llc_colored(d) )
> > + allocate_memory(d, &kinfo);
> While doing some checks, I realized that the issue from previous series is still present.
> Given that dom0 is hwdom, how are you going to prevent conflicts between allocated RAM and HW resources
> that are to be mapped to dom0?
Are you referring to the address ranges picked for RAM region and how to
make sure they don't conflict with something else (e.g. the MMIO region
of a device)?
I thought that for dom0 we were reusing the same address layout of the
host, so device MMIO would be mapped 1:1, memory would not be mapped 1:1
(due to cache coloring) but it would be mapped to the same guest address
ranges corresponding to RAM addresses on the host. Is it not the case in
this version of the patch series?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support
2024-01-18 0:23 ` Stefano Stabellini
@ 2024-01-18 7:40 ` Michal Orzel
0 siblings, 0 replies; 43+ messages in thread
From: Michal Orzel @ 2024-01-18 7:40 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Carlo Nonato, xen-devel, Luca Miccio, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Marco Solieri
On 18/01/2024 01:23, Stefano Stabellini wrote:
>
>
> On Fri, 12 Jan 2024, Michal Orzel wrote:
>> Hi Carlo,
>>
>> On 23/01/2023 16:47, Carlo Nonato wrote:
>>>
>>>
>>> From: Luca Miccio <lucmiccio@gmail.com>
>>>
>>> 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.
>>>
>>> Here is also introduced a common configuration syntax for cache colors.
>>>
>>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
>>> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
>>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
>>> ---
>>> 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 | 32 ++++++++++++++++++++++---
>>> xen/arch/arm/domain_build.c | 17 +++++++++++--
>>> xen/arch/arm/include/asm/llc_coloring.h | 4 ++++
>>> xen/arch/arm/llc_coloring.c | 14 +++++++++++
>>> 4 files changed, 62 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
>>> index 0244d2f606..c2e0e87426 100644
>>> --- a/docs/misc/arm/cache-coloring.rst
>>> +++ b/docs/misc/arm/cache-coloring.rst
>>> @@ -83,12 +83,38 @@ manually set the way size it's left for the user to overcome failing situations
>>> or for debugging/testing purposes. See `Coloring parameters and domain
>>> configurations`_ section for more information on that.
>>>
>>> +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] |
>>> ++---------------------+-----------------------------------+
>>> +
>>> Coloring parameters and domain configurations
>>> *********************************************
>>>
>>> -LLC way size (as previously discussed) can be set using the appropriate command
>>> -line parameter. See the relevant documentation in
>>> -"docs/misc/xen-command-line.pandoc".
>>> +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".
>>>
>>> **Note:** If no color configuration is provided for a domain, the default one,
>>> which corresponds to all available colors, is used instead.
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index f35f4d2456..093d4ad6f6 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2,6 +2,7 @@
>>> #include <xen/init.h>
>>> #include <xen/compile.h>
>>> #include <xen/lib.h>
>>> +#include <xen/llc_coloring.h>
>>> #include <xen/mm.h>
>>> #include <xen/param.h>
>>> #include <xen/domain_page.h>
>>> @@ -4014,7 +4015,10 @@ static int __init construct_dom0(struct domain *d)
>>> /* type must be set before allocate_memory */
>>> d->arch.type = kinfo.type;
>>> #endif
>>> - allocate_memory_11(d, &kinfo);
>>> + if ( is_domain_llc_colored(d) )
>>> + allocate_memory(d, &kinfo);
>> While doing some checks, I realized that the issue from previous series is still present.
>> Given that dom0 is hwdom, how are you going to prevent conflicts between allocated RAM and HW resources
>> that are to be mapped to dom0?
>
> Are you referring to the address ranges picked for RAM region and how to
> make sure they don't conflict with something else (e.g. the MMIO region
> of a device)?
>
> I thought that for dom0 we were reusing the same address layout of the
> host, so device MMIO would be mapped 1:1, memory would not be mapped 1:1
> (due to cache coloring) but it would be mapped to the same guest address
> ranges corresponding to RAM addresses on the host. Is it not the case in
> this version of the patch series?
It is not the case (see [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support).
Hwdom would get assigned the normal Xen guest memory map. Instead we should map memory
to host RAM to prevent conflicts with MMIO and reserved memory (e.g. calling find_unallocated_memory
in hwdom case to retrieve host RAM excluding rsvm).
~Michal
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (2 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 03/11] xen/arm: add Dom0 cache coloring support Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-24 16:29 ` Jan Beulich
2023-01-23 15:47 ` [PATCH v4 05/11] tools: add support for cache coloring configuration Carlo Nonato
` (7 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Marco Solieri
This commit updates the domctl interface to allow the user to set cache
coloring configurations from the toolstack.
It also implements the functionality for arm64.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- updated XEN_DOMCTL_INTERFACE_VERSION
---
xen/arch/arm/llc_coloring.c | 14 ++++++++++++++
xen/common/domctl.c | 12 +++++++++++-
xen/include/public/domctl.h | 6 +++++-
xen/include/xen/llc_coloring.h | 4 ++++
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 51f057d7c9..2d0457cdbc 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -10,6 +10,7 @@
*/
#include <xen/bitops.h>
#include <xen/errno.h>
+#include <xen/guest_access.h>
#include <xen/keyhandler.h>
#include <xen/llc_coloring.h>
#include <xen/param.h>
@@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
return colors;
}
+unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
+{
+ unsigned int *colors;
+
+ if ( !config->num_llc_colors )
+ return NULL;
+
+ colors = alloc_colors(config->num_llc_colors);
+ copy_from_guest(colors, config->llc_colors, config->num_llc_colors);
+
+ return colors;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ad71ad8a4c..505626ec46 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -8,6 +8,7 @@
#include <xen/types.h>
#include <xen/lib.h>
+#include <xen/llc_coloring.h>
#include <xen/err.h>
#include <xen/mm.h>
#include <xen/sched.h>
@@ -409,6 +410,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
domid_t dom;
static domid_t rover = 0;
+ unsigned int *llc_colors = NULL, num_llc_colors = 0;
dom = op->domain;
if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
@@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
rover = dom;
}
- d = domain_create(dom, &op->u.createdomain, false);
+ if ( llc_coloring_enabled )
+ {
+ llc_colors = llc_colors_from_guest(&op->u.createdomain);
+ num_llc_colors = op->u.createdomain.num_llc_colors;
+ }
+
+ d = domain_create_llc_colored(dom, &op->u.createdomain, false,
+ llc_colors, num_llc_colors);
+
if ( IS_ERR(d) )
{
ret = PTR_ERR(d);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 51be28c3de..49cccc8503 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
/* CPU pool to use; specify 0 or a specific existing pool */
uint32_t cpupool_id;
+ /* IN LLC coloring parameters */
+ uint32_t num_llc_colors;
+ XEN_GUEST_HANDLE(uint32) llc_colors;
+
struct xen_arch_domainconfig arch;
};
diff --git a/xen/include/xen/llc_coloring.h b/xen/include/xen/llc_coloring.h
index 625930d378..2855f38296 100644
--- a/xen/include/xen/llc_coloring.h
+++ b/xen/include/xen/llc_coloring.h
@@ -24,6 +24,8 @@ int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
void domain_llc_coloring_free(struct domain *d);
void domain_dump_llc_colors(struct domain *d);
+unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config);
+
#else
#define llc_coloring_enabled (false)
@@ -36,6 +38,8 @@ static inline int domain_llc_coloring_init(struct domain *d,
}
static inline void domain_llc_coloring_free(struct domain *d) {}
static inline void domain_dump_llc_colors(struct domain *d) {}
+static inline unsigned int *llc_colors_from_guest(
+ struct xen_domctl_createdomain *config) { return NULL; }
#endif /* CONFIG_HAS_LLC_COLORING */
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-23 15:47 ` [PATCH v4 04/11] xen: extend domctl interface for cache coloring Carlo Nonato
@ 2023-01-24 16:29 ` Jan Beulich
2023-01-25 16:27 ` Carlo Nonato
2023-01-26 7:25 ` Jan Beulich
0 siblings, 2 replies; 43+ messages in thread
From: Jan Beulich @ 2023-01-24 16:29 UTC (permalink / raw)
To: Carlo Nonato
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
> return colors;
> }
>
> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
const struct ...?
> +{
> + unsigned int *colors;
> +
> + if ( !config->num_llc_colors )
> + return NULL;
> +
> + colors = alloc_colors(config->num_llc_colors);
Error handling needs to occur here; the panic() in alloc_colors() needs
to go away.
> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> rover = dom;
> }
>
> - d = domain_create(dom, &op->u.createdomain, false);
> + if ( llc_coloring_enabled )
> + {
> + llc_colors = llc_colors_from_guest(&op->u.createdomain);
> + num_llc_colors = op->u.createdomain.num_llc_colors;
I think you would better avoid setting num_llc_colors to non-zero if
you got back NULL from the function. It's at best confusing.
> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> /* CPU pool to use; specify 0 or a specific existing pool */
> uint32_t cpupool_id;
>
> + /* IN LLC coloring parameters */
> + uint32_t num_llc_colors;
> + XEN_GUEST_HANDLE(uint32) llc_colors;
Despite your earlier replies I continue to be unconvinced that this
is information which needs to be available right at domain_create.
Without that you'd also get away without the sufficiently odd
domain_create_llc_colored(). (Odd because: Think of two or three
more extended features appearing, all of which want a special cased
domain_create().)
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-24 16:29 ` Jan Beulich
@ 2023-01-25 16:27 ` Carlo Nonato
2023-01-26 10:20 ` Julien Grall
2023-01-26 7:25 ` Jan Beulich
1 sibling, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-25 16:27 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
Hi Jan,
On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
> > return colors;
> > }
> >
> > +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
>
> const struct ...?
>
> > +{
> > + unsigned int *colors;
> > +
> > + if ( !config->num_llc_colors )
> > + return NULL;
> > +
> > + colors = alloc_colors(config->num_llc_colors);
>
> Error handling needs to occur here; the panic() in alloc_colors() needs
> to go away.
>
> > @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > rover = dom;
> > }
> >
> > - d = domain_create(dom, &op->u.createdomain, false);
> > + if ( llc_coloring_enabled )
> > + {
> > + llc_colors = llc_colors_from_guest(&op->u.createdomain);
> > + num_llc_colors = op->u.createdomain.num_llc_colors;
>
> I think you would better avoid setting num_llc_colors to non-zero if
> you got back NULL from the function. It's at best confusing.
>
> > @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> > /* CPU pool to use; specify 0 or a specific existing pool */
> > uint32_t cpupool_id;
> >
> > + /* IN LLC coloring parameters */
> > + uint32_t num_llc_colors;
> > + XEN_GUEST_HANDLE(uint32) llc_colors;
>
> Despite your earlier replies I continue to be unconvinced that this
> is information which needs to be available right at domain_create.
> Without that you'd also get away without the sufficiently odd
> domain_create_llc_colored(). (Odd because: Think of two or three
> more extended features appearing, all of which want a special cased
> domain_create().)
Yes, I definitely see your point. Still there is the p2m table allocation
problem that you and Julien have discussed previously. I'm not sure I
understood what the approach is.
> Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-25 16:27 ` Carlo Nonato
@ 2023-01-26 10:20 ` Julien Grall
2023-01-26 11:19 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2023-01-26 10:20 UTC (permalink / raw)
To: Carlo Nonato, Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Marco Solieri, xen-devel
Hi,
On 25/01/2023 16:27, Carlo Nonato wrote:
> On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
>>> return colors;
>>> }
>>>
>>> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
>>
>> const struct ...?
>>
>>> +{
>>> + unsigned int *colors;
>>> +
>>> + if ( !config->num_llc_colors )
>>> + return NULL;
>>> +
>>> + colors = alloc_colors(config->num_llc_colors);
>>
>> Error handling needs to occur here; the panic() in alloc_colors() needs
>> to go away.
>>
>>> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> rover = dom;
>>> }
>>>
>>> - d = domain_create(dom, &op->u.createdomain, false);
>>> + if ( llc_coloring_enabled )
>>> + {
>>> + llc_colors = llc_colors_from_guest(&op->u.createdomain);
>>> + num_llc_colors = op->u.createdomain.num_llc_colors;
>>
>> I think you would better avoid setting num_llc_colors to non-zero if
>> you got back NULL from the function. It's at best confusing.
>>
>>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
>>> /* CPU pool to use; specify 0 or a specific existing pool */
>>> uint32_t cpupool_id;
>>>
>>> + /* IN LLC coloring parameters */
>>> + uint32_t num_llc_colors;
>>> + XEN_GUEST_HANDLE(uint32) llc_colors;
>>
>> Despite your earlier replies I continue to be unconvinced that this
>> is information which needs to be available right at domain_create.
>> Without that you'd also get away without the sufficiently odd
>> domain_create_llc_colored(). (Odd because: Think of two or three
>> more extended features appearing, all of which want a special cased
>> domain_create().)
>
> Yes, I definitely see your point. Still there is the p2m table allocation
> problem that you and Julien have discussed previously. I'm not sure I
> understood what the approach is.
Henry has sent a series [1] to remove the requirement to allocate the
P2M in domain_create().
With that series applied, there requirements to pass the colors at
domain creation should be lifted.
Cheers,
[1]
https://lore.kernel.org/xen-devel/20230116015820.1269387-1-Henry.Wang@arm.com/
>
>> Jan
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-26 10:20 ` Julien Grall
@ 2023-01-26 11:19 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 11:19 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, Marco Solieri, xen-devel
Hi Julien and Jan,
On Thu, Jan 26, 2023 at 11:21 AM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 25/01/2023 16:27, Carlo Nonato wrote:
> > On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 23.01.2023 16:47, Carlo Nonato wrote:
> >>> @@ -275,6 +276,19 @@ unsigned int *dom0_llc_colors(unsigned int *num_colors)
> >>> return colors;
> >>> }
> >>>
> >>> +unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
> >>
> >> const struct ...?
> >>
> >>> +{
> >>> + unsigned int *colors;
> >>> +
> >>> + if ( !config->num_llc_colors )
> >>> + return NULL;
> >>> +
> >>> + colors = alloc_colors(config->num_llc_colors);
> >>
> >> Error handling needs to occur here; the panic() in alloc_colors() needs
> >> to go away.
> >>
> >>> @@ -434,7 +436,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>> rover = dom;
> >>> }
> >>>
> >>> - d = domain_create(dom, &op->u.createdomain, false);
> >>> + if ( llc_coloring_enabled )
> >>> + {
> >>> + llc_colors = llc_colors_from_guest(&op->u.createdomain);
> >>> + num_llc_colors = op->u.createdomain.num_llc_colors;
> >>
> >> I think you would better avoid setting num_llc_colors to non-zero if
> >> you got back NULL from the function. It's at best confusing.
> >>
> >>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> >>> /* CPU pool to use; specify 0 or a specific existing pool */
> >>> uint32_t cpupool_id;
> >>>
> >>> + /* IN LLC coloring parameters */
> >>> + uint32_t num_llc_colors;
> >>> + XEN_GUEST_HANDLE(uint32) llc_colors;
> >>
> >> Despite your earlier replies I continue to be unconvinced that this
> >> is information which needs to be available right at domain_create.
> >> Without that you'd also get away without the sufficiently odd
> >> domain_create_llc_colored(). (Odd because: Think of two or three
> >> more extended features appearing, all of which want a special cased
> >> domain_create().)
> >
> > Yes, I definitely see your point. Still there is the p2m table allocation
> > problem that you and Julien have discussed previously. I'm not sure I
> > understood what the approach is.
>
> Henry has sent a series [1] to remove the requirement to allocate the
> P2M in domain_create().
>
> With that series applied, there requirements to pass the colors at
> domain creation should be lifted.
>
> Cheers,
>
> [1]
> https://lore.kernel.org/xen-devel/20230116015820.1269387-1-Henry.Wang@arm.com/
Really nice. Thanks to both.
> >
> >> Jan
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-24 16:29 ` Jan Beulich
2023-01-25 16:27 ` Carlo Nonato
@ 2023-01-26 7:25 ` Jan Beulich
2023-01-26 11:18 ` Carlo Nonato
1 sibling, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-26 7:25 UTC (permalink / raw)
To: Carlo Nonato
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 24.01.2023 17:29, Jan Beulich wrote:
> On 23.01.2023 16:47, Carlo Nonato wrote:
>> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
>> /* CPU pool to use; specify 0 or a specific existing pool */
>> uint32_t cpupool_id;
>>
>> + /* IN LLC coloring parameters */
>> + uint32_t num_llc_colors;
>> + XEN_GUEST_HANDLE(uint32) llc_colors;
>
> Despite your earlier replies I continue to be unconvinced that this
> is information which needs to be available right at domain_create.
> Without that you'd also get away without the sufficiently odd
> domain_create_llc_colored(). (Odd because: Think of two or three
> more extended features appearing, all of which want a special cased
> domain_create().)
And perhaps the real question is: Why do the two items need passing
to a special variant of domain_create() in the first place? The
necessary information already is passed to the normal function via
struct xen_domctl_createdomain. All it would take is to read the
array from guest space later, when struct domain was already
allocated and is hence available for storing the pointer. (Passing
the count separately is redundant in any event.)
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring
2023-01-26 7:25 ` Jan Beulich
@ 2023-01-26 11:18 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 11:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
Hi Jan,
On Thu, Jan 26, 2023 at 8:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.01.2023 17:29, Jan Beulich wrote:
> > On 23.01.2023 16:47, Carlo Nonato wrote:
> >> @@ -92,6 +92,10 @@ struct xen_domctl_createdomain {
> >> /* CPU pool to use; specify 0 or a specific existing pool */
> >> uint32_t cpupool_id;
> >>
> >> + /* IN LLC coloring parameters */
> >> + uint32_t num_llc_colors;
> >> + XEN_GUEST_HANDLE(uint32) llc_colors;
> >
> > Despite your earlier replies I continue to be unconvinced that this
> > is information which needs to be available right at domain_create.
> > Without that you'd also get away without the sufficiently odd
> > domain_create_llc_colored(). (Odd because: Think of two or three
> > more extended features appearing, all of which want a special cased
> > domain_create().)
>
> And perhaps the real question is: Why do the two items need passing
> to a special variant of domain_create() in the first place? The
> necessary information already is passed to the normal function via
> struct xen_domctl_createdomain. All it would take is to read the
> array from guest space later, when struct domain was already
> allocated and is hence available for storing the pointer. (Passing
> the count separately is redundant in any event.)
This was our first approach. However, struct xen_domctl_createdomain is used
both by domctl (pointing to guest memory) and by Xen itself (using Xen memory)
and Julien wasn't happy with this approach because it required some
kind of hack.
See this message from him:
https://marc.info/?l=xen-devel&m=166637496520053
and my answer:
https://marc.info/?l=xen-devel&m=166782830201561
> Jan
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 05/11] tools: add support for cache coloring configuration
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (3 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 04/11] xen: extend domctl interface for cache coloring Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-26 14:22 ` Anthony PERARD
2023-01-23 15:47 ` [PATCH v4 06/11] xen/arm: add support for cache coloring configuration via device-tree Carlo Nonato
` (6 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Wei Liu, Anthony PERARD, Juergen Gross, Marco Solieri
Add a new "llc_colors" parameter that defines the LLC color assignment for
a domain. The user can specify one or more color ranges using the same
syntax used everywhere else for color config described in the
documentation.
The parameter is defined as a list of strings that represent the color
ranges.
Documentation is also added.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- removed overlapping color ranges checks during parsing
- moved hypercall buffer initialization in libxenctrl
---
docs/man/xl.cfg.5.pod.in | 10 +++++++++
tools/libs/ctrl/xc_domain.c | 17 ++++++++++++++
tools/libs/light/libxl_create.c | 2 ++
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 38 +++++++++++++++++++++++++++++++-
5 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 024bceeb61..96f9249c3d 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2903,6 +2903,16 @@ Currently, only the "sbsa_uart" model is supported for ARM.
=back
+=over 4
+
+=item B<llc_colors=[ "RANGE", "RANGE", ...]>
+
+Specify the Last Level Cache (LLC) color configuration for the guest.
+B<RANGE> can be either a single color value or a hypen-separated closed
+interval of colors (such as "0-4").
+
+=back
+
=head3 x86
=over 4
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index e939d07157..064f54c349 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -28,6 +28,20 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
{
int err;
DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BUFFER(uint32_t, llc_colors);
+
+ if ( config->num_llc_colors )
+ {
+ size_t bytes = sizeof(uint32_t) * config->num_llc_colors;
+
+ llc_colors = xc_hypercall_buffer_alloc(xch, llc_colors, bytes);
+ if ( llc_colors == NULL ) {
+ PERROR("Could not allocate LLC colors for xc_domain_create");
+ return -ENOMEM;
+ }
+ memcpy(llc_colors, config->llc_colors.p, bytes);
+ set_xen_guest_handle(config->llc_colors, llc_colors);
+ }
domctl.cmd = XEN_DOMCTL_createdomain;
domctl.domain = *pdomid;
@@ -39,6 +53,9 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
*pdomid = (uint16_t)domctl.domain;
*config = domctl.u.createdomain;
+ if ( llc_colors )
+ xc_hypercall_buffer_free(xch, llc_colors);
+
return 0;
}
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index beec3f6b6f..6d0c768241 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -638,6 +638,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
.grant_opts = XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
.vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
.cpupool_id = info->poolid,
+ .num_llc_colors = b_info->num_llc_colors,
+ .llc_colors.p = b_info->llc_colors,
};
if (info->type != LIBXL_DOMAIN_TYPE_PV) {
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 0cfad8508d..1f944ca6d7 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -562,6 +562,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("ioports", Array(libxl_ioport_range, "num_ioports")),
("irqs", Array(uint32, "num_irqs")),
("iomem", Array(libxl_iomem_range, "num_iomem")),
+ ("llc_colors", Array(uint32, "num_llc_colors")),
("claim_mode", libxl_defbool),
("event_channels", uint32),
("kernel", string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 853e9f357a..0f8c469fb5 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1297,8 +1297,9 @@ void parse_config_data(const char *config_source,
XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
*usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
- *mca_caps;
+ *mca_caps, *llc_colors;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
+ int num_llc_colors;
int pci_power_mgmt = 0;
int pci_msitranslate = 0;
int pci_permissive = 0;
@@ -1447,6 +1448,41 @@ void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
b_info->max_memkb = l * 1024;
+ if (!xlu_cfg_get_list(config, "llc_colors", &llc_colors, &num_llc_colors, 0)) {
+ int k, cur_index = 0;
+
+ b_info->num_llc_colors = 0;
+ for (i = 0; i < num_llc_colors; i++) {
+ uint32_t start = 0, end = 0;
+
+ buf = xlu_cfg_get_listitem(llc_colors, i);
+ if (!buf) {
+ fprintf(stderr,
+ "xl: Can't get element %d in LLC color list\n", i);
+ exit(1);
+ }
+
+ if (sscanf(buf, "%" SCNu32 "-%" SCNu32, &start, &end) != 2) {
+ if (sscanf(buf, "%" SCNu32, &start) != 1) {
+ fprintf(stderr, "xl: Invalid LLC color range: %s\n", buf);
+ exit(1);
+ }
+ end = start;
+ } else if (start > end) {
+ fprintf(stderr,
+ "xl: Start LLC color is greater than end: %s\n", buf);
+ exit(1);
+ }
+
+ b_info->num_llc_colors += (end - start) + 1;
+ b_info->llc_colors = (uint32_t *)realloc(b_info->llc_colors,
+ sizeof(*b_info->llc_colors) * b_info->num_llc_colors);
+
+ for (k = start; k <= end; k++)
+ b_info->llc_colors[cur_index++] = k;
+ }
+ }
+
if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
vcpus = l;
if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, l)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 05/11] tools: add support for cache coloring configuration
2023-01-23 15:47 ` [PATCH v4 05/11] tools: add support for cache coloring configuration Carlo Nonato
@ 2023-01-26 14:22 ` Anthony PERARD
2023-01-26 16:34 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Anthony PERARD @ 2023-01-26 14:22 UTC (permalink / raw)
To: Carlo Nonato; +Cc: xen-devel, Wei Liu, Juergen Gross, Marco Solieri
On Mon, Jan 23, 2023 at 04:47:29PM +0100, Carlo Nonato wrote:
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index e939d07157..064f54c349 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -28,6 +28,20 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
> {
> int err;
> DECLARE_DOMCTL;
> + DECLARE_HYPERCALL_BUFFER(uint32_t, llc_colors);
> +
> + if ( config->num_llc_colors )
> + {
> + size_t bytes = sizeof(uint32_t) * config->num_llc_colors;
> +
> + llc_colors = xc_hypercall_buffer_alloc(xch, llc_colors, bytes);
> + if ( llc_colors == NULL ) {
> + PERROR("Could not allocate LLC colors for xc_domain_create");
> + return -ENOMEM;
> + }
> + memcpy(llc_colors, config->llc_colors.p, bytes);
> + set_xen_guest_handle(config->llc_colors, llc_colors);
I think this two lines looks wrong. There is a double usage of
config->llc_colors, to both store a user pointer and then to store an
hypercall buffer. Also, accessing llc_colors.p field is probably wrong.
I guess the caller of xc_domain_create() (that is libxl) will have to
take care of the hypercall buffer. It is already filling the
xen_domctl_createdomain struct that is passed to the hypercall, so it's
probably fine to handle hypercall buffer which is part of it.
What happen in the hypervisor when both num_llc_colors and llc_colors
are set to 0 in the struct xen_domctl_createdomain? Is it fine? That is
to figure out if all users of xc_domain_create() needs to be updated.
Also, ocaml binding is "broken", or at least needs updating. This is due
to the addition of llc_colors into xen_domctl_createdomain, the size
been different than the expected size.
> + }
>
> domctl.cmd = XEN_DOMCTL_createdomain;
> domctl.domain = *pdomid;
> @@ -39,6 +53,9 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
> *pdomid = (uint16_t)domctl.domain;
> *config = domctl.u.createdomain;
>
> + if ( llc_colors )
> + xc_hypercall_buffer_free(xch, llc_colors);
> +
> return 0;
> }
>
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index beec3f6b6f..6d0c768241 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -638,6 +638,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> .grant_opts = XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
> .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
> .cpupool_id = info->poolid,
> + .num_llc_colors = b_info->num_llc_colors,
> + .llc_colors.p = b_info->llc_colors,
> };
>
> if (info->type != LIBXL_DOMAIN_TYPE_PV) {
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 0cfad8508d..1f944ca6d7 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -562,6 +562,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("ioports", Array(libxl_ioport_range, "num_ioports")),
> ("irqs", Array(uint32, "num_irqs")),
> ("iomem", Array(libxl_iomem_range, "num_iomem")),
> + ("llc_colors", Array(uint32, "num_llc_colors")),
For this, you are going to need to add a LIBXL_HAVE_ macro in libxl.h.
They are plenty of example as well as an explanation.
But a good name I guess would be LIBXL_HAVE_BUILDINFO_LLC_COLORS along
with a short comment.
> ("claim_mode", libxl_defbool),
> ("event_channels", uint32),
> ("kernel", string),
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 05/11] tools: add support for cache coloring configuration
2023-01-26 14:22 ` Anthony PERARD
@ 2023-01-26 16:34 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 16:34 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Juergen Gross, Marco Solieri
Hi Anthony,
On Thu, Jan 26, 2023 at 3:22 PM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Mon, Jan 23, 2023 at 04:47:29PM +0100, Carlo Nonato wrote:
> > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> > index e939d07157..064f54c349 100644
> > --- a/tools/libs/ctrl/xc_domain.c
> > +++ b/tools/libs/ctrl/xc_domain.c
> > @@ -28,6 +28,20 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
> > {
> > int err;
> > DECLARE_DOMCTL;
> > + DECLARE_HYPERCALL_BUFFER(uint32_t, llc_colors);
> > +
> > + if ( config->num_llc_colors )
> > + {
> > + size_t bytes = sizeof(uint32_t) * config->num_llc_colors;
> > +
> > + llc_colors = xc_hypercall_buffer_alloc(xch, llc_colors, bytes);
> > + if ( llc_colors == NULL ) {
> > + PERROR("Could not allocate LLC colors for xc_domain_create");
> > + return -ENOMEM;
> > + }
> > + memcpy(llc_colors, config->llc_colors.p, bytes);
> > + set_xen_guest_handle(config->llc_colors, llc_colors);
>
> I think this two lines looks wrong. There is a double usage of
> config->llc_colors, to both store a user pointer and then to store an
> hypercall buffer. Also, accessing llc_colors.p field is probably wrong.
> I guess the caller of xc_domain_create() (that is libxl) will have to
> take care of the hypercall buffer. It is already filling the
> xen_domctl_createdomain struct that is passed to the hypercall, so it's
> probably fine to handle hypercall buffer which is part of it.
This is what I did in v3 :) (https://marc.info/?l=xen-devel&m=166930291506578)
However things probably will change again because of a new interface in Xen.
> What happen in the hypervisor when both num_llc_colors and llc_colors
> are set to 0 in the struct xen_domctl_createdomain? Is it fine? That is
> to figure out if all users of xc_domain_create() needs to be updated.
A default coloring configuration is generated if the array is null or its
length is 0. So no problems on the Xen side.
> Also, ocaml binding is "broken", or at least needs updating. This is due
> to the addition of llc_colors into xen_domctl_createdomain, the size
> been different than the expected size.
>
>
> > + }
> >
> > domctl.cmd = XEN_DOMCTL_createdomain;
> > domctl.domain = *pdomid;
> > @@ -39,6 +53,9 @@ int xc_domain_create(xc_interface *xch, uint32_t *pdomid,
> > *pdomid = (uint16_t)domctl.domain;
> > *config = domctl.u.createdomain;
> >
> > + if ( llc_colors )
> > + xc_hypercall_buffer_free(xch, llc_colors);
> > +
> > return 0;
> > }
> >
> > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> > index beec3f6b6f..6d0c768241 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -638,6 +638,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > .grant_opts = XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
> > .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
> > .cpupool_id = info->poolid,
> > + .num_llc_colors = b_info->num_llc_colors,
> > + .llc_colors.p = b_info->llc_colors,
> > };
> >
> > if (info->type != LIBXL_DOMAIN_TYPE_PV) {
> > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> > index 0cfad8508d..1f944ca6d7 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -562,6 +562,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("ioports", Array(libxl_ioport_range, "num_ioports")),
> > ("irqs", Array(uint32, "num_irqs")),
> > ("iomem", Array(libxl_iomem_range, "num_iomem")),
> > + ("llc_colors", Array(uint32, "num_llc_colors")),
>
> For this, you are going to need to add a LIBXL_HAVE_ macro in libxl.h.
> They are plenty of example as well as an explanation.
> But a good name I guess would be LIBXL_HAVE_BUILDINFO_LLC_COLORS along
> with a short comment.
Ok, thanks for the explanation.
> > ("claim_mode", libxl_defbool),
> > ("event_channels", uint32),
> > ("kernel", string),
>
>
> Thanks,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 06/11] xen/arm: add support for cache coloring configuration via device-tree
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (4 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 05/11] tools: add support for cache coloring configuration Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 07/11] xen: add cache coloring allocator for domains Carlo Nonato
` (5 subsequent siblings)
11 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This commit adds the "llc-colors" Device Tree attribute that can be used
for DomUs and Dom0less color configurations. The syntax is the same used
for every color config.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
docs/misc/arm/cache-coloring.rst | 43 +++++++++++++++++++++++++
docs/misc/arm/device-tree/booting.txt | 4 +++
xen/arch/arm/domain_build.c | 18 ++++++++++-
xen/arch/arm/include/asm/llc_coloring.h | 3 ++
xen/arch/arm/llc_coloring.c | 10 ++++++
5 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index c2e0e87426..a28f75dc26 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -116,6 +116,49 @@ 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".
+DomUs colors can be set either in the xl configuration file (relative
+documentation at "docs/man/xl.cfg.pod.5.in") or via Device Tree, also for
+Dom0less configurations (relative documentation in
+"docs/misc/arm/device-tree/booting.txt"), as in the following example:
+
+.. raw:: html
+
+ <pre>
+ xen,xen-bootargs = "console=dtuart dtuart=serial0 dom0_mem=1G dom0_max_vcpus=1 sched=null llc-coloring=on llc-way-size=64K xen-llc-colors=0-1 dom0-llc-colors=2-6";
+ xen,dom0-bootargs "console=hvc0 earlycon=xen earlyprintk=xen root=/dev/ram0"
+
+ dom0 {
+ compatible = "xen,linux-zimage" "xen,multiboot-module";
+ reg = <0x0 0x1000000 0x0 15858176>;
+ };
+
+ dom0-ramdisk {
+ compatible = "xen,linux-initrd" "xen,multiboot-module";
+ reg = <0x0 0x2000000 0x0 20638062>;
+ };
+
+ domU0 {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ compatible = "xen,domain";
+ memory = <0x0 0x40000>;
+ llc-colors = "4-8,10,11,12";
+ cpus = <0x1>;
+ vpl011 = <0x1>;
+
+ module@2000000 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ reg = <0x2000000 0xffffff>;
+ bootargs = "console=ttyAMA0";
+ };
+
+ module@30000000 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ reg = <0x3000000 0xffffff>;
+ };
+ };
+ </pre>
+
**Note:** If no color configuration is provided for a domain, the default one,
which corresponds to all available colors, is used instead.
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 3879340b5e..ad71c16b00 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -162,6 +162,10 @@ with the following properties:
An integer specifying the number of vcpus to allocate to the guest.
+- llc-colors
+ A string specifying the LLC color configuration for the guest.
+ Refer to "docs/misc/arm/cache_coloring.rst" for syntax.
+
- vpl011
An empty property to enable/disable a virtual pl011 for the guest to
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 093d4ad6f6..2c1307d349 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3854,6 +3854,8 @@ void __init create_domUs(void)
struct dt_device_node *node;
const struct dt_device_node *cpupool_node,
*chosen = dt_find_node_by_path("/chosen");
+ const char *llc_colors_str;
+ unsigned int *llc_colors = NULL, num_llc_colors = 0;
BUG_ON(chosen == NULL);
dt_for_each_child_node(chosen, node)
@@ -3960,12 +3962,26 @@ void __init create_domUs(void)
d_cfg.max_maptrack_frames = val;
}
+ if ( !dt_property_read_string(node, "llc-colors", &llc_colors_str) )
+ {
+ if ( !llc_coloring_enabled )
+ printk(XENLOG_WARNING
+ "'llc-colors' found, but LLC coloring is disabled\n");
+ else if ( dt_find_property(node, "xen,static-mem", NULL) )
+ panic("static-mem and LLC coloring are incompatible\n");
+ else
+ llc_colors = llc_colors_from_str(llc_colors_str,
+ &num_llc_colors);
+ }
+
/*
* The variable max_init_domid is initialized with zero, so here it's
* very important to use the pre-increment operator to call
* domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
*/
- d = domain_create(++max_init_domid, &d_cfg, flags);
+ d = domain_create_llc_colored(++max_init_domid, &d_cfg, flags,
+ llc_colors, num_llc_colors);
+
if ( IS_ERR(d) )
panic("Error creating domain %s\n", dt_node_name(node));
diff --git a/xen/arch/arm/include/asm/llc_coloring.h b/xen/arch/arm/include/asm/llc_coloring.h
index 382ff7de47..7a01b8841c 100644
--- a/xen/arch/arm/include/asm/llc_coloring.h
+++ b/xen/arch/arm/include/asm/llc_coloring.h
@@ -18,12 +18,15 @@
bool __init llc_coloring_init(void);
unsigned int *dom0_llc_colors(unsigned int *num_colors);
+unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors);
#else /* !CONFIG_LLC_COLORING */
static inline bool __init llc_coloring_init(void) { return true; }
static inline unsigned int *dom0_llc_colors(
unsigned int *num_colors) { return NULL; }
+static inline unsigned int *llc_colors_from_str(
+ const char *str, unsigned int *num_colors) { return NULL; }
#endif /* CONFIG_LLC_COLORING */
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 2d0457cdbc..ba5279a022 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -289,6 +289,16 @@ unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config)
return colors;
}
+unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors)
+{
+ unsigned int *colors = alloc_colors(nr_colors);
+
+ if ( parse_color_config(str, colors, num_colors) )
+ panic("Error parsing LLC color configuration\n");
+
+ return colors;
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (5 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 06/11] xen/arm: add support for cache coloring configuration via device-tree Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-24 16:50 ` Jan Beulich
` (2 more replies)
2023-01-23 15:47 ` [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables Carlo Nonato
` (4 subsequent siblings)
11 siblings, 3 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Marco Solieri, Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
This commit adds a new memory page allocator that implements the cache
coloring mechanism. The allocation algorithm follows the given domain color
configuration and maximizes contiguity in the page selection of multiple
subsequent requests.
Pages are stored in a color-indexed array of lists, each one sorted by
machine address, that is referred to as "colored heap". Those lists are
filled by a simple init function which computes the color of each page.
When a domain requests a page, the allocator takes one from those lists
whose colors equals the domain configuration. It chooses the page with the
lowest machine address such that contiguous pages are sequentially
allocated if this is made possible by a color assignment which includes
adjacent colors.
The allocator can handle only requests with order equal to 0 since the
single color granularity is represented in memory by one page.
The buddy allocator must coexist with the colored one because the Xen heap
isn't colored. For this reason a new Kconfig option and a command line
parameter are added to let the user set the amount of memory reserved for
the buddy allocator. Even when cache coloring is enabled, this memory
isn't managed by the colored allocator.
Colored heap information is dumped in the dump_heap() debug-key function.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
v4:
- moved colored allocator code after buddy allocator because it now has
some dependencies on buddy functions
- buddy_alloc_size is now used only by the colored allocator
- fixed a bug that allowed the buddy to merge pages when they were colored
- free_color_heap_page() now calls mark_page_free()
- free_color_heap_page() uses of the frametable array for faster searches
- added FIXME comment for the linear search in free_color_heap_page()
- removed alloc_color_domheap_page() to let the colored allocator exploit
some more buddy allocator code
- alloc_color_heap_page() now allocs min address pages first
- reduced the mess in end_boot_allocator(): use the first loop for
init_color_heap_pages()
- fixed page_list_add_prev() (list.h) since it was doing the opposite of
what it was supposed to do
- fixed page_list_add_prev() (non list.h) to check also for next existence
- removed unused page_list_add_next()
- moved p2m code in another patch
---
docs/misc/arm/cache-coloring.rst | 49 ++++++
docs/misc/xen-command-line.pandoc | 14 ++
xen/arch/Kconfig | 12 ++
xen/arch/arm/include/asm/mm.h | 3 +
xen/arch/arm/llc_coloring.c | 12 ++
xen/common/page_alloc.c | 247 +++++++++++++++++++++++++++---
xen/include/xen/llc_coloring.h | 5 +
xen/include/xen/mm.h | 33 ++++
8 files changed, 355 insertions(+), 20 deletions(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index a28f75dc26..d56dafe815 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -15,10 +15,16 @@ In Kconfig:
value meaning and when it should be changed).
CONFIG_NR_LLC_COLORS=<n>
+- If needed, change the amount of memory reserved for the buddy allocator
+ (see `Colored allocator and buddy allocator`_).
+
+ CONFIG_BUDDY_ALLOCATOR_SIZE=<n>
Compile Xen and the toolstack and then:
- Set the `llc-coloring=on` command line option.
+- If needed, set the amount of memory reserved for the buddy allocator
+ via the appropriate command line option.
- Set `Coloring parameters and domain configurations`_.
Background
@@ -162,6 +168,18 @@ Dom0less configurations (relative documentation in
**Note:** If no color configuration is provided for a domain, the default one,
which corresponds to all available colors, is used instead.
+Colored allocator and buddy allocator
+*************************************
+
+The colored allocator distributes pages based on color configurations of
+domains so that each domains only gets pages of its own colors.
+The colored allocator is meant as an alternative to the buddy allocator because
+its allocation policy is by definition incompatible with the generic one. Since
+the Xen heap is not colored yet, we need to support the coexistence of the two
+allocators and some memory must be left for the buddy one.
+The buddy allocator memory can be reserved from the Xen Kconfig or with the
+help of a command-line option.
+
Known issues and limitations
****************************
@@ -172,3 +190,34 @@ In the domain configuration, "xen,static-mem" allows memory to be statically
allocated to the domain. This isn't possibile when LLC coloring is enabled,
because that memory can't be guaranteed to use only colors assigned to the
domain.
+
+Cache coloring is intended only for embedded systems
+####################################################
+
+The current implementation aims to satisfy the need of predictability in
+embedded systems with small amount of memory to be managed in a colored way.
+Given that, some shortcuts are taken in the development. Expect worse
+performances on larger systems.
+
+Colored allocator can only make use of order-0 pages
+####################################################
+
+The cache coloring technique relies on memory mappings and on the smallest
+amount of memory that can be mapped to achieve the maximum number of colors
+(cache partitions) possible. This amount is what is normally called a page and,
+in Xen terminology, the order-0 page is the smallest one. The fairly simple
+colored allocator currently implemented, makes use only of such pages.
+It must be said that a more complex one could, in theory, adopt higher order
+pages if the colors selection contained adjacent colors. Two subsequent colors,
+for example, can be represented by an order-1 page, four colors correspond to
+an order-2 page, etc.
+
+Fail to boot colored DomUs with large memory size
+#################################################
+
+If the Linux kernel used for Dom0 does not contain the upstream commit
+3941552aec1e04d63999988a057ae09a1c56ebeb and uses the hypercall buffer device,
+colored DomUs with memory size larger then 127 MB cannot be created. This is
+caused by the default limit of this buffer of 64 pages. The solution is to
+manually apply the above patch, or to check if there is an updated version of
+the kernel in use for Dom0 that contains this change.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index eb105c03af..a89c0cef61 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
cause Xen not to use Indirect Branch Tracking even when support is
available in hardware.
+### buddy-alloc-size (arm64)
+> `= <size>`
+
+> Default: `64M`
+
+Amount of memory reserved for the buddy allocator when colored allocator is
+active. This options is available only when `CONFIG_LLC_COLORING` is enabled.
+The colored allocator is meant as an alternative to the buddy allocator,
+because its allocation policy is by definition incompatible with the
+generic one. Since the Xen heap systems is not colored yet, we need to
+support the coexistence of the two allocators for now. This parameter, which is
+optional and for expert only, it's used to set the amount of memory reserved to
+the buddy allocator.
+
### clocksource (x86)
> `= pit | hpet | acpi | tsc`
diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 39c23f2528..4378b5f338 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -45,3 +45,15 @@ config NR_LLC_COLORS
more than what needed in the general case.
Note that if, at any time, a color configuration with more colors than
the maximum is employed, an error is produced.
+
+config BUDDY_ALLOCATOR_SIZE
+ int "Buddy allocator reserved memory size (MiB)"
+ default "64"
+ depends on LLC_COLORING
+ help
+ Amount of memory reserved for the buddy allocator to work alongside
+ the colored one. The colored allocator is meant as an alternative to the
+ buddy allocator because its allocation policy is by definition
+ incompatible with the generic one. Since the Xen heap is not colored yet,
+ we need to support the coexistence of the two allocators and some memory
+ must be left for the buddy one.
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index bff6923f3e..596293f792 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -128,6 +128,9 @@ struct page_info
#else
#define PGC_static 0
#endif
+/* Page is cache colored */
+#define _PGC_colored PG_shift(4)
+#define PGC_colored PG_mask(1, 4)
/* ... */
/* Page is broken? */
#define _PGC_broken PG_shift(7)
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index ba5279a022..22612d455b 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -33,6 +33,8 @@ static paddr_t __ro_after_init addr_col_mask;
static unsigned int __ro_after_init dom0_colors[CONFIG_NR_LLC_COLORS];
static unsigned int __ro_after_init dom0_num_colors;
+#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
+
/*
* Parse the coloring configuration given in the buf string, following the
* syntax below.
@@ -299,6 +301,16 @@ unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors)
return colors;
}
+unsigned int page_to_llc_color(const struct page_info *pg)
+{
+ return addr_to_color(page_to_maddr(pg));
+}
+
+unsigned int get_nr_llc_colors(void)
+{
+ return nr_colors;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index e40473f71e..59bd6bcdac 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -126,6 +126,7 @@
#include <xen/irq.h>
#include <xen/keyhandler.h>
#include <xen/lib.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/nodemask.h>
#include <xen/numa.h>
@@ -158,6 +159,10 @@
#define PGC_static 0
#endif
+#ifndef PGC_colored
+#define PGC_colored 0
+#endif
+
#ifndef PGT_TYPE_INFO_INITIALIZER
#define PGT_TYPE_INFO_INITIALIZER 0
#endif
@@ -924,6 +929,13 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
}
}
+/* Initialise fields which have other uses for free pages. */
+static void init_free_page_fields(struct page_info *pg)
+{
+ pg->u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
+ page_set_owner(pg, NULL);
+}
+
/* Allocate 2^@order contiguous pages. */
static struct page_info *alloc_heap_pages(
unsigned int zone_lo, unsigned int zone_hi,
@@ -1032,10 +1044,7 @@ static struct page_info *alloc_heap_pages(
accumulate_tlbflush(&need_tlbflush, &pg[i],
&tlbflush_timestamp);
- /* Initialise fields which have other uses for free pages. */
- pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
- page_set_owner(&pg[i], NULL);
-
+ init_free_page_fields(&pg[i]);
}
spin_unlock(&heap_lock);
@@ -1488,7 +1497,7 @@ static void free_heap_pages(
/* Merge with predecessor block? */
if ( !mfn_valid(page_to_mfn(predecessor)) ||
!page_state_is(predecessor, free) ||
- (predecessor->count_info & PGC_static) ||
+ (predecessor->count_info & (PGC_static | PGC_colored)) ||
(PFN_ORDER(predecessor) != order) ||
(page_to_nid(predecessor) != node) )
break;
@@ -1512,7 +1521,7 @@ static void free_heap_pages(
/* Merge with successor block? */
if ( !mfn_valid(page_to_mfn(successor)) ||
!page_state_is(successor, free) ||
- (successor->count_info & PGC_static) ||
+ (successor->count_info & (PGC_static | PGC_colored)) ||
(PFN_ORDER(successor) != order) ||
(page_to_nid(successor) != node) )
break;
@@ -1928,6 +1937,182 @@ static unsigned long avail_heap_pages(
return free_pages;
}
+#ifdef CONFIG_LLC_COLORING
+/*************************
+ * COLORED SIDE-ALLOCATOR
+ *
+ * Pages are grouped by LLC color in lists which are globally referred to as the
+ * color heap. Lists are populated in end_boot_allocator().
+ * After initialization there will be N lists where N is the number of
+ * available colors on the platform.
+ */
+typedef struct page_list_head colored_pages_t;
+static colored_pages_t *__ro_after_init _color_heap;
+static unsigned long *__ro_after_init free_colored_pages;
+
+/* Memory required for buddy allocator to work with colored one */
+static unsigned long __initdata buddy_alloc_size =
+ CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
+
+#define color_heap(color) (&_color_heap[color])
+
+static bool is_free_colored_page(struct page_info *page)
+{
+ return page && (page->count_info & PGC_state_free) &&
+ (page->count_info & PGC_colored);
+}
+
+/*
+ * The {free|alloc}_color_heap_page overwrite pg->count_info, but they do it in
+ * the same way as the buddy allocator corresponding functions do:
+ * protecting the access with a critical section using heap_lock.
+ */
+static void free_color_heap_page(struct page_info *pg)
+{
+ unsigned int color = page_to_llc_color(pg), nr_colors = get_nr_llc_colors();
+ unsigned long pdx = page_to_pdx(pg);
+ colored_pages_t *head = color_heap(color);
+ struct page_info *prev = pdx >= nr_colors ? pg - nr_colors : NULL;
+ struct page_info *next = pdx + nr_colors < FRAMETABLE_NR ? pg + nr_colors
+ : NULL;
+
+ spin_lock(&heap_lock);
+
+ if ( is_free_colored_page(prev) )
+ next = page_list_next(prev, head);
+ else if ( !is_free_colored_page(next) )
+ {
+ /*
+ * FIXME: linear search is slow, but also note that the frametable is
+ * used to find free pages in the immediate neighborhood of pg in
+ * constant time. When freeing contiguous pages, the insert position of
+ * most of them is found without the linear search.
+ */
+ page_list_for_each( next, head )
+ {
+ if ( page_to_maddr(next) > page_to_maddr(pg) )
+ break;
+ }
+ }
+
+ mark_page_free(pg, page_to_mfn(pg));
+ pg->count_info |= PGC_colored;
+ free_colored_pages[color]++;
+ page_list_add_prev(pg, next, head);
+
+ spin_unlock(&heap_lock);
+}
+
+static struct page_info *alloc_color_heap_page(unsigned int memflags,
+ struct domain *d)
+{
+ struct page_info *pg = NULL;
+ unsigned int i, color;
+ bool need_tlbflush = false;
+ uint32_t tlbflush_timestamp = 0;
+
+ spin_lock(&heap_lock);
+
+ for ( i = 0; i < d->num_llc_colors; i++ )
+ {
+ struct page_info *tmp;
+
+ if ( page_list_empty(color_heap(d->llc_colors[i])) )
+ continue;
+
+ tmp = page_list_first(color_heap(d->llc_colors[i]));
+ if ( !pg || page_to_maddr(tmp) < page_to_maddr(pg) )
+ {
+ pg = tmp;
+ color = d->llc_colors[i];
+ }
+ }
+
+ if ( !pg )
+ {
+ spin_unlock(&heap_lock);
+ return NULL;
+ }
+
+ pg->count_info = PGC_state_inuse | PGC_colored;
+ free_colored_pages[color]--;
+ page_list_del(pg, color_heap(color));
+
+ if ( !(memflags & MEMF_no_tlbflush) )
+ accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
+
+ init_free_page_fields(pg);
+
+ spin_unlock(&heap_lock);
+
+ if ( need_tlbflush )
+ filtered_flush_tlb_mask(tlbflush_timestamp);
+
+ flush_page_to_ram(mfn_x(page_to_mfn(pg)),
+ !(memflags & MEMF_no_icache_flush));
+
+ return pg;
+}
+
+static void __init init_color_heap_pages(struct page_info *pg,
+ unsigned long nr_pages)
+{
+ unsigned int i;
+
+ if ( buddy_alloc_size )
+ {
+ unsigned long buddy_pages = min(PFN_DOWN(buddy_alloc_size), nr_pages);
+
+ init_heap_pages(pg, buddy_pages);
+ nr_pages -= buddy_pages;
+ buddy_alloc_size -= buddy_pages << PAGE_SHIFT;
+ pg += buddy_pages;
+ }
+
+ if ( !_color_heap )
+ {
+ unsigned int nr_colors = get_nr_llc_colors();
+
+ _color_heap = xmalloc_array(colored_pages_t, nr_colors);
+ BUG_ON(!_color_heap);
+ free_colored_pages = xzalloc_array(unsigned long, nr_colors);
+ BUG_ON(!free_colored_pages);
+
+ for ( i = 0; i < nr_colors; i++ )
+ INIT_PAGE_LIST_HEAD(color_heap(i));
+ }
+
+ printk(XENLOG_DEBUG
+ "Init color heap with %lu pages starting from: %#"PRIx64"\n",
+ nr_pages, page_to_maddr(pg));
+
+ for ( i = 0; i < nr_pages; i++ )
+ free_color_heap_page(&pg[i]);
+}
+
+static void dump_color_heap(void)
+{
+ unsigned int color;
+
+ printk("Dumping color heap info\n");
+ for ( color = 0; color < get_nr_llc_colors(); color++ )
+ printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
+}
+
+#else /* !CONFIG_LLC_COLORING */
+
+static void free_color_heap_page(struct page_info *pg) {}
+static void __init init_color_heap_pages(struct page_info *pg,
+ unsigned long nr_pages) {}
+static struct page_info *alloc_color_heap_page(unsigned int memflags,
+ struct domain *d)
+{
+ return NULL;
+}
+static void dump_color_heap(void) {}
+
+#endif /* CONFIG_LLC_COLORING */
+
void __init end_boot_allocator(void)
{
unsigned int i;
@@ -1936,12 +2121,19 @@ void __init end_boot_allocator(void)
for ( i = 0; i < nr_bootmem_regions; i++ )
{
struct bootmem_region *r = &bootmem_region_list[i];
- if ( (r->s < r->e) &&
- (mfn_to_nid(_mfn(r->s)) == cpu_to_node(0)) )
+ if ( r->s < r->e )
{
- init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
- r->e = r->s;
- break;
+ if ( llc_coloring_enabled )
+ {
+ init_color_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ r->e = r->s;
+ }
+ else if ( mfn_to_nid(_mfn(r->s)) == cpu_to_node(0) )
+ {
+ init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ r->e = r->s;
+ break;
+ }
}
}
for ( i = nr_bootmem_regions; i-- > 0; )
@@ -2332,6 +2524,7 @@ int assign_pages(
{
int rc = 0;
unsigned int i;
+ unsigned long allowed_flags = (PGC_extra | PGC_static | PGC_colored);
spin_lock(&d->page_alloc_lock);
@@ -2349,7 +2542,7 @@ int assign_pages(
for ( i = 0; i < nr; i++ )
{
- ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
+ ASSERT(!(pg[i].count_info & ~allowed_flags));
if ( pg[i].count_info & PGC_extra )
extra_pages++;
}
@@ -2408,8 +2601,8 @@ int assign_pages(
ASSERT(page_get_owner(&pg[i]) == NULL);
page_set_owner(&pg[i], d);
smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
- pg[i].count_info =
- (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1;
+ pg[i].count_info = (pg[i].count_info & allowed_flags) |
+ PGC_allocated | 1;
page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
}
@@ -2443,7 +2636,14 @@ struct page_info *alloc_domheap_pages(
if ( memflags & MEMF_no_owner )
memflags |= MEMF_no_refcount;
- if ( !dma_bitsize )
+ /* Only domains are supported for coloring */
+ if ( d && is_domain_llc_colored(d) )
+ {
+ /* Colored allocation must be done on 0 order */
+ if ( order || (pg = alloc_color_heap_page(memflags, d)) == NULL )
+ return NULL;
+ }
+ else if ( !dma_bitsize )
memflags &= ~MEMF_no_dma;
else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi )
pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
@@ -2468,7 +2668,10 @@ struct page_info *alloc_domheap_pages(
}
if ( assign_page(pg, order, d, memflags) )
{
- free_heap_pages(pg, order, memflags & MEMF_no_scrub);
+ if ( pg->count_info & PGC_colored )
+ free_color_heap_page(pg);
+ else
+ free_heap_pages(pg, order, memflags & MEMF_no_scrub);
return NULL;
}
}
@@ -2551,7 +2754,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
scrub = 1;
}
- free_heap_pages(pg, order, scrub);
+ if ( pg->count_info & PGC_colored )
+ free_color_heap_page(pg);
+ else
+ free_heap_pages(pg, order, scrub);
}
if ( drop_dom_ref )
@@ -2658,6 +2864,9 @@ static void cf_check dump_heap(unsigned char key)
continue;
printk("Node %d has %lu unscrubbed pages\n", i, node_need_scrub[i]);
}
+
+ if ( llc_coloring_enabled )
+ dump_color_heap();
}
static __init int cf_check register_heap_trigger(void)
@@ -2790,9 +2999,7 @@ static bool prepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
* to PGC_state_inuse.
*/
pg[i].count_info = PGC_static | PGC_state_inuse;
- /* Initialise fields which have other uses for free pages. */
- pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
- page_set_owner(&pg[i], NULL);
+ init_free_page_fields(&pg[i]);
}
spin_unlock(&heap_lock);
diff --git a/xen/include/xen/llc_coloring.h b/xen/include/xen/llc_coloring.h
index 2855f38296..2e9abf3b3a 100644
--- a/xen/include/xen/llc_coloring.h
+++ b/xen/include/xen/llc_coloring.h
@@ -17,6 +17,8 @@
#include <asm/llc_coloring.h>
+struct page_info;
+
extern bool llc_coloring_enabled;
int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
@@ -26,6 +28,9 @@ void domain_dump_llc_colors(struct domain *d);
unsigned int *llc_colors_from_guest(struct xen_domctl_createdomain *config);
+unsigned int page_to_llc_color(const struct page_info *pg);
+unsigned int get_nr_llc_colors(void);
+
#else
#define llc_coloring_enabled (false)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 9d14aed74b..8ea72c744e 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -299,6 +299,33 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
}
head->tail = page;
}
+static inline void
+_page_list_add(struct page_info *page, struct page_info *prev,
+ struct page_info *next)
+{
+ page->list.prev = page_to_pdx(prev);
+ page->list.next = page_to_pdx(next);
+ prev->list.next = page_to_pdx(page);
+ next->list.prev = page_to_pdx(page);
+}
+static inline void
+page_list_add_prev(struct page_info *page, struct page_info *next,
+ struct page_list_head *head)
+{
+ struct page_info *prev;
+
+ if ( !next )
+ {
+ page_list_add_tail(page, head);
+ return;
+ }
+
+ prev = page_list_prev(next, head);
+ if ( !prev )
+ page_list_add(page, head);
+ else
+ _page_list_add(page, prev, next);
+}
static inline bool_t
__page_list_del_head(struct page_info *page, struct page_list_head *head,
struct page_info *next, struct page_info *prev)
@@ -451,6 +478,12 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
list_add_tail(&page->list, head);
}
static inline void
+page_list_add_prev(struct page_info *page, struct page_info *next,
+ struct page_list_head *head)
+{
+ list_add_tail(&page->list, &next->list);
+}
+static inline void
page_list_del(struct page_info *page, struct page_list_head *head)
{
list_del(&page->list);
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-23 15:47 ` [PATCH v4 07/11] xen: add cache coloring allocator for domains Carlo Nonato
@ 2023-01-24 16:50 ` Jan Beulich
2023-01-26 11:00 ` Carlo Nonato
2023-01-24 16:58 ` Jan Beulich
2023-01-26 16:29 ` Jan Beulich
2 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-24 16:50 UTC (permalink / raw)
To: Carlo Nonato
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
>
> This commit adds a new memory page allocator that implements the cache
> coloring mechanism. The allocation algorithm follows the given domain color
> configuration and maximizes contiguity in the page selection of multiple
> subsequent requests.
>
> Pages are stored in a color-indexed array of lists, each one sorted by
> machine address, that is referred to as "colored heap". Those lists are
> filled by a simple init function which computes the color of each page.
> When a domain requests a page, the allocator takes one from those lists
> whose colors equals the domain configuration. It chooses the page with the
> lowest machine address such that contiguous pages are sequentially
> allocated if this is made possible by a color assignment which includes
> adjacent colors.
What use is this with ...
> The allocator can handle only requests with order equal to 0 since the
> single color granularity is represented in memory by one page.
... this restriction? Plus aiui there's no guarantee of contiguous pages
coming back in any event (because things depend on what may have been
allocated / freed earlier on), so why even give the impression of there
being a way to obtain contiguous pages?
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-24 16:50 ` Jan Beulich
@ 2023-01-26 11:00 ` Carlo Nonato
2023-01-26 12:37 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 11:00 UTC (permalink / raw)
To: Jan Beulich
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
Hi Jan,
On Tue, Jan 24, 2023 at 5:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> >
> > This commit adds a new memory page allocator that implements the cache
> > coloring mechanism. The allocation algorithm follows the given domain color
> > configuration and maximizes contiguity in the page selection of multiple
> > subsequent requests.
> >
> > Pages are stored in a color-indexed array of lists, each one sorted by
> > machine address, that is referred to as "colored heap". Those lists are
> > filled by a simple init function which computes the color of each page.
> > When a domain requests a page, the allocator takes one from those lists
> > whose colors equals the domain configuration. It chooses the page with the
> > lowest machine address such that contiguous pages are sequentially
> > allocated if this is made possible by a color assignment which includes
> > adjacent colors.
>
> What use is this with ...
>
> > The allocator can handle only requests with order equal to 0 since the
> > single color granularity is represented in memory by one page.
>
> ... this restriction? Plus aiui there's no guarantee of contiguous pages
> coming back in any event (because things depend on what may have been
> allocated / freed earlier on), so why even give the impression of there
> being a way to obtain contiguous pages?
I really need us to be on the same "page" (no pun intended) here cause we
discussed the subject multiple times and I'm probably missing important
details.
First, is physical memory contiguity important? I'm assuming this is good
because then some hardware optimization can occur when accessing memory.
I'm taking it for granted because it's what the original author of the series
thought, but I don't have an objective view of this.
Then, let's state what contiguity means with coloring:
*if* there are contiguous free pages and *if* subsequent requests are made
and *if* the coloring configuration allows it, the allocator guarantees
contiguity because it serves pages *in order*.
From the fragmentation perspective (first prerequisite), this is somewhat
similar to the buddy case where only if contiguous pages are freed they can
be allocated after. So order of operation is always important for
fragmentation in dynamic allocation. The main difference is speed
(I'm not comparing them on this aspect).
The second prerequisite requires that users of the allocator have exclusive
access to it until the request is carried out. If interleaved requests happen,
contiguity is practically impossible. How often does this happen? I view
allocation as something that happens mainly at domain creation time, one
domain at a time which results in a lot of subsequent requests, and then
contiguity (if other prerequisites hold) isn't an impression.
Obviously fragmentation is inherently higher with coloring because it actually
needs to partition memory, so the third prerequisite actually limits contiguity
a lot.
> Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-26 11:00 ` Carlo Nonato
@ 2023-01-26 12:37 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2023-01-26 12:37 UTC (permalink / raw)
To: Carlo Nonato
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 26.01.2023 12:00, Carlo Nonato wrote:
> On Tue, Jan 24, 2023 at 5:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>> From: Luca Miccio <lucmiccio@gmail.com>
>>>
>>> This commit adds a new memory page allocator that implements the cache
>>> coloring mechanism. The allocation algorithm follows the given domain color
>>> configuration and maximizes contiguity in the page selection of multiple
>>> subsequent requests.
>>>
>>> Pages are stored in a color-indexed array of lists, each one sorted by
>>> machine address, that is referred to as "colored heap". Those lists are
>>> filled by a simple init function which computes the color of each page.
>>> When a domain requests a page, the allocator takes one from those lists
>>> whose colors equals the domain configuration. It chooses the page with the
>>> lowest machine address such that contiguous pages are sequentially
>>> allocated if this is made possible by a color assignment which includes
>>> adjacent colors.
>>
>> What use is this with ...
>>
>>> The allocator can handle only requests with order equal to 0 since the
>>> single color granularity is represented in memory by one page.
>>
>> ... this restriction? Plus aiui there's no guarantee of contiguous pages
>> coming back in any event (because things depend on what may have been
>> allocated / freed earlier on), so why even give the impression of there
>> being a way to obtain contiguous pages?
>
> I really need us to be on the same "page" (no pun intended) here cause we
> discussed the subject multiple times and I'm probably missing important
> details.
>
> First, is physical memory contiguity important? I'm assuming this is good
> because then some hardware optimization can occur when accessing memory.
> I'm taking it for granted because it's what the original author of the series
> thought, but I don't have an objective view of this.
I'd need to have a reference to a concrete description of such hardware
behavior to believe it. On x86 I certainly know of an "adjacent cacheline
prefetch" optimization in hardware, but that won't cross page boundaries
afair.
Contiguous memory can be advantageous for I/O (allowing bigger chunks in
individual requests or s/g list elements), and it is certainly a prereq
to using large page mappings (which earlier on you already said isn't of
interest in your use case).
> Then, let's state what contiguity means with coloring:
> *if* there are contiguous free pages and *if* subsequent requests are made
> and *if* the coloring configuration allows it, the allocator guarantees
> contiguity because it serves pages *in order*.
I don't think it can, simply because ...
> From the fragmentation perspective (first prerequisite), this is somewhat
> similar to the buddy case where only if contiguous pages are freed they can
> be allocated after. So order of operation is always important for
> fragmentation in dynamic allocation. The main difference is speed
> (I'm not comparing them on this aspect).
>
> The second prerequisite requires that users of the allocator have exclusive
> access to it until the request is carried out. If interleaved requests happen,
> contiguity is practically impossible. How often does this happen? I view
> allocation as something that happens mainly at domain creation time, one
> domain at a time which results in a lot of subsequent requests, and then
> contiguity (if other prerequisites hold) isn't an impression.
... the "exclusive access" here is a fiction: Domain memory is populated
by the tool stack. Such tool stack allocations (even if properly serialized
in the control domain) will necessarily compete with anything done
internally in the hypervisor. Specifically the hypervisor may, at any time,
allocate a page (perhaps just for transient use), and then free that page
again, or free another one which was allocated before the tool stack
started populating domain memory.
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-23 15:47 ` [PATCH v4 07/11] xen: add cache coloring allocator for domains Carlo Nonato
2023-01-24 16:50 ` Jan Beulich
@ 2023-01-24 16:58 ` Jan Beulich
2023-01-26 16:29 ` Jan Beulich
2 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2023-01-24 16:58 UTC (permalink / raw)
To: Carlo Nonato, xen-devel
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri
On 23.01.2023 16:47, Carlo Nonato wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
> cause Xen not to use Indirect Branch Tracking even when support is
> available in hardware.
>
> +### buddy-alloc-size (arm64)
> +> `= <size>`
> +
> +> Default: `64M`
> +
> +Amount of memory reserved for the buddy allocator when colored allocator is
> +active. This options is available only when `CONFIG_LLC_COLORING` is enabled.
> +The colored allocator is meant as an alternative to the buddy allocator,
> +because its allocation policy is by definition incompatible with the
> +generic one. Since the Xen heap systems is not colored yet, we need to
> +support the coexistence of the two allocators for now. This parameter, which is
> +optional and for expert only, it's used to set the amount of memory reserved to
> +the buddy allocator.
> +
> ### clocksource (x86)
> > `= pit | hpet | acpi | tsc`
>
This hunk looks to be the result of a bad merge, as the new option should
go ahead of "cet", not after it.
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-23 15:47 ` [PATCH v4 07/11] xen: add cache coloring allocator for domains Carlo Nonato
2023-01-24 16:50 ` Jan Beulich
2023-01-24 16:58 ` Jan Beulich
@ 2023-01-26 16:29 ` Jan Beulich
2023-01-27 10:17 ` Carlo Nonato
2 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-26 16:29 UTC (permalink / raw)
To: Carlo Nonato
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
> cause Xen not to use Indirect Branch Tracking even when support is
> available in hardware.
>
> +### buddy-alloc-size (arm64)
I can't find where such a command line option would be processed.
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -128,6 +128,9 @@ struct page_info
> #else
> #define PGC_static 0
> #endif
> +/* Page is cache colored */
> +#define _PGC_colored PG_shift(4)
> +#define PGC_colored PG_mask(1, 4)
Is there a reason you don't follow the conditional approach we've taken
for PGC_static?
Thinking of which - what are the planned interactions with the static
allocator? If the two are exclusive of one another, I guess that would
need expressing somehow.
> --- a/xen/arch/arm/llc_coloring.c
> +++ b/xen/arch/arm/llc_coloring.c
> @@ -33,6 +33,8 @@ static paddr_t __ro_after_init addr_col_mask;
> static unsigned int __ro_after_init dom0_colors[CONFIG_NR_LLC_COLORS];
> static unsigned int __ro_after_init dom0_num_colors;
>
> +#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
You're shifting right by PAGE_SHIFT here just to ...
> @@ -299,6 +301,16 @@ unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors)
> return colors;
> }
>
> +unsigned int page_to_llc_color(const struct page_info *pg)
> +{
> + return addr_to_color(page_to_maddr(pg));
... undo the corresponding left shift in page_to_maddr(). Depending
on other uses of addr_col_mask it may be worthwhile to either change
that to or accompany it by a mask to operate on frame numbers.
> @@ -924,6 +929,13 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
> }
> }
>
> +/* Initialise fields which have other uses for free pages. */
> +static void init_free_page_fields(struct page_info *pg)
> +{
> + pg->u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> + page_set_owner(pg, NULL);
> +}
To limit the size of the functional change, abstracting out this function
could helpfully be a separate patch (and could then also likely go in ahead
of time, simplifying things slightly for you as well).
> @@ -1488,7 +1497,7 @@ static void free_heap_pages(
> /* Merge with predecessor block? */
> if ( !mfn_valid(page_to_mfn(predecessor)) ||
> !page_state_is(predecessor, free) ||
> - (predecessor->count_info & PGC_static) ||
> + (predecessor->count_info & (PGC_static | PGC_colored)) ||
> (PFN_ORDER(predecessor) != order) ||
> (page_to_nid(predecessor) != node) )
> break;
> @@ -1512,7 +1521,7 @@ static void free_heap_pages(
> /* Merge with successor block? */
> if ( !mfn_valid(page_to_mfn(successor)) ||
> !page_state_is(successor, free) ||
> - (successor->count_info & PGC_static) ||
> + (successor->count_info & (PGC_static | PGC_colored)) ||
> (PFN_ORDER(successor) != order) ||
> (page_to_nid(successor) != node) )
> break;
This, especially without being mentioned in the description (only in the
revision log), could likely also be split out (and then also be properly
justified).
> @@ -1928,6 +1937,182 @@ static unsigned long avail_heap_pages(
> return free_pages;
> }
>
> +#ifdef CONFIG_LLC_COLORING
> +/*************************
> + * COLORED SIDE-ALLOCATOR
> + *
> + * Pages are grouped by LLC color in lists which are globally referred to as the
> + * color heap. Lists are populated in end_boot_allocator().
> + * After initialization there will be N lists where N is the number of
> + * available colors on the platform.
> + */
> +typedef struct page_list_head colored_pages_t;
To me this type rather hides information, so I think I would prefer if
you dropped it.
> +static colored_pages_t *__ro_after_init _color_heap;
> +static unsigned long *__ro_after_init free_colored_pages;
> +
> +/* Memory required for buddy allocator to work with colored one */
> +static unsigned long __initdata buddy_alloc_size =
> + CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
Please don't open-code MB().
> +#define color_heap(color) (&_color_heap[color])
> +
> +static bool is_free_colored_page(struct page_info *page)
const please (and wherever applicable throughout the series)
> +{
> + return page && (page->count_info & PGC_state_free) &&
> + (page->count_info & PGC_colored);
> +}
> +
> +/*
> + * The {free|alloc}_color_heap_page overwrite pg->count_info, but they do it in
> + * the same way as the buddy allocator corresponding functions do:
> + * protecting the access with a critical section using heap_lock.
> + */
I think such a comment would only be useful if you did things differently,
even if just slightly. And indeed I think you do, e.g. by ORing in
PGC_colored below (albeit that's still similar to unprepare_staticmem_pages(),
so perhaps fine without further explanation). Differences are what may need
commenting on (such that the safety thereof can be judged upon).
> +static void free_color_heap_page(struct page_info *pg)
> +{
> + unsigned int color = page_to_llc_color(pg), nr_colors = get_nr_llc_colors();
> + unsigned long pdx = page_to_pdx(pg);
> + colored_pages_t *head = color_heap(color);
> + struct page_info *prev = pdx >= nr_colors ? pg - nr_colors : NULL;
> + struct page_info *next = pdx + nr_colors < FRAMETABLE_NR ? pg + nr_colors
> + : NULL;
Are these two calculations safe? At least on x86 parts of frame_table[] may
not be populated, so de-referencing prev and/or next might fault.
> + spin_lock(&heap_lock);
> +
> + if ( is_free_colored_page(prev) )
> + next = page_list_next(prev, head);
> + else if ( !is_free_colored_page(next) )
> + {
> + /*
> + * FIXME: linear search is slow, but also note that the frametable is
> + * used to find free pages in the immediate neighborhood of pg in
> + * constant time. When freeing contiguous pages, the insert position of
> + * most of them is found without the linear search.
> + */
> + page_list_for_each( next, head )
> + {
> + if ( page_to_maddr(next) > page_to_maddr(pg) )
> + break;
> + }
> + }
> +
> + mark_page_free(pg, page_to_mfn(pg));
> + pg->count_info |= PGC_colored;
> + free_colored_pages[color]++;
> + page_list_add_prev(pg, next, head);
> +
> + spin_unlock(&heap_lock);
> +}
There's no scrubbing here at all, and no mention of the lack thereof in
the description.
> +static void __init init_color_heap_pages(struct page_info *pg,
> + unsigned long nr_pages)
> +{
> + unsigned int i;
> +
> + if ( buddy_alloc_size )
> + {
> + unsigned long buddy_pages = min(PFN_DOWN(buddy_alloc_size), nr_pages);
> +
> + init_heap_pages(pg, buddy_pages);
> + nr_pages -= buddy_pages;
> + buddy_alloc_size -= buddy_pages << PAGE_SHIFT;
> + pg += buddy_pages;
> + }
I think you want to bail here if nr_pages is now zero, not the least to avoid
crashing ...
> + if ( !_color_heap )
> + {
> + unsigned int nr_colors = get_nr_llc_colors();
> +
> + _color_heap = xmalloc_array(colored_pages_t, nr_colors);
> + BUG_ON(!_color_heap);
> + free_colored_pages = xzalloc_array(unsigned long, nr_colors);
> + BUG_ON(!free_colored_pages);
... here in case the amount that was freed was really tiny.
> + for ( i = 0; i < nr_colors; i++ )
> + INIT_PAGE_LIST_HEAD(color_heap(i));
> + }
> +
> + printk(XENLOG_DEBUG
> + "Init color heap with %lu pages starting from: %#"PRIx64"\n",
> + nr_pages, page_to_maddr(pg));
> +
> + for ( i = 0; i < nr_pages; i++ )
> + free_color_heap_page(&pg[i]);
> +}
> +
> +static void dump_color_heap(void)
> +{
> + unsigned int color;
> +
> + printk("Dumping color heap info\n");
> + for ( color = 0; color < get_nr_llc_colors(); color++ )
> + printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
When there are many colors and most memory is used, you may produce a
lot of output here for just displaying zeros. May I suggest that you
log only non-zero values?
> +}
> +
> +#else /* !CONFIG_LLC_COLORING */
> +
> +static void free_color_heap_page(struct page_info *pg) {}
> +static void __init init_color_heap_pages(struct page_info *pg,
> + unsigned long nr_pages) {}
> +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> + struct domain *d)
> +{
> + return NULL;
> +}
> +static void dump_color_heap(void) {}
As said elsewhere (albeit for a slightly different reason): It may be
worthwhile to try to omit these stubs and instead expose the normal
code to the compiler unconditionally, relying on DCE. That'll reduce
the risk of people breaking the coloring code without noticing, when
build-testing only other configurations.
> @@ -1936,12 +2121,19 @@ void __init end_boot_allocator(void)
> for ( i = 0; i < nr_bootmem_regions; i++ )
> {
> struct bootmem_region *r = &bootmem_region_list[i];
> - if ( (r->s < r->e) &&
> - (mfn_to_nid(_mfn(r->s)) == cpu_to_node(0)) )
> + if ( r->s < r->e )
> {
> - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> - r->e = r->s;
> - break;
> + if ( llc_coloring_enabled )
> + {
> + init_color_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + r->e = r->s;
> + }
> + else if ( mfn_to_nid(_mfn(r->s)) == cpu_to_node(0) )
> + {
> + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + r->e = r->s;
> + break;
> + }
I think the coloring part here deserves a comment, or else it can easily
look as if there was a missing "break" (or it was placed in the wrong
scope). I also think it might help to restructure your change a little,
both to reduce the diff and to keep indentation bounded:
if ( r->s >= r->e )
continue;
if ( llc_coloring_enabled )
...
Also please take the opportunity to add the missing blank lines between
declaration and statements.
> @@ -2332,6 +2524,7 @@ int assign_pages(
> {
> int rc = 0;
> unsigned int i;
> + unsigned long allowed_flags = (PGC_extra | PGC_static | PGC_colored);
This is one of the few cases where I think "const" would be helpful even
on a not-pointed-to type. There's also not really any need for parentheses
here. As to the name, ...
> @@ -2349,7 +2542,7 @@ int assign_pages(
>
> for ( i = 0; i < nr; i++ )
> {
> - ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
> + ASSERT(!(pg[i].count_info & ~allowed_flags));
... while "allowed" may be fine for this use, it really isn't ...
> @@ -2408,8 +2601,8 @@ int assign_pages(
> ASSERT(page_get_owner(&pg[i]) == NULL);
> page_set_owner(&pg[i], d);
> smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
> - pg[i].count_info =
> - (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1;
> + pg[i].count_info = (pg[i].count_info & allowed_flags) |
> + PGC_allocated | 1;
... here. Maybe "preserved_flags" (or just "preserved")?
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -299,6 +299,33 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> }
> head->tail = page;
> }
> +static inline void
> +_page_list_add(struct page_info *page, struct page_info *prev,
> + struct page_info *next)
> +{
> + page->list.prev = page_to_pdx(prev);
> + page->list.next = page_to_pdx(next);
> + prev->list.next = page_to_pdx(page);
> + next->list.prev = page_to_pdx(page);
> +}
> +static inline void
> +page_list_add_prev(struct page_info *page, struct page_info *next,
> + struct page_list_head *head)
> +{
> + struct page_info *prev;
> +
> + if ( !next )
> + {
> + page_list_add_tail(page, head);
> + return;
> + }
!next is ambiguous in its meaning, so a comment towards the intended
behavior here would be helpful. It could be that the tail insertion is
necessary behavior, but it also could be that insertion anywhere would
actually be okay, and tail insertion is merely the variant you ended up
picking.
Then again ...
> + prev = page_list_prev(next, head);
> + if ( !prev )
> + page_list_add(page, head);
> + else
> + _page_list_add(page, prev, next);
> +}
> static inline bool_t
> __page_list_del_head(struct page_info *page, struct page_list_head *head,
> struct page_info *next, struct page_info *prev)
> @@ -451,6 +478,12 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> list_add_tail(&page->list, head);
> }
> static inline void
> +page_list_add_prev(struct page_info *page, struct page_info *next,
> + struct page_list_head *head)
> +{
> + list_add_tail(&page->list, &next->list);
... you don't care about !next here at all?
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-26 16:29 ` Jan Beulich
@ 2023-01-27 10:17 ` Carlo Nonato
2023-01-27 13:31 ` Jan Beulich
0 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-27 10:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
Hi Jan,
On Thu, Jan 26, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
> > cause Xen not to use Indirect Branch Tracking even when support is
> > available in hardware.
> >
> > +### buddy-alloc-size (arm64)
>
> I can't find where such a command line option would be processed.
Another merge problem... sorry.
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -128,6 +128,9 @@ struct page_info
> > #else
> > #define PGC_static 0
> > #endif
> > +/* Page is cache colored */
> > +#define _PGC_colored PG_shift(4)
> > +#define PGC_colored PG_mask(1, 4)
>
> Is there a reason you don't follow the conditional approach we've taken
> for PGC_static?
Nope, fixed that.
> Thinking of which - what are the planned interactions with the static
> allocator? If the two are exclusive of one another, I guess that would
> need expressing somehow.
>
> > --- a/xen/arch/arm/llc_coloring.c
> > +++ b/xen/arch/arm/llc_coloring.c
> > @@ -33,6 +33,8 @@ static paddr_t __ro_after_init addr_col_mask;
> > static unsigned int __ro_after_init dom0_colors[CONFIG_NR_LLC_COLORS];
> > static unsigned int __ro_after_init dom0_num_colors;
> >
> > +#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
>
> You're shifting right by PAGE_SHIFT here just to ...
>
> > @@ -299,6 +301,16 @@ unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors)
> > return colors;
> > }
> >
> > +unsigned int page_to_llc_color(const struct page_info *pg)
> > +{
> > + return addr_to_color(page_to_maddr(pg));
>
> ... undo the corresponding left shift in page_to_maddr(). Depending
> on other uses of addr_col_mask it may be worthwhile to either change
> that to or accompany it by a mask to operate on frame numbers.
Yep, this would simplify things probably.
> > @@ -924,6 +929,13 @@ static struct page_info *get_free_buddy(unsigned int zone_lo,
> > }
> > }
> >
> > +/* Initialise fields which have other uses for free pages. */
> > +static void init_free_page_fields(struct page_info *pg)
> > +{
> > + pg->u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> > + page_set_owner(pg, NULL);
> > +}
>
> To limit the size of the functional change, abstracting out this function
> could helpfully be a separate patch (and could then also likely go in ahead
> of time, simplifying things slightly for you as well).
>
> > @@ -1488,7 +1497,7 @@ static void free_heap_pages(
> > /* Merge with predecessor block? */
> > if ( !mfn_valid(page_to_mfn(predecessor)) ||
> > !page_state_is(predecessor, free) ||
> > - (predecessor->count_info & PGC_static) ||
> > + (predecessor->count_info & (PGC_static | PGC_colored)) ||
> > (PFN_ORDER(predecessor) != order) ||
> > (page_to_nid(predecessor) != node) )
> > break;
> > @@ -1512,7 +1521,7 @@ static void free_heap_pages(
> > /* Merge with successor block? */
> > if ( !mfn_valid(page_to_mfn(successor)) ||
> > !page_state_is(successor, free) ||
> > - (successor->count_info & PGC_static) ||
> > + (successor->count_info & (PGC_static | PGC_colored)) ||
> > (PFN_ORDER(successor) != order) ||
> > (page_to_nid(successor) != node) )
> > break;
>
> This, especially without being mentioned in the description (only in the
> revision log), could likely also be split out (and then also be properly
> justified).
You mean to make it a prereq patch or putting it after this patch?
In the second case it would be like a fix for the previous patch, something
that is to be avoided, right?
> > @@ -1928,6 +1937,182 @@ static unsigned long avail_heap_pages(
> > return free_pages;
> > }
> >
> > +#ifdef CONFIG_LLC_COLORING
> > +/*************************
> > + * COLORED SIDE-ALLOCATOR
> > + *
> > + * Pages are grouped by LLC color in lists which are globally referred to as the
> > + * color heap. Lists are populated in end_boot_allocator().
> > + * After initialization there will be N lists where N is the number of
> > + * available colors on the platform.
> > + */
> > +typedef struct page_list_head colored_pages_t;
>
> To me this type rather hides information, so I think I would prefer if
> you dropped it.
Ok.
> > +static colored_pages_t *__ro_after_init _color_heap;
> > +static unsigned long *__ro_after_init free_colored_pages;
> > +
> > +/* Memory required for buddy allocator to work with colored one */
> > +static unsigned long __initdata buddy_alloc_size =
> > + CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
>
> Please don't open-code MB().
>
> > +#define color_heap(color) (&_color_heap[color])
> > +
> > +static bool is_free_colored_page(struct page_info *page)
>
> const please (and wherever applicable throughout the series)
>
> > +{
> > + return page && (page->count_info & PGC_state_free) &&
> > + (page->count_info & PGC_colored);
> > +}
> > +
> > +/*
> > + * The {free|alloc}_color_heap_page overwrite pg->count_info, but they do it in
> > + * the same way as the buddy allocator corresponding functions do:
> > + * protecting the access with a critical section using heap_lock.
> > + */
>
> I think such a comment would only be useful if you did things differently,
> even if just slightly. And indeed I think you do, e.g. by ORing in
> PGC_colored below (albeit that's still similar to unprepare_staticmem_pages(),
> so perhaps fine without further explanation). Differences are what may need
> commenting on (such that the safety thereof can be judged upon).
>
> > +static void free_color_heap_page(struct page_info *pg)
> > +{
> > + unsigned int color = page_to_llc_color(pg), nr_colors = get_nr_llc_colors();
> > + unsigned long pdx = page_to_pdx(pg);
> > + colored_pages_t *head = color_heap(color);
> > + struct page_info *prev = pdx >= nr_colors ? pg - nr_colors : NULL;
> > + struct page_info *next = pdx + nr_colors < FRAMETABLE_NR ? pg + nr_colors
> > + : NULL;
>
> Are these two calculations safe? At least on x86 parts of frame_table[] may
> not be populated, so de-referencing prev and/or next might fault.
I have to admit I've not taken this into consideration. I'll check that.
> > + spin_lock(&heap_lock);
> > +
> > + if ( is_free_colored_page(prev) )
> > + next = page_list_next(prev, head);
> > + else if ( !is_free_colored_page(next) )
> > + {
> > + /*
> > + * FIXME: linear search is slow, but also note that the frametable is
> > + * used to find free pages in the immediate neighborhood of pg in
> > + * constant time. When freeing contiguous pages, the insert position of
> > + * most of them is found without the linear search.
> > + */
> > + page_list_for_each( next, head )
> > + {
> > + if ( page_to_maddr(next) > page_to_maddr(pg) )
> > + break;
> > + }
> > + }
> > +
> > + mark_page_free(pg, page_to_mfn(pg));
> > + pg->count_info |= PGC_colored;
> > + free_colored_pages[color]++;
> > + page_list_add_prev(pg, next, head);
> > +
> > + spin_unlock(&heap_lock);
> > +}
>
> There's no scrubbing here at all, and no mention of the lack thereof in
> the description.
This comes from the very first version (which I'm not an author of).
Can you explain to me briefly what is it and if I need it? Or can you give
me pointers?
> > +static void __init init_color_heap_pages(struct page_info *pg,
> > + unsigned long nr_pages)
> > +{
> > + unsigned int i;
> > +
> > + if ( buddy_alloc_size )
> > + {
> > + unsigned long buddy_pages = min(PFN_DOWN(buddy_alloc_size), nr_pages);
> > +
> > + init_heap_pages(pg, buddy_pages);
> > + nr_pages -= buddy_pages;
> > + buddy_alloc_size -= buddy_pages << PAGE_SHIFT;
> > + pg += buddy_pages;
> > + }
>
> I think you want to bail here if nr_pages is now zero, not the least to avoid
> crashing ...
>
> > + if ( !_color_heap )
> > + {
> > + unsigned int nr_colors = get_nr_llc_colors();
> > +
> > + _color_heap = xmalloc_array(colored_pages_t, nr_colors);
> > + BUG_ON(!_color_heap);
> > + free_colored_pages = xzalloc_array(unsigned long, nr_colors);
> > + BUG_ON(!free_colored_pages);
>
> ... here in case the amount that was freed was really tiny.
Here the array is allocated with nr_colors not nr_pages. nr_colors can't be 0.
And if nr_pages is 0 it just means that all pages went to the buddy. I see no
crash in this case.
> > + for ( i = 0; i < nr_colors; i++ )
> > + INIT_PAGE_LIST_HEAD(color_heap(i));
> > + }
> > +
> > + printk(XENLOG_DEBUG
> > + "Init color heap with %lu pages starting from: %#"PRIx64"\n",
> > + nr_pages, page_to_maddr(pg));
> > +
> > + for ( i = 0; i < nr_pages; i++ )
> > + free_color_heap_page(&pg[i]);
> > +}
> > +
> > +static void dump_color_heap(void)
> > +{
> > + unsigned int color;
> > +
> > + printk("Dumping color heap info\n");
> > + for ( color = 0; color < get_nr_llc_colors(); color++ )
> > + printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
>
> When there are many colors and most memory is used, you may produce a
> lot of output here for just displaying zeros. May I suggest that you
> log only non-zero values?
Yep.
> > +}
> > +
> > +#else /* !CONFIG_LLC_COLORING */
> > +
> > +static void free_color_heap_page(struct page_info *pg) {}
> > +static void __init init_color_heap_pages(struct page_info *pg,
> > + unsigned long nr_pages) {}
> > +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> > + struct domain *d)
> > +{
> > + return NULL;
> > +}
> > +static void dump_color_heap(void) {}
>
> As said elsewhere (albeit for a slightly different reason): It may be
> worthwhile to try to omit these stubs and instead expose the normal
> code to the compiler unconditionally, relying on DCE. That'll reduce
> the risk of people breaking the coloring code without noticing, when
> build-testing only other configurations.
Yep.
> > @@ -1936,12 +2121,19 @@ void __init end_boot_allocator(void)
> > for ( i = 0; i < nr_bootmem_regions; i++ )
> > {
> > struct bootmem_region *r = &bootmem_region_list[i];
> > - if ( (r->s < r->e) &&
> > - (mfn_to_nid(_mfn(r->s)) == cpu_to_node(0)) )
> > + if ( r->s < r->e )
> > {
> > - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> > - r->e = r->s;
> > - break;
> > + if ( llc_coloring_enabled )
> > + {
> > + init_color_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> > + r->e = r->s;
> > + }
> > + else if ( mfn_to_nid(_mfn(r->s)) == cpu_to_node(0) )
> > + {
> > + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> > + r->e = r->s;
> > + break;
> > + }
>
> I think the coloring part here deserves a comment, or else it can easily
> look as if there was a missing "break" (or it was placed in the wrong
> scope). I also think it might help to restructure your change a little,
> both to reduce the diff and to keep indentation bounded:
>
> if ( r->s >= r->e )
> continue;
>
> if ( llc_coloring_enabled )
> ...
>
> Also please take the opportunity to add the missing blank lines between
> declaration and statements.
>
> > @@ -2332,6 +2524,7 @@ int assign_pages(
> > {
> > int rc = 0;
> > unsigned int i;
> > + unsigned long allowed_flags = (PGC_extra | PGC_static | PGC_colored);
>
> This is one of the few cases where I think "const" would be helpful even
> on a not-pointed-to type. There's also not really any need for parentheses
> here. As to the name, ...
>
> > @@ -2349,7 +2542,7 @@ int assign_pages(
> >
> > for ( i = 0; i < nr; i++ )
> > {
> > - ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
> > + ASSERT(!(pg[i].count_info & ~allowed_flags));
>
> ... while "allowed" may be fine for this use, it really isn't ...
>
> > @@ -2408,8 +2601,8 @@ int assign_pages(
> > ASSERT(page_get_owner(&pg[i]) == NULL);
> > page_set_owner(&pg[i], d);
> > smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
> > - pg[i].count_info =
> > - (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1;
> > + pg[i].count_info = (pg[i].count_info & allowed_flags) |
> > + PGC_allocated | 1;
>
> ... here. Maybe "preserved_flags" (or just "preserved")?
>
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -299,6 +299,33 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> > }
> > head->tail = page;
> > }
> > +static inline void
> > +_page_list_add(struct page_info *page, struct page_info *prev,
> > + struct page_info *next)
> > +{
> > + page->list.prev = page_to_pdx(prev);
> > + page->list.next = page_to_pdx(next);
> > + prev->list.next = page_to_pdx(page);
> > + next->list.prev = page_to_pdx(page);
> > +}
> > +static inline void
> > +page_list_add_prev(struct page_info *page, struct page_info *next,
> > + struct page_list_head *head)
> > +{
> > + struct page_info *prev;
> > +
> > + if ( !next )
> > + {
> > + page_list_add_tail(page, head);
> > + return;
> > + }
>
> !next is ambiguous in its meaning, so a comment towards the intended
> behavior here would be helpful. It could be that the tail insertion is
> necessary behavior, but it also could be that insertion anywhere would
> actually be okay, and tail insertion is merely the variant you ended up
> picking.
This makes it possible to call the function when next has been used to iterate
over a list. If there's no next we are at the end of the list, so add to the
tail. The other way is to handle the case outside this function.
> Then again ...
>
> > + prev = page_list_prev(next, head);
> > + if ( !prev )
> > + page_list_add(page, head);
> > + else
> > + _page_list_add(page, prev, next);
> > +}
> > static inline bool_t
> > __page_list_del_head(struct page_info *page, struct page_list_head *head,
> > struct page_info *next, struct page_info *prev)
> > @@ -451,6 +478,12 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> > list_add_tail(&page->list, head);
> > }
> > static inline void
> > +page_list_add_prev(struct page_info *page, struct page_info *next,
> > + struct page_list_head *head)
> > +{
> > + list_add_tail(&page->list, &next->list);
>
> ... you don't care about !next here at all?
I did it this way since *page is never checked for NULL as well. Also, this
other type of list isn't NULL terminated.
> Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 07/11] xen: add cache coloring allocator for domains
2023-01-27 10:17 ` Carlo Nonato
@ 2023-01-27 13:31 ` Jan Beulich
0 siblings, 0 replies; 43+ messages in thread
From: Jan Beulich @ 2023-01-27 13:31 UTC (permalink / raw)
To: Carlo Nonato
Cc: Luca Miccio, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Marco Solieri, xen-devel
On 27.01.2023 11:17, Carlo Nonato wrote:
> On Thu, Jan 26, 2023 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.01.2023 16:47, Carlo Nonato wrote:
>>> --- a/xen/arch/arm/include/asm/mm.h
>>> +++ b/xen/arch/arm/include/asm/mm.h
>>> @@ -128,6 +128,9 @@ struct page_info
>>> #else
>>> #define PGC_static 0
>>> #endif
>>> +/* Page is cache colored */
>>> +#define _PGC_colored PG_shift(4)
>>> +#define PGC_colored PG_mask(1, 4)
>>
>> Is there a reason you don't follow the conditional approach we've taken
>> for PGC_static?
>
> Nope, fixed that.
And btw, if at all possible please avoid the #else part. page_alloc.c
already deals with that case, so it would be needed only if you have a
use of this constant somewhere else.
>>> @@ -1488,7 +1497,7 @@ static void free_heap_pages(
>>> /* Merge with predecessor block? */
>>> if ( !mfn_valid(page_to_mfn(predecessor)) ||
>>> !page_state_is(predecessor, free) ||
>>> - (predecessor->count_info & PGC_static) ||
>>> + (predecessor->count_info & (PGC_static | PGC_colored)) ||
>>> (PFN_ORDER(predecessor) != order) ||
>>> (page_to_nid(predecessor) != node) )
>>> break;
>>> @@ -1512,7 +1521,7 @@ static void free_heap_pages(
>>> /* Merge with successor block? */
>>> if ( !mfn_valid(page_to_mfn(successor)) ||
>>> !page_state_is(successor, free) ||
>>> - (successor->count_info & PGC_static) ||
>>> + (successor->count_info & (PGC_static | PGC_colored)) ||
>>> (PFN_ORDER(successor) != order) ||
>>> (page_to_nid(successor) != node) )
>>> break;
>>
>> This, especially without being mentioned in the description (only in the
>> revision log), could likely also be split out (and then also be properly
>> justified).
>
> You mean to make it a prereq patch or putting it after this patch?
A prereq, for ...
> In the second case it would be like a fix for the previous patch, something
> that is to be avoided, right?
... precisely this reason. Elsewhere you make a constant from
PGC_extra | PGC_static | PGC_colored. Maybe that could become a file scope
one, which you could then use here as well. Then the change wouldn't even
depend on you already having introduced PGC_colored (but otherwise just
having the stub #define earlier in the file would suffice as well for this
to compile fine independent of the bulk of the changes).
(FTAOD PGC_extra would be meaningless / benign here, for only ever being
set on allocated pages.)
>>> +static void free_color_heap_page(struct page_info *pg)
>>> +{
>>> + unsigned int color = page_to_llc_color(pg), nr_colors = get_nr_llc_colors();
>>> + unsigned long pdx = page_to_pdx(pg);
>>> + colored_pages_t *head = color_heap(color);
>>> + struct page_info *prev = pdx >= nr_colors ? pg - nr_colors : NULL;
>>> + struct page_info *next = pdx + nr_colors < FRAMETABLE_NR ? pg + nr_colors
>>> + : NULL;
>>
>> Are these two calculations safe? At least on x86 parts of frame_table[] may
>> not be populated, so de-referencing prev and/or next might fault.
>
> I have to admit I've not taken this into consideration. I'll check that.
>
>>> + spin_lock(&heap_lock);
>>> +
>>> + if ( is_free_colored_page(prev) )
>>> + next = page_list_next(prev, head);
>>> + else if ( !is_free_colored_page(next) )
>>> + {
>>> + /*
>>> + * FIXME: linear search is slow, but also note that the frametable is
>>> + * used to find free pages in the immediate neighborhood of pg in
>>> + * constant time. When freeing contiguous pages, the insert position of
>>> + * most of them is found without the linear search.
>>> + */
>>> + page_list_for_each( next, head )
>>> + {
>>> + if ( page_to_maddr(next) > page_to_maddr(pg) )
>>> + break;
>>> + }
>>> + }
>>> +
>>> + mark_page_free(pg, page_to_mfn(pg));
>>> + pg->count_info |= PGC_colored;
>>> + free_colored_pages[color]++;
>>> + page_list_add_prev(pg, next, head);
>>> +
>>> + spin_unlock(&heap_lock);
>>> +}
>>
>> There's no scrubbing here at all, and no mention of the lack thereof in
>> the description.
>
> This comes from the very first version (which I'm not an author of).
> Can you explain to me briefly what is it and if I need it? Or can you give
> me pointers?
Did you look for occurrences of the word "scrub" elsewhere in the file?
You want to avoid pages holding information from one guest to become
visible unchanged to another one.
>>> +static void __init init_color_heap_pages(struct page_info *pg,
>>> + unsigned long nr_pages)
>>> +{
>>> + unsigned int i;
>>> +
>>> + if ( buddy_alloc_size )
>>> + {
>>> + unsigned long buddy_pages = min(PFN_DOWN(buddy_alloc_size), nr_pages);
>>> +
>>> + init_heap_pages(pg, buddy_pages);
>>> + nr_pages -= buddy_pages;
>>> + buddy_alloc_size -= buddy_pages << PAGE_SHIFT;
>>> + pg += buddy_pages;
>>> + }
>>
>> I think you want to bail here if nr_pages is now zero, not the least to avoid
>> crashing ...
>>
>>> + if ( !_color_heap )
>>> + {
>>> + unsigned int nr_colors = get_nr_llc_colors();
>>> +
>>> + _color_heap = xmalloc_array(colored_pages_t, nr_colors);
>>> + BUG_ON(!_color_heap);
>>> + free_colored_pages = xzalloc_array(unsigned long, nr_colors);
>>> + BUG_ON(!free_colored_pages);
>>
>> ... here in case the amount that was freed was really tiny.
>
> Here the array is allocated with nr_colors not nr_pages. nr_colors can't be 0.
> And if nr_pages is 0 it just means that all pages went to the buddy. I see no
> crash in this case.
Aren't the two BUG_ON() not very clear crash causes? My comment wasn't
about nr_pages == 0 being an issue further down (I think I had convinced
myself that this case was handled correctly), but about the buddy
allocator still having too little memory to satisfy the two allocations
here (which also you don't need yet if there's no memory to go to the
colored allocator in the first place).
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -299,6 +299,33 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
>>> }
>>> head->tail = page;
>>> }
>>> +static inline void
>>> +_page_list_add(struct page_info *page, struct page_info *prev,
>>> + struct page_info *next)
>>> +{
>>> + page->list.prev = page_to_pdx(prev);
>>> + page->list.next = page_to_pdx(next);
>>> + prev->list.next = page_to_pdx(page);
>>> + next->list.prev = page_to_pdx(page);
>>> +}
>>> +static inline void
>>> +page_list_add_prev(struct page_info *page, struct page_info *next,
>>> + struct page_list_head *head)
>>> +{
>>> + struct page_info *prev;
>>> +
>>> + if ( !next )
>>> + {
>>> + page_list_add_tail(page, head);
>>> + return;
>>> + }
>>
>> !next is ambiguous in its meaning, so a comment towards the intended
>> behavior here would be helpful. It could be that the tail insertion is
>> necessary behavior, but it also could be that insertion anywhere would
>> actually be okay, and tail insertion is merely the variant you ended up
>> picking.
>
> This makes it possible to call the function when next has been used to iterate
> over a list. If there's no next we are at the end of the list, so add to the
> tail. The other way is to handle the case outside this function.
>
>> Then again ...
>>
>>> + prev = page_list_prev(next, head);
>>> + if ( !prev )
>>> + page_list_add(page, head);
>>> + else
>>> + _page_list_add(page, prev, next);
>>> +}
>>> static inline bool_t
>>> __page_list_del_head(struct page_info *page, struct page_list_head *head,
>>> struct page_info *next, struct page_info *prev)
>>> @@ -451,6 +478,12 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
>>> list_add_tail(&page->list, head);
>>> }
>>> static inline void
>>> +page_list_add_prev(struct page_info *page, struct page_info *next,
>>> + struct page_list_head *head)
>>> +{
>>> + list_add_tail(&page->list, &next->list);
>>
>> ... you don't care about !next here at all?
>
> I did it this way since *page is never checked for NULL as well. Also, this
> other type of list isn't NULL terminated.
I see. In which case properly explaining the intended behavior and use case
in the earlier function should hopefully eliminate the need for anything
special here. I have to admit though that I consider this a little fragile
for a seemingly generic helper function; I wonder whether the special case
wouldn't better be handled in the caller instead.
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (6 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 07/11] xen: add cache coloring allocator for domains Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-26 10:25 ` Julien Grall
2023-01-23 15:47 ` [PATCH v4 09/11] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START" Carlo Nonato
` (3 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
Cache colored domains can benefit from having p2m page tables allocated
with the same coloring schema so that isolation can be achieved also for
those kind of memory accesses.
In order to do that, the domain struct is passed to the allocator and the
MEMF_no_owner flag is used.
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- fixed p2m page allocation using MEMF_no_owner memflag
---
xen/arch/arm/p2m.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..f9faeb61af 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -4,6 +4,7 @@
#include <xen/iocap.h>
#include <xen/ioreq.h>
#include <xen/lib.h>
+#include <xen/llc_coloring.h>
#include <xen/sched.h>
#include <xen/softirq.h>
@@ -56,7 +57,10 @@ static struct page_info *p2m_alloc_page(struct domain *d)
*/
if ( is_hardware_domain(d) )
{
- pg = alloc_domheap_page(NULL, 0);
+ if ( is_domain_llc_colored(d) )
+ pg = alloc_domheap_page(d, MEMF_no_owner);
+ else
+ pg = alloc_domheap_page(NULL, 0);
if ( pg == NULL )
printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
}
@@ -105,7 +109,10 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
if ( d->arch.paging.p2m_total_pages < pages )
{
/* Need to allocate more memory from domheap */
- pg = alloc_domheap_page(NULL, 0);
+ if ( is_domain_llc_colored(d) )
+ pg = alloc_domheap_page(d, MEMF_no_owner);
+ else
+ pg = alloc_domheap_page(NULL, 0);
if ( pg == NULL )
{
printk(XENLOG_ERR "Failed to allocate P2M pages.\n");
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables
2023-01-23 15:47 ` [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables Carlo Nonato
@ 2023-01-26 10:25 ` Julien Grall
2023-01-26 11:02 ` Carlo Nonato
0 siblings, 1 reply; 43+ messages in thread
From: Julien Grall @ 2023-01-26 10:25 UTC (permalink / raw)
To: Carlo Nonato, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk, Marco Solieri
Hi Carlo,
On 23/01/2023 15:47, Carlo Nonato wrote:
> Cache colored domains can benefit from having p2m page tables allocated
> with the same coloring schema so that isolation can be achieved also for
> those kind of memory accesses.
> In order to do that, the domain struct is passed to the allocator and the
> MEMF_no_owner flag is used.
>
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> ---
> v4:
> - fixed p2m page allocation using MEMF_no_owner memflag
> ---
> xen/arch/arm/p2m.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..f9faeb61af 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -4,6 +4,7 @@
> #include <xen/iocap.h>
> #include <xen/ioreq.h>
> #include <xen/lib.h>
> +#include <xen/llc_coloring.h>
> #include <xen/sched.h>
> #include <xen/softirq.h>
>
> @@ -56,7 +57,10 @@ static struct page_info *p2m_alloc_page(struct domain *d)
> */
> if ( is_hardware_domain(d) )
> {
> - pg = alloc_domheap_page(NULL, 0);
> + if ( is_domain_llc_colored(d) )
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + else
> + pg = alloc_domheap_page(NULL, 0);
I don't think we need to special case a colored domain here.You could
simply always pass the domain/MEMF_no_owner and let the function decide
what to do.
This approach would also be useful when NUMA will be supported on Arm
(the series is still under review).
> if ( pg == NULL )
> printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
> }
> @@ -105,7 +109,10 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
> if ( d->arch.paging.p2m_total_pages < pages )
> {
> /* Need to allocate more memory from domheap */
> - pg = alloc_domheap_page(NULL, 0);
> + if ( is_domain_llc_colored(d) )
> + pg = alloc_domheap_page(d, MEMF_no_owner);
> + else
> + pg = alloc_domheap_page(NULL, 0);
Ditto.
> if ( pg == NULL )
> {
> printk(XENLOG_ERR "Failed to allocate P2M pages.\n");
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables
2023-01-26 10:25 ` Julien Grall
@ 2023-01-26 11:02 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-26 11:02 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
Hi Julien,
On Thu, Jan 26, 2023 at 11:25 AM Julien Grall <julien@xen.org> wrote:
>
> Hi Carlo,
>
> On 23/01/2023 15:47, Carlo Nonato wrote:
> > Cache colored domains can benefit from having p2m page tables allocated
> > with the same coloring schema so that isolation can be achieved also for
> > those kind of memory accesses.
> > In order to do that, the domain struct is passed to the allocator and the
> > MEMF_no_owner flag is used.
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> > ---
> > v4:
> > - fixed p2m page allocation using MEMF_no_owner memflag
> > ---
> > xen/arch/arm/p2m.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 948f199d84..f9faeb61af 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -4,6 +4,7 @@
> > #include <xen/iocap.h>
> > #include <xen/ioreq.h>
> > #include <xen/lib.h>
> > +#include <xen/llc_coloring.h>
> > #include <xen/sched.h>
> > #include <xen/softirq.h>
> >
> > @@ -56,7 +57,10 @@ static struct page_info *p2m_alloc_page(struct domain *d)
> > */
> > if ( is_hardware_domain(d) )
> > {
> > - pg = alloc_domheap_page(NULL, 0);
> > + if ( is_domain_llc_colored(d) )
> > + pg = alloc_domheap_page(d, MEMF_no_owner);
> > + else
> > + pg = alloc_domheap_page(NULL, 0);
> I don't think we need to special case a colored domain here.You could
> simply always pass the domain/MEMF_no_owner and let the function decide
> what to do.
>
> This approach would also be useful when NUMA will be supported on Arm
> (the series is still under review).
Ok, nice. This was pointed out also by Jan in the previous revision.
> > if ( pg == NULL )
> > printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
> > }
> > @@ -105,7 +109,10 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
> > if ( d->arch.paging.p2m_total_pages < pages )
> > {
> > /* Need to allocate more memory from domheap */
> > - pg = alloc_domheap_page(NULL, 0);
> > + if ( is_domain_llc_colored(d) )
> > + pg = alloc_domheap_page(d, MEMF_no_owner);
> > + else
> > + pg = alloc_domheap_page(NULL, 0);
>
> Ditto.
>
> > if ( pg == NULL )
> > {
> > printk(XENLOG_ERR "Failed to allocate P2M pages.\n");
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 09/11] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START"
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (7 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 08/11] xen/arm: use colored allocator for p2m page tables Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 10/11] xen/arm: add Xen cache colors command line parameter Carlo Nonato
` (2 subsequent siblings)
11 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This reverts commit 0c18fb76323bfb13615b6f13c98767face2d8097 (not clean).
This is not a clean revert since the rework of the memory layout, but it is
sufficiently similar to a clean one.
The only difference is that the BOOT_RELOC_VIRT_START must match the new
layout.
Cache coloring support for Xen needs to relocate Xen code and data in a new
colored physical space. The BOOT_RELOC_VIRT_START will be used as the virtual
base address for a temporary mapping to this new space.
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
xen/arch/arm/include/asm/config.h | 4 +++-
xen/arch/arm/mm.c | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index c5d407a749..5359acd529 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -96,7 +96,8 @@
* 2M - 4M Xen text, data, bss
* 4M - 6M Fixmap: special-purpose 4K mapping slots
* 6M - 10M Early boot mapping of FDT
- * 10M - 12M Livepatch vmap (if compiled in)
+ * 10M - 12M Early relocation address (used when relocating Xen)
+ * and later for livepatch vmap (if compiled in)
*
* 1G - 2G VMAP: ioremap and early_ioremap
*
@@ -133,6 +134,7 @@
#define BOOT_FDT_VIRT_START (FIXMAP_VIRT_START + FIXMAP_VIRT_SIZE)
#define BOOT_FDT_VIRT_SIZE _AT(vaddr_t, MB(4))
+#define BOOT_RELOC_VIRT_START (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
#ifdef CONFIG_LIVEPATCH
#define LIVEPATCH_VMAP_START (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
#define LIVEPATCH_VMAP_SIZE _AT(vaddr_t, MB(2))
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b9c698088b..7015a0f841 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -145,6 +145,7 @@ static void __init __maybe_unused build_assertions(void)
/* 2MB aligned regions */
BUILD_BUG_ON(XEN_VIRT_START & ~SECOND_MASK);
BUILD_BUG_ON(FIXMAP_ADDR(0) & ~SECOND_MASK);
+ BUILD_BUG_ON(BOOT_RELOC_VIRT_START & ~SECOND_MASK);
/* 1GB aligned regions */
#ifdef CONFIG_ARM_32
BUILD_BUG_ON(XENHEAP_VIRT_START & ~FIRST_MASK);
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v4 10/11] xen/arm: add Xen cache colors command line parameter
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (8 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 09/11] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START" Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-23 15:47 ` [PATCH v4 11/11] xen/arm: add cache coloring support for Xen Carlo Nonato
2023-01-23 15:52 ` [PATCH v4 00/11] Arm cache coloring Jan Beulich
11 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Luca Miccio, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri, Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
This commit adds a new command line parameter to configure Xen cache
colors. These colors can be dumped with the cache coloring info debug-key.
By default, Xen uses the first color.
Benchmarking the VM interrupt response time provides an estimation of
LLC usage by Xen's most latency-critical runtime task. Results on Arm
Cortex-A53 on Xilinx Zynq UltraScale+ XCZU9EG show that one color, which
reserves 64 KiB of L2, is enough to attain best responsiveness.
More colors are instead very likely to be needed on processors whose L1
cache is physically-indexed and physically-tagged, such as Cortex-A57.
In such cases, coloring applies to L1 also, and there typically are two
distinct L1-colors. Therefore, reserving only one color for Xen would
senselessly partitions a cache memory that is already private, i.e.
underutilize it. The default amount of Xen colors is thus set to one.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
docs/misc/xen-command-line.pandoc | 10 ++++++++++
xen/arch/arm/llc_coloring.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index a89c0cef61..d486946648 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2796,6 +2796,16 @@ In the case that x2apic is in use, this option switches between physical and
clustered mode. The default, given no hint from the **FADT**, is cluster
mode.
+### xen-llc-colors (arm64)
+> `= List of [ <integer> | <integer>-<integer> ]`
+
+> Default: `0: the lowermost color`
+
+Specify Xen LLC color configuration. This options is available only when
+`CONFIG_LLC_COLORING` is enabled.
+Two colors are most likely needed on platforms where private caches are
+physically indexed, e.g. the L1 instruction cache of the Arm Cortex-A57.
+
### xenheap_megabytes (arm32)
> `= <size>`
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 22612d455b..745e93a61a 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -19,6 +19,10 @@
#include <asm/processor.h>
#include <asm/sysregs.h>
+/* By default Xen uses the lowest color */
+#define XEN_DEFAULT_COLOR 0
+#define XEN_DEFAULT_NUM_COLORS 1
+
bool llc_coloring_enabled;
boolean_param("llc-coloring", llc_coloring_enabled);
@@ -33,6 +37,9 @@ static paddr_t __ro_after_init addr_col_mask;
static unsigned int __ro_after_init dom0_colors[CONFIG_NR_LLC_COLORS];
static unsigned int __ro_after_init dom0_num_colors;
+static unsigned int __ro_after_init xen_colors[CONFIG_NR_LLC_COLORS];
+static unsigned int __ro_after_init xen_num_colors;
+
#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
/*
@@ -83,6 +90,12 @@ static int parse_color_config(const char *buf, unsigned int *colors,
return *s ? -EINVAL : 0;
}
+static int __init parse_xen_colors(const char *s)
+{
+ return parse_color_config(s, xen_colors, &xen_num_colors);
+}
+custom_param("xen-llc-colors", parse_xen_colors);
+
static int __init parse_dom0_colors(const char *s)
{
return parse_color_config(s, dom0_colors, &dom0_num_colors);
@@ -166,6 +179,8 @@ static void dump_coloring_info(unsigned char key)
printk("LLC way size: %u KiB\n", llc_way_size >> 10);
printk("Number of LLC colors supported: %u\n", nr_colors);
printk("Address to LLC color mask: 0x%lx\n", addr_col_mask);
+ printk("Xen LLC colors: ");
+ print_colors(xen_colors, xen_num_colors);
}
bool __init llc_coloring_init(void)
@@ -202,6 +217,21 @@ bool __init llc_coloring_init(void)
addr_col_mask = (nr_colors - 1) << PAGE_SHIFT;
+ if ( !xen_num_colors )
+ {
+ printk(XENLOG_WARNING
+ "Xen LLC color config not found. Using default color: %u\n",
+ XEN_DEFAULT_COLOR);
+ xen_colors[0] = XEN_DEFAULT_COLOR;
+ xen_num_colors = XEN_DEFAULT_NUM_COLORS;
+ }
+
+ if ( !check_colors(xen_colors, xen_num_colors) )
+ {
+ printk(XENLOG_ERR "Bad LLC color config for Xen\n");
+ return false;
+ }
+
register_keyhandler('K', dump_coloring_info, "dump LLC coloring info", 1);
return true;
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v4 11/11] xen/arm: add cache coloring support for Xen
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (9 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 10/11] xen/arm: add Xen cache colors command line parameter Carlo Nonato
@ 2023-01-23 15:47 ` Carlo Nonato
2023-01-23 15:52 ` [PATCH v4 00/11] Arm cache coloring Jan Beulich
11 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 15:47 UTC (permalink / raw)
To: xen-devel
Cc: Carlo Nonato, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This commit adds the cache coloring support for Xen own physical space.
It extends the implementation of setup_pagetables() to make use of Xen
cache coloring configuration. Page tables construction is essentially the
same except for the fact that PTEs point to a new temporary mapped,
physically colored space.
The temporary mapping is also used to relocate Xen to the new physical
space starting at the address taken from the old get_xen_paddr() function
which is brought back for the occasion.
The temporary mapping is finally converted to a mapping of the "old"
(meaning the original physical space) Xen code, so that the boot CPU can
actually address the variables and functions used by secondary CPUs until
they enable the MMU. This happens when the boot CPU needs to bring up other
CPUs (psci.c and smpboot.c) and when the TTBR value is passed to them
(init_secondary_pagetables()).
Finally, since the alternative framework needs to remap the Xen text and
inittext sections, this operation must be done in a coloring-aware way.
The function xen_remap_colored() is introduced for that.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v4:
- removed set_value_for_secondary() because it was wrongly cleaning cache
- relocate_xen() now calls switch_ttbr_id()
---
xen/arch/arm/alternative.c | 9 ++-
xen/arch/arm/arm64/head.S | 50 +++++++++++++
xen/arch/arm/arm64/mm.c | 26 +++++--
xen/arch/arm/include/asm/llc_coloring.h | 22 ++++++
xen/arch/arm/include/asm/mm.h | 7 +-
xen/arch/arm/llc_coloring.c | 45 ++++++++++++
xen/arch/arm/mm.c | 94 ++++++++++++++++++++++---
xen/arch/arm/psci.c | 9 ++-
xen/arch/arm/setup.c | 75 +++++++++++++++++++-
xen/arch/arm/smpboot.c | 9 ++-
xen/arch/arm/xen.lds.S | 2 +-
11 files changed, 325 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index f00e3b9b3c..29f1ff34d4 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -9,6 +9,7 @@
#include <xen/init.h>
#include <xen/types.h>
#include <xen/kernel.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/vmap.h>
#include <xen/smp.h>
@@ -209,8 +210,12 @@ void __init apply_alternatives_all(void)
* The text and inittext section are read-only. So re-map Xen to
* be able to patch the code.
*/
- xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
- VMAP_DEFAULT);
+ if ( llc_coloring_enabled )
+ xenmap = xen_remap_colored(xen_mfn, xen_size);
+ else
+ xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
+ VMAP_DEFAULT);
+
/* Re-mapping Xen is not expected to fail during boot. */
BUG_ON(!xenmap);
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a61b4d3c27..9ed7610afa 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -801,6 +801,56 @@ fail: PRINT("- Boot failed -\r\n")
b 1b
ENDPROC(fail)
+GLOBAL(_end_boot)
+
+/* Copy Xen to new location and switch TTBR
+ * x0 ttbr
+ * x1 source address
+ * x2 destination address
+ * x3 length
+ *
+ * Source and destination must be word aligned, length is rounded up
+ * to a 16 byte boundary.
+ *
+ * MUST BE VERY CAREFUL when saving things to RAM over the copy */
+ENTRY(relocate_xen)
+ /* Copy 16 bytes at a time using:
+ * x9: counter
+ * x10: data
+ * x11: data
+ * x12: source
+ * x13: destination
+ */
+ mov x9, x3
+ mov x12, x1
+ mov x13, x2
+
+1: ldp x10, x11, [x12], #16
+ stp x10, x11, [x13], #16
+
+ subs x9, x9, #16
+ bgt 1b
+
+ /* Flush destination from dcache using:
+ * x9: counter
+ * x10: step
+ * x11: vaddr
+ */
+ dsb sy /* So the CPU issues all writes to the range */
+
+ mov x9, x3
+ ldr x10, =dcache_line_bytes /* x10 := step */
+ ldr x10, [x10]
+ mov x11, x2
+
+1: dc cvac, x11
+
+ add x11, x11, x10
+ subs x9, x9, x10
+ bgt 1b
+
+ b switch_ttbr_id
+
/*
* Switch TTBR
*
diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
index 2ede4e75ae..4419381fdd 100644
--- a/xen/arch/arm/arm64/mm.c
+++ b/xen/arch/arm/arm64/mm.c
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include <xen/init.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <asm/setup.h>
@@ -121,26 +122,43 @@ void update_identity_mapping(bool enable)
}
extern void switch_ttbr_id(uint64_t ttbr);
+extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
typedef void (switch_ttbr_fn)(uint64_t ttbr);
+typedef void (relocate_xen_fn)(uint64_t ttbr, void *src, void *dst, size_t len);
void __init switch_ttbr(uint64_t ttbr)
{
- vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
- switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
+ vaddr_t vaddr, id_addr;
lpae_t pte;
+ if ( llc_coloring_enabled )
+ vaddr = (vaddr_t)relocate_xen;
+ else
+ vaddr = (vaddr_t)switch_ttbr_id;
+
+ id_addr = virt_to_maddr(vaddr);
+
/* Enable the identity mapping in the boot page tables */
update_identity_mapping(true);
/* Enable the identity mapping in the runtime page tables */
- pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
+ pte = pte_of_xenaddr(vaddr);
pte.pt.table = 1;
pte.pt.xn = 0;
pte.pt.ro = 1;
write_pte(&xen_third_id[third_table_offset(id_addr)], pte);
/* Switch TTBR */
- fn(ttbr);
+ if ( llc_coloring_enabled )
+ {
+ relocate_xen_fn *fn = (relocate_xen_fn *)id_addr;
+ fn(ttbr, _start, (void *)BOOT_RELOC_VIRT_START, _end - _start);
+ }
+ else
+ {
+ switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
+ fn(ttbr);
+ }
/*
* Disable the identity mapping in the runtime page tables.
diff --git a/xen/arch/arm/include/asm/llc_coloring.h b/xen/arch/arm/include/asm/llc_coloring.h
index 7a01b8841c..ae5c4ff606 100644
--- a/xen/arch/arm/include/asm/llc_coloring.h
+++ b/xen/arch/arm/include/asm/llc_coloring.h
@@ -15,11 +15,28 @@
#ifdef CONFIG_LLC_COLORING
+#include <xen/mm-frame.h>
+
+/**
+ * Iterate over each Xen mfn in the colored space.
+ * @mfn: the current mfn. The first non colored mfn must be provided as the
+ * starting point.
+ * @i: loop index.
+ */
+#define for_each_xen_colored_mfn(mfn, i) \
+ for ( i = 0, mfn = xen_colored_mfn(mfn); \
+ i < (_end - _start) >> PAGE_SHIFT; \
+ i++, mfn = xen_colored_mfn(mfn_add(mfn, 1)) )
+
bool __init llc_coloring_init(void);
unsigned int *dom0_llc_colors(unsigned int *num_colors);
unsigned int *llc_colors_from_str(const char *str, unsigned int *num_colors);
+paddr_t xen_colored_map_size(paddr_t size);
+mfn_t xen_colored_mfn(mfn_t mfn);
+void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size);
+
#else /* !CONFIG_LLC_COLORING */
static inline bool __init llc_coloring_init(void) { return true; }
@@ -27,6 +44,11 @@ static inline unsigned int *dom0_llc_colors(
unsigned int *num_colors) { return NULL; }
static inline unsigned int *llc_colors_from_str(
const char *str, unsigned int *num_colors) { return NULL; }
+paddr_t xen_colored_map_size(paddr_t size) { return 0; }
+static inline void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size)
+{
+ return NULL;
+}
#endif /* CONFIG_LLC_COLORING */
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 596293f792..1b3be348b7 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -195,14 +195,19 @@ extern unsigned long total_pages;
#define PDX_GROUP_SHIFT SECOND_SHIFT
+#define virt_to_reloc_virt(virt) \
+ (((vaddr_t)virt) - XEN_VIRT_START + BOOT_RELOC_VIRT_START)
+
/* Boot-time pagetable setup */
-extern void setup_pagetables(unsigned long boot_phys_offset);
+extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
/* Map FDT in boot pagetable */
extern void *early_fdt_map(paddr_t fdt_paddr);
/* Switch to a new root page-tables */
extern void switch_ttbr(uint64_t ttbr);
/* Remove early mappings */
extern void remove_early_mappings(void);
+/* Remove early LLC coloring mappings */
+extern void remove_llc_coloring_mappings(void);
/* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the
* new page table */
extern int init_secondary_pagetables(int cpu);
diff --git a/xen/arch/arm/llc_coloring.c b/xen/arch/arm/llc_coloring.c
index 745e93a61a..ded1f33ad5 100644
--- a/xen/arch/arm/llc_coloring.c
+++ b/xen/arch/arm/llc_coloring.c
@@ -15,6 +15,7 @@
#include <xen/llc_coloring.h>
#include <xen/param.h>
#include <xen/types.h>
+#include <xen/vmap.h>
#include <asm/processor.h>
#include <asm/sysregs.h>
@@ -41,6 +42,8 @@ static unsigned int __ro_after_init xen_colors[CONFIG_NR_LLC_COLORS];
static unsigned int __ro_after_init xen_num_colors;
#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
+#define addr_set_color(addr, color) (((addr) & ~addr_col_mask) \
+ | ((color) << PAGE_SHIFT))
/*
* Parse the coloring configuration given in the buf string, following the
@@ -341,6 +344,48 @@ unsigned int get_nr_llc_colors(void)
return nr_colors;
}
+paddr_t xen_colored_map_size(paddr_t size)
+{
+ return ROUNDUP(size * nr_colors, XEN_PADDR_ALIGN);
+}
+
+mfn_t xen_colored_mfn(mfn_t mfn)
+{
+ paddr_t maddr = mfn_to_maddr(mfn);
+ unsigned int i, color = addr_to_color(maddr);
+
+ for( i = 0; i < xen_num_colors; i++ )
+ {
+ if ( color == xen_colors[i] )
+ return mfn;
+ else if ( color < xen_colors[i] )
+ return maddr_to_mfn(addr_set_color(maddr, xen_colors[i]));
+ }
+
+ /* Jump to next color space (llc_way_size bytes) and use the first color */
+ return maddr_to_mfn(addr_set_color(maddr + llc_way_size, xen_colors[0]));
+}
+
+void *xen_remap_colored(mfn_t xen_mfn, paddr_t xen_size)
+{
+ unsigned int i;
+ void *xenmap;
+ mfn_t *xen_colored_mfns = xmalloc_array(mfn_t, xen_size >> PAGE_SHIFT);
+
+ if ( !xen_colored_mfns )
+ panic("Can't allocate LLC colored MFNs\n");
+
+ for_each_xen_colored_mfn( xen_mfn, i )
+ {
+ xen_colored_mfns[i] = xen_mfn;
+ }
+
+ xenmap = vmap(xen_colored_mfns, xen_size >> PAGE_SHIFT);
+ xfree(xen_colored_mfns);
+
+ return xenmap;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7015a0f841..f14fb98088 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -14,6 +14,7 @@
#include <xen/guest_access.h>
#include <xen/init.h>
#include <xen/libfdt/libfdt.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/pfn.h>
#include <xen/pmap.h>
@@ -96,6 +97,9 @@ DEFINE_BOOT_PAGE_TABLE(boot_third);
DEFINE_PAGE_TABLE(xen_pgtable);
static DEFINE_PAGE_TABLE(xen_first);
#define THIS_CPU_PGTABLE xen_pgtable
+#ifdef CONFIG_LLC_COLORING
+static DEFINE_PAGE_TABLE(xen_colored_temp);
+#endif
#else
#define HYP_PT_ROOT_LEVEL 1
/* Per-CPU pagetable pages */
@@ -391,7 +395,12 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache)
lpae_t pte_of_xenaddr(vaddr_t va)
{
- paddr_t ma = va + phys_offset;
+ paddr_t ma;
+
+ if ( llc_coloring_enabled )
+ ma = virt_to_maddr(virt_to_reloc_virt(va));
+ else
+ ma = va + phys_offset;
return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
}
@@ -484,9 +493,54 @@ static void clear_table(void *table)
clean_and_invalidate_dcache_va_range(table, PAGE_SIZE);
}
-/* Boot-time pagetable setup.
- * Changes here may need matching changes in head.S */
-void __init setup_pagetables(unsigned long boot_phys_offset)
+#ifdef CONFIG_LLC_COLORING
+static void __init create_llc_coloring_mappings(paddr_t xen_paddr)
+{
+ lpae_t pte;
+ unsigned int i;
+ mfn_t mfn = maddr_to_mfn(xen_paddr);
+
+ for_each_xen_colored_mfn( mfn, i )
+ {
+ pte = mfn_to_xen_entry(mfn, MT_NORMAL);
+ pte.pt.table = 1; /* level 3 mappings always have this bit set */
+ xen_colored_temp[i] = pte;
+ }
+
+ pte = mfn_to_xen_entry(virt_to_mfn(xen_colored_temp), MT_NORMAL);
+ pte.pt.table = 1;
+ write_pte(&boot_second[second_table_offset(BOOT_RELOC_VIRT_START)], pte);
+}
+
+void __init remove_llc_coloring_mappings(void)
+{
+ int rc;
+
+ /* destroy the _PAGE_BLOCK mapping */
+ rc = modify_xen_mappings(BOOT_RELOC_VIRT_START,
+ BOOT_RELOC_VIRT_START + SZ_2M,
+ _PAGE_BLOCK);
+ BUG_ON(rc);
+}
+#else
+static void __init create_llc_coloring_mappings(paddr_t xen_paddr) {}
+void __init remove_llc_coloring_mappings(void) {}
+#endif /* CONFIG_LLC_COLORING */
+
+/*
+ * Boot-time pagetable setup with coloring support
+ * Changes here may need matching changes in head.S
+ *
+ * The coloring support consists of:
+ * - Create a temporary colored mapping that conforms to Xen color selection.
+ * - pte_of_xenaddr takes care of translating the virtual addresses to the
+ * new colored physical space and the returns the pte, so that the page table
+ * initialization can remain the same.
+ * - Copy Xen to the new colored physical space by exploiting the temporary
+ * mapping.
+ * - Update TTBR0_EL2 with the new root page table address.
+ */
+void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
{
uint64_t ttbr;
lpae_t pte, *p;
@@ -494,6 +548,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
phys_offset = boot_phys_offset;
+ if ( llc_coloring_enabled )
+ create_llc_coloring_mappings(xen_paddr);
+
arch_setup_page_tables();
#ifdef CONFIG_ARM_64
@@ -543,10 +600,13 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
pte.pt.table = 1;
xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte;
+ if ( llc_coloring_enabled )
+ ttbr = virt_to_maddr(virt_to_reloc_virt(xen_pgtable));
+ else
#ifdef CONFIG_ARM_64
- ttbr = (uintptr_t) xen_pgtable + phys_offset;
+ ttbr = (uintptr_t) xen_pgtable + phys_offset;
#else
- ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
+ ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
#endif
switch_ttbr(ttbr);
@@ -556,6 +616,18 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
#ifdef CONFIG_ARM_32
per_cpu(xen_pgtable, 0) = cpu0_pgtable;
#endif
+
+ /*
+ * Keep original Xen memory mapped because secondary CPUs still point to it
+ * and a few variables needs to be accessed by the master CPU in order to
+ * let them boot. This mapping will also replace the one created at the
+ * beginning of setup_pagetables.
+ */
+ if ( llc_coloring_enabled )
+ map_pages_to_xen(BOOT_RELOC_VIRT_START,
+ maddr_to_mfn(XEN_VIRT_START + phys_offset),
+ SZ_2M >> PAGE_SHIFT, PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
+
}
static void clear_boot_pagetables(void)
@@ -576,12 +648,18 @@ static void clear_boot_pagetables(void)
#ifdef CONFIG_ARM_64
int init_secondary_pagetables(int cpu)
{
+ uint64_t *init_ttbr_addr = &init_ttbr;
+
clear_boot_pagetables();
+ if ( llc_coloring_enabled )
+ init_ttbr_addr = (uint64_t *)virt_to_reloc_virt(&init_ttbr);
+
/* Set init_ttbr for this CPU coming up. All CPus share a single setof
* pagetables, but rewrite it each time for consistency with 32 bit. */
- init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
- clean_dcache(init_ttbr);
+ *init_ttbr_addr = virt_to_maddr(xen_pgtable);
+ clean_dcache(*init_ttbr_addr);
+
return 0;
}
#else
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 695d2fa1f1..fdc798dd14 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -11,6 +11,7 @@
#include <xen/types.h>
#include <xen/init.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/smp.h>
#include <asm/cpufeature.h>
@@ -39,9 +40,13 @@ static uint32_t psci_cpu_on_nr;
int call_psci_cpu_on(int cpu)
{
struct arm_smccc_res res;
+ vaddr_t init_secondary_addr = (vaddr_t)init_secondary;
- arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
- &res);
+ if ( llc_coloring_enabled )
+ init_secondary_addr = virt_to_reloc_virt(init_secondary);
+
+ arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
+ __pa(init_secondary_addr), &res);
return PSCI_RET(res);
}
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c04e5012f0..72da5a8e5e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -456,7 +456,7 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
return fdt;
}
-#ifdef CONFIG_ARM_32
+#if defined (CONFIG_ARM_32) || defined(CONFIG_LLC_COLORING)
/*
* Returns the end address of the highest region in the range s..e
* with required size and alignment that does not conflict with the
@@ -548,7 +548,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
}
return e;
}
+#endif
+#ifdef CONFIG_ARM_32
/*
* Find a contiguous region that fits in the static heap region with
* required size and alignment, and return the end address of the region
@@ -622,6 +624,62 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
return lowest;
}
+#ifdef CONFIG_LLC_COLORING
+/**
+ * get_xen_paddr - get physical address to relocate Xen to
+ *
+ * Xen is relocated to as near to the top of RAM as possible and
+ * aligned to a XEN_PADDR_ALIGN boundary.
+ */
+static paddr_t __init get_xen_paddr(uint32_t xen_size)
+{
+ struct meminfo *mi = &bootinfo.mem;
+ paddr_t min_size;
+ paddr_t paddr = 0;
+ int i;
+
+ min_size = (xen_size + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
+
+ /* Find the highest bank with enough space. */
+ for ( i = 0; i < mi->nr_banks; i++ )
+ {
+ const struct membank *bank = &mi->bank[i];
+ paddr_t s, e;
+
+ if ( bank->size >= min_size )
+ {
+ e = consider_modules(bank->start, bank->start + bank->size,
+ min_size, XEN_PADDR_ALIGN, 0);
+ if ( !e )
+ continue;
+
+#ifdef CONFIG_ARM_32
+ /* Xen must be under 4GB */
+ if ( e > 0x100000000ULL )
+ e = 0x100000000ULL;
+ if ( e < bank->start )
+ continue;
+#endif
+
+ s = e - min_size;
+
+ if ( s > paddr )
+ paddr = s;
+ }
+ }
+
+ if ( !paddr )
+ panic("Not enough memory to relocate Xen\n");
+
+ printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+ paddr, paddr + min_size);
+
+ return paddr;
+}
+#else
+static paddr_t __init get_xen_paddr(uint32_t xen_size) { return 0; }
+#endif
+
static void __init init_pdx(void)
{
paddr_t bank_start, bank_size, bank_end;
@@ -1004,8 +1062,6 @@ void __init start_xen(unsigned long boot_phys_offset,
/* Initialize traps early allow us to get backtrace when an error occurred */
init_traps();
- setup_pagetables(boot_phys_offset);
-
smp_clear_cpu_maps();
device_tree_flattened = early_fdt_map(fdt_paddr);
@@ -1031,8 +1087,13 @@ void __init start_xen(unsigned long boot_phys_offset,
{
if ( !llc_coloring_init() )
panic("Xen LLC coloring support: setup failed\n");
+ xen_bootmodule->size = xen_colored_map_size(_end - _start);
+ xen_bootmodule->start = get_xen_paddr(xen_bootmodule->size);
}
+ setup_pagetables(boot_phys_offset, xen_bootmodule->start);
+ device_tree_flattened = early_fdt_map(fdt_paddr);
+
setup_mm();
/* Parse the ACPI tables for possible boot-time configuration */
@@ -1147,6 +1208,14 @@ void __init start_xen(unsigned long boot_phys_offset,
setup_virt_paging();
+ /*
+ * The removal is done earlier than discard_initial_modules beacuse the
+ * livepatch init uses a virtual address equal to BOOT_RELOC_VIRT_START.
+ * Remove LLC coloring mappings to expose a clear state to the livepatch
+ * module.
+ */
+ if ( llc_coloring_enabled )
+ remove_llc_coloring_mappings();
do_initcalls();
/*
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 4a89b3a834..7e437724b4 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -13,6 +13,7 @@
#include <xen/domain_page.h>
#include <xen/errno.h>
#include <xen/init.h>
+#include <xen/llc_coloring.h>
#include <xen/mm.h>
#include <xen/param.h>
#include <xen/sched.h>
@@ -445,6 +446,7 @@ int __cpu_up(unsigned int cpu)
{
int rc;
s_time_t deadline;
+ unsigned long *smp_up_cpu_addr = &smp_up_cpu;
printk("Bringing up CPU%d\n", cpu);
@@ -460,9 +462,12 @@ int __cpu_up(unsigned int cpu)
/* Tell the remote CPU what its logical CPU ID is. */
init_data.cpuid = cpu;
+ if ( llc_coloring_enabled )
+ smp_up_cpu_addr = (unsigned long *)virt_to_reloc_virt(&smp_up_cpu);
+
/* Open the gate for this CPU */
- smp_up_cpu = cpu_logical_map(cpu);
- clean_dcache(smp_up_cpu);
+ *smp_up_cpu_addr = cpu_logical_map(cpu);
+ clean_dcache(*smp_up_cpu_addr);
rc = arch_cpu_up(cpu);
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3f7ebd19f3..a69c43e961 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -212,7 +212,7 @@ SECTIONS
. = ALIGN(POINTER_ALIGN);
__bss_end = .;
} :text
- _end = . ;
+ _end = ALIGN(PAGE_SIZE);
/* Section for the device tree blob (if any). */
.dtb : { *(.dtb) } :text
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 00/11] Arm cache coloring
2023-01-23 15:47 [PATCH v4 00/11] Arm cache coloring Carlo Nonato
` (10 preceding siblings ...)
2023-01-23 15:47 ` [PATCH v4 11/11] xen/arm: add cache coloring support for Xen Carlo Nonato
@ 2023-01-23 15:52 ` Jan Beulich
2023-01-23 16:17 ` Carlo Nonato
11 siblings, 1 reply; 43+ messages in thread
From: Jan Beulich @ 2023-01-23 15:52 UTC (permalink / raw)
To: Carlo Nonato
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Bertrand Marquis, Volodymyr Babchuk, Anthony PERARD,
Juergen Gross, xen-devel
On 23.01.2023 16:47, Carlo Nonato wrote:
> Shared caches in multi-core CPU architectures represent a problem for
> predictability of memory access latency. This jeopardizes applicability
> of many Arm platform in real-time critical and mixed-criticality
> scenarios. We introduce support for cache partitioning with page
> coloring, a transparent software technique that enables isolation
> between domains and Xen, and thus avoids cache interference.
>
> When creating a domain, a simple syntax (e.g. `0-3` or `4-11`) allows
> the user to define assignments of cache partitions ids, called colors,
> where assigning different colors guarantees no mutual eviction on cache
> will ever happen. This instructs the Xen memory allocator to provide
> the i-th color assignee only with pages that maps to color i, i.e. that
> are indexed in the i-th cache partition.
>
> The proposed implementation supports the dom0less feature.
> The proposed implementation doesn't support the static-mem feature.
> The solution has been tested in several scenarios, including Xilinx Zynq
> MPSoCs.
>
> v4 global changes:
> - added "llc" acronym (Last Level Cache) in multiple places in code
> (e.g. coloring.{c|h} -> llc_coloring.{c|h}) to better describe the
Please can you use dashes in favor of underscores in the names of new
files?
Jan
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 00/11] Arm cache coloring
2023-01-23 15:52 ` [PATCH v4 00/11] Arm cache coloring Jan Beulich
@ 2023-01-23 16:17 ` Carlo Nonato
0 siblings, 0 replies; 43+ messages in thread
From: Carlo Nonato @ 2023-01-23 16:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Bertrand Marquis, Volodymyr Babchuk, Anthony PERARD,
Juergen Gross, xen-devel
Hi Jan,
On Mon, Jan 23, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.01.2023 16:47, Carlo Nonato wrote:
> > Shared caches in multi-core CPU architectures represent a problem for
> > predictability of memory access latency. This jeopardizes applicability
> > of many Arm platform in real-time critical and mixed-criticality
> > scenarios. We introduce support for cache partitioning with page
> > coloring, a transparent software technique that enables isolation
> > between domains and Xen, and thus avoids cache interference.
> >
> > When creating a domain, a simple syntax (e.g. `0-3` or `4-11`) allows
> > the user to define assignments of cache partitions ids, called colors,
> > where assigning different colors guarantees no mutual eviction on cache
> > will ever happen. This instructs the Xen memory allocator to provide
> > the i-th color assignee only with pages that maps to color i, i.e. that
> > are indexed in the i-th cache partition.
> >
> > The proposed implementation supports the dom0less feature.
> > The proposed implementation doesn't support the static-mem feature.
> > The solution has been tested in several scenarios, including Xilinx Zynq
> > MPSoCs.
> >
> > v4 global changes:
> > - added "llc" acronym (Last Level Cache) in multiple places in code
> > (e.g. coloring.{c|h} -> llc_coloring.{c|h}) to better describe the
>
> Please can you use dashes in favor of underscores in the names of new
> files?
Yes, ok.
> Jan
I also forgot to mention that this patch series applies on top of the
most recent
version of Julien's series (https://marc.info/?l=xen-devel&m=167360469228247).
Thanks.
^ permalink raw reply [flat|nested] 43+ messages in thread