Hi Carlo,
On 08/01/2024 10:27, Carlo Nonato wrote:
On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <jul...@xen.org> wrote:
On 02/01/2024 09:51, Carlo Nonato wrote:
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 <lucmic...@gmail.com>
Signed-off-by: Carlo Nonato <carlo.non...@minervasys.tech>
Signed-off-by: Marco Solieri <marco.soli...@minervasys.tech>
---
v5:
- added a new hypercall to set colors
- uint for the guest handle
v4:
- updated XEN_DOMCTL_INTERFACE_VERSION
---
xen/arch/arm/llc-coloring.c | 17 +++++++++++++++++
xen/common/domctl.c | 11 +++++++++++
xen/include/public/domctl.h | 10 +++++++++-
xen/include/xen/llc-coloring.h | 3 +++
4 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
index 5ce58aba70..a08614ec36 100644
--- a/xen/arch/arm/llc-coloring.c
+++ b/xen/arch/arm/llc-coloring.c
@@ -9,6 +9,7 @@
* Carlo Nonato <carlo.non...@minervasys.tech>
*/
#include <xen/errno.h>
+#include <xen/guest_access.h>
#include <xen/keyhandler.h>
#include <xen/llc-coloring.h>
#include <xen/param.h>
@@ -278,6 +279,22 @@ int dom0_set_llc_colors(struct domain *d)
return domain_check_colors(d);
}
+int domain_set_llc_colors_domctl(struct domain *d,
+ const struct xen_domctl_set_llc_colors
*config)
+{
+ if ( d->num_llc_colors )
+ return -EEXIST;
+
+ if ( domain_alloc_colors(d, config->num_llc_colors) )
domain_alloc_colors() doesn't sanity check config->num_llc_colors before
allocating the array. You want a check the size before so we would not
try to allocate an arbitrary amount of memory.
+ return -ENOMEM;
+
+ if ( copy_from_guest(d->llc_colors, config->llc_colors,
+ config->num_llc_colors) )
+ return -EFAULT;
+
+ return domain_check_colors(d);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f7..b6867d0602 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>
@@ -858,6 +859,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
__HYPERVISOR_domctl, "h", u_domctl);
break;
+ case XEN_DOMCTL_set_llc_colors:
+ if ( !llc_coloring_enabled )
+ break;
+
+ ret = domain_set_llc_colors_domctl(d, &op->u.set_llc_colors);
+ if ( ret == -EEXIST )
+ printk(XENLOG_ERR
+ "Can't set LLC colors on an already created domain\n");
To me, the message doesn't match the check in
domain_set_llc_colors_domctl(). But I think you want to check that no
memory was yet allocated to the domain. Otherwise, you coloring will be
wrong.
Also, it is a bit unclear why you print a message for -EEXIST but not
the others. In this instance, I would consider to print nothing at all.
The problem here is that we don't support recoloring. When a domain is
created it receives a coloring configuration and it can't change. If this
hypercall is called twice I have to stop the second time somehow.
Looking at your check what you prevent is a toolstack updating the array
twice. But that would be ok (/!\ I am not saying we should allow it) so
long no memory has been allocated to the domain.
But I also consider we would re-color once we started to allocate memory
for the domain (either RAM or P2M). This seems to be missed out in your
check.
Cheers,
--
Julien Grall
