| Message ID | 20240102095138.17933-5-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
Hi Carlo, 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 <lucmiccio@gmail.com> > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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. > + break; > + > default: > ret = arch_do_domctl(op, d, u_domctl); > break; > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index a33f9ec32b..2b12069294 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 0x00000016 > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017 > > /* > * NB. xen_domctl.domain is an IN/OUT parameter for this operation. > @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op { > typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > > +struct xen_domctl_set_llc_colors { > + /* IN LLC coloring parameters */ > + unsigned int num_llc_colors; I think there will be a padding here. So can you make it explicit? > + XEN_GUEST_HANDLE_64(uint) llc_colors; > +}; > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -1277,6 +1283,7 @@ struct xen_domctl { > #define XEN_DOMCTL_vmtrace_op 84 > #define XEN_DOMCTL_get_paging_mempool_size 85 > #define XEN_DOMCTL_set_paging_mempool_size 86 > +#define XEN_DOMCTL_set_llc_colors 87 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1339,6 +1346,7 @@ struct xen_domctl { > struct xen_domctl_vuart_op vuart_op; > struct xen_domctl_vmtrace_op vmtrace_op; > struct xen_domctl_paging_mempool paging_mempool; > + struct xen_domctl_set_llc_colors set_llc_colors; > uint8_t pad[128]; > } u; > }; > diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h > index cedd97d4b5..fa2edc8ad8 100644 > --- a/xen/include/xen/llc-coloring.h > +++ b/xen/include/xen/llc-coloring.h > @@ -33,6 +33,9 @@ extern bool llc_coloring_enabled; > void domain_llc_coloring_free(struct domain *d); > void domain_dump_llc_colors(struct domain *d); > > +int domain_set_llc_colors_domctl(struct domain *d, > + const struct xen_domctl_set_llc_colors *config); > + > #endif /* __COLORING_H__ */ > > /* Cheers,
On 02.01.2024 10:51, Carlo Nonato wrote: > --- a/xen/arch/arm/llc-coloring.c > +++ b/xen/arch/arm/llc-coloring.c > @@ -9,6 +9,7 @@ > * Carlo Nonato <carlo.nonato@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) ) > + return -ENOMEM; > + > + if ( copy_from_guest(d->llc_colors, config->llc_colors, > + config->num_llc_colors) ) > + return -EFAULT; > + > + return domain_check_colors(d); > +} What part of this is Arm-specific? I ask in particular because while you place this in an Arm-specific source file, ... > --- 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"); > + break; > + > default: > ret = arch_do_domctl(op, d, u_domctl); > break; ... you don't handle the new domctl in Arm's arch_do_domctl(). > --- 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 0x00000016 > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017 There's no need for such a bump when ... > @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op { > typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > > +struct xen_domctl_set_llc_colors { > + /* IN LLC coloring parameters */ > + unsigned int num_llc_colors; > + XEN_GUEST_HANDLE_64(uint) llc_colors; > +}; > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -1277,6 +1283,7 @@ struct xen_domctl { > #define XEN_DOMCTL_vmtrace_op 84 > #define XEN_DOMCTL_get_paging_mempool_size 85 > #define XEN_DOMCTL_set_paging_mempool_size 86 > +#define XEN_DOMCTL_set_llc_colors 87 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1339,6 +1346,7 @@ struct xen_domctl { > struct xen_domctl_vuart_op vuart_op; > struct xen_domctl_vmtrace_op vmtrace_op; > struct xen_domctl_paging_mempool paging_mempool; > + struct xen_domctl_set_llc_colors set_llc_colors; > uint8_t pad[128]; > } u; > }; ... all you do is add a new domctl. As to the new struct - you'll want to use uint<N>_t there, not unsigned int. Jan
Hi Julien, On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@xen.org> wrote: > > Hi Carlo, > > 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 <lucmiccio@gmail.com> > > > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > > Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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. I'm ok with printing nothing, but -EEXIST to me seems logical. > > + break; > > + > > default: > > ret = arch_do_domctl(op, d, u_domctl); > > break; > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > > index a33f9ec32b..2b12069294 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 0x00000016 > > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017 > > > > /* > > * NB. xen_domctl.domain is an IN/OUT parameter for this operation. > > @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op { > > typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > > DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > > > > +struct xen_domctl_set_llc_colors { > > + /* IN LLC coloring parameters */ > > + unsigned int num_llc_colors; > > I think there will be a padding here. So can you make it explicit? > > > + XEN_GUEST_HANDLE_64(uint) llc_colors; > > +}; > > + > > struct xen_domctl { > > uint32_t cmd; > > #define XEN_DOMCTL_createdomain 1 > > @@ -1277,6 +1283,7 @@ struct xen_domctl { > > #define XEN_DOMCTL_vmtrace_op 84 > > #define XEN_DOMCTL_get_paging_mempool_size 85 > > #define XEN_DOMCTL_set_paging_mempool_size 86 > > +#define XEN_DOMCTL_set_llc_colors 87 > > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > > @@ -1339,6 +1346,7 @@ struct xen_domctl { > > struct xen_domctl_vuart_op vuart_op; > > struct xen_domctl_vmtrace_op vmtrace_op; > > struct xen_domctl_paging_mempool paging_mempool; > > + struct xen_domctl_set_llc_colors set_llc_colors; > > uint8_t pad[128]; > > } u; > > }; > > diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h > > index cedd97d4b5..fa2edc8ad8 100644 > > --- a/xen/include/xen/llc-coloring.h > > +++ b/xen/include/xen/llc-coloring.h > > @@ -33,6 +33,9 @@ extern bool llc_coloring_enabled; > > void domain_llc_coloring_free(struct domain *d); > > void domain_dump_llc_colors(struct domain *d); > > > > +int domain_set_llc_colors_domctl(struct domain *d, > > + const struct xen_domctl_set_llc_colors *config); > > + > > #endif /* __COLORING_H__ */ > > > > /* > > Cheers, > > -- > Julien Grall Thanks.
Hi Carlo, On 08/01/2024 10:27, Carlo Nonato wrote: > On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@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 <lucmiccio@gmail.com> >>> >>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> >>> Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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,
Hi Julien, On Mon, Jan 8, 2024 at 12:01 PM Julien Grall <julien@xen.org> wrote: > > Hi Carlo, > > On 08/01/2024 10:27, Carlo Nonato wrote: > > On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@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 <lucmiccio@gmail.com> > >>> > >>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > >>> Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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. So you want to be able to change colors if no memory has yet been allocated? I don't know what to check that. > Cheers, > > -- > Julien Grall
Hi Jan, On Mon, Jan 8, 2024 at 9:43 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 02.01.2024 10:51, Carlo Nonato wrote: > > --- a/xen/arch/arm/llc-coloring.c > > +++ b/xen/arch/arm/llc-coloring.c > > @@ -9,6 +9,7 @@ > > * Carlo Nonato <carlo.nonato@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) ) > > + return -ENOMEM; > > + > > + if ( copy_from_guest(d->llc_colors, config->llc_colors, > > + config->num_llc_colors) ) > > + return -EFAULT; > > + > > + return domain_check_colors(d); > > +} > > What part of this is Arm-specific? I ask in particular because while you > place this in an Arm-specific source file, ... > > > --- 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"); > > + break; > > + > > default: > > ret = arch_do_domctl(op, d, u_domctl); > > break; > > ... you don't handle the new domctl in Arm's arch_do_domctl(). No arm specific code here. I need a new xen/common/llc-coloring.c file where to put common stuff. > > --- 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 0x00000016 > > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017 > > There's no need for such a bump when ... > > > @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op { > > typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > > DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > > > > +struct xen_domctl_set_llc_colors { > > + /* IN LLC coloring parameters */ > > + unsigned int num_llc_colors; > > + XEN_GUEST_HANDLE_64(uint) llc_colors; > > +}; > > + > > struct xen_domctl { > > uint32_t cmd; > > #define XEN_DOMCTL_createdomain 1 > > @@ -1277,6 +1283,7 @@ struct xen_domctl { > > #define XEN_DOMCTL_vmtrace_op 84 > > #define XEN_DOMCTL_get_paging_mempool_size 85 > > #define XEN_DOMCTL_set_paging_mempool_size 86 > > +#define XEN_DOMCTL_set_llc_colors 87 > > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > > @@ -1339,6 +1346,7 @@ struct xen_domctl { > > struct xen_domctl_vuart_op vuart_op; > > struct xen_domctl_vmtrace_op vmtrace_op; > > struct xen_domctl_paging_mempool paging_mempool; > > + struct xen_domctl_set_llc_colors set_llc_colors; > > uint8_t pad[128]; > > } u; > > }; > > ... all you do is add a new domctl. > > As to the new struct - you'll want to use uint<N>_t there, not > unsigned int. > > Jan Thanks.
Hi Carlo, On 08/01/2024 11:19, Carlo Nonato wrote: > Hi Julien, > > On Mon, Jan 8, 2024 at 12:01 PM Julien Grall <julien@xen.org> wrote: >> >> Hi Carlo, >> >> On 08/01/2024 10:27, Carlo Nonato wrote: >>> On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@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 <lucmiccio@gmail.com> >>>>> >>>>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> >>>>> Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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. > > So you want to be able to change colors if no memory has yet been allocated? No. I am saying that that we should not be able to allow changing the colors after the memory has been allocated. To give an example, your current code would allow: 1) Setup the P2M pools or allocate RAM 2) Set the color This would render the coloring configuration moot. Whether we want to allow changing the coloring before hand is a different question and as I wrote earlier on, I don't mind if you want to forbid that. > I don't know what to check that. You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) is still 0. I think for RAM, you can check d->tot_pages == 0. Cheers,
Hi Julien On Mon, Jan 8, 2024 at 12:36 PM Julien Grall <julien@xen.org> wrote: > > Hi Carlo, > > On 08/01/2024 11:19, Carlo Nonato wrote: > > Hi Julien, > > > > On Mon, Jan 8, 2024 at 12:01 PM Julien Grall <julien@xen.org> wrote: > >> > >> Hi Carlo, > >> > >> On 08/01/2024 10:27, Carlo Nonato wrote: > >>> On Fri, Jan 5, 2024 at 6:26 PM Julien Grall <julien@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 <lucmiccio@gmail.com> > >>>>> > >>>>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > >>>>> Signed-off-by: Marco Solieri <marco.solieri@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.nonato@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. > > > > So you want to be able to change colors if no memory has yet been allocated? > > No. I am saying that that we should not be able to allow changing the > colors after the memory has been allocated. To give an example, your > current code would allow: > > 1) Setup the P2M pools or allocate RAM > 2) Set the color > > This would render the coloring configuration moot. > > Whether we want to allow changing the coloring before hand is a > different question and as I wrote earlier on, I don't mind if you want > to forbid that. At the moment I'm relying on the toolstack in the sense that I know that it will set colors right after domain creation and before memory allocation. Calling alloc_domheap_pages() without a coloring configuration makes Xen crash, so it's mandatory to have the configuration done before any allocation. I know that we shouldn't rely on the toolstack this much, but I didn't find a better way. Given this assumption, looking for an already existing color configuration of a domain is sufficient to avoid what you are saying. Is it possible to enforce such a constraint with domctl? I mean to be sure that this domctl will be called at a precise time. Thanks. > > I don't know what to check that. > > You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > is still 0. I think for RAM, you can check d->tot_pages == 0. > > Cheers, > > -- > Julien Grall
Hi, On 08/01/2024 15:18, Carlo Nonato wrote: >> No. I am saying that that we should not be able to allow changing the >> colors after the memory has been allocated. To give an example, your >> current code would allow: >> >> 1) Setup the P2M pools or allocate RAM >> 2) Set the color >> >> This would render the coloring configuration moot. >> >> Whether we want to allow changing the coloring before hand is a >> different question and as I wrote earlier on, I don't mind if you want >> to forbid that. > > At the moment I'm relying on the toolstack in the sense that I know that it > will set colors right after domain creation and before memory allocation. > Calling alloc_domheap_pages() without a coloring configuration makes Xen > crash, so it's mandatory to have the configuration done before any allocation. > I know that we shouldn't rely on the toolstack this much, but I didn't > find a better way. Given this assumption, looking for an already existing > color configuration of a domain is sufficient to avoid what you are saying. > > Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. Yes. You can... > > Thanks. > >>> I don't know what to check that. >> >> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) >> is still 0. I think for RAM, you can check d->tot_pages == 0. ... reject the call if either of the two fields are not zero. Cheers,
Hi Julien, On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: > > Hi, > > On 08/01/2024 15:18, Carlo Nonato wrote: > >> No. I am saying that that we should not be able to allow changing the > >> colors after the memory has been allocated. To give an example, your > >> current code would allow: > >> > >> 1) Setup the P2M pools or allocate RAM > >> 2) Set the color > >> > >> This would render the coloring configuration moot. > >> > >> Whether we want to allow changing the coloring before hand is a > >> different question and as I wrote earlier on, I don't mind if you want > >> to forbid that. > > > > At the moment I'm relying on the toolstack in the sense that I know that it > > will set colors right after domain creation and before memory allocation. > > Calling alloc_domheap_pages() without a coloring configuration makes Xen > > crash, so it's mandatory to have the configuration done before any allocation. > > I know that we shouldn't rely on the toolstack this much, but I didn't > > find a better way. Given this assumption, looking for an already existing > > color configuration of a domain is sufficient to avoid what you are saying. > > > > Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. > > Yes. You can... > > > > > Thanks. > > > >>> I don't know what to check that. > >> > >> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > >> is still 0. I think for RAM, you can check d->tot_pages == 0. > > ... reject the call if either of the two fields are not zero. What I'm saying is that Xen would crash before even reaching this point if no colors were provided. Let's say that the toolstack or whatever hypercall user isn't calling this domctl at all (or not at the right time), then the domain colored allocator would always return null pages since there are no colors. We would have a crash and your if (or mine) would be useless. Let's say that now the domctl is called at the right time (no p2m, no tot_pages, no colors) then we can set the colors and everything works. From this point other calls to this domctl would be skipped because of your if which is equivalent to mine (checking for colors existence). Also bringing in checks on p2m would require arch specific code which I was trying to avoid. Thanks. > Cheers, > > -- > Julien Grall
On 08.01.2024 17:31, Carlo Nonato wrote: > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: >> On 08/01/2024 15:18, Carlo Nonato wrote: >>>> No. I am saying that that we should not be able to allow changing the >>>> colors after the memory has been allocated. To give an example, your >>>> current code would allow: >>>> >>>> 1) Setup the P2M pools or allocate RAM >>>> 2) Set the color >>>> >>>> This would render the coloring configuration moot. >>>> >>>> Whether we want to allow changing the coloring before hand is a >>>> different question and as I wrote earlier on, I don't mind if you want >>>> to forbid that. >>> >>> At the moment I'm relying on the toolstack in the sense that I know that it >>> will set colors right after domain creation and before memory allocation. >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen >>> crash, so it's mandatory to have the configuration done before any allocation. >>> I know that we shouldn't rely on the toolstack this much, but I didn't >>> find a better way. Given this assumption, looking for an already existing >>> color configuration of a domain is sufficient to avoid what you are saying. >>> >>> Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. >> >> Yes. You can... >> >>> >>> Thanks. >>> >>>>> I don't know what to check that. >>>> >>>> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) >>>> is still 0. I think for RAM, you can check d->tot_pages == 0. >> >> ... reject the call if either of the two fields are not zero. > > What I'm saying is that Xen would crash before even reaching this point if no > colors were provided. Let's say that the toolstack or whatever hypercall user > isn't calling this domctl at all (or not at the right time), then the domain > colored allocator would always return null pages since there are no colors. > We would have a crash and your if (or mine) would be useless. Why is it that you can't simply allocated arbitrary memory if coloring information wasn't set up front? Aiui that'll be required anyway, as there shouldn't be a restriction that all domains have to use coloring. Jan
Hi Carlo, On 08/01/2024 16:31, Carlo Nonato wrote: > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 08/01/2024 15:18, Carlo Nonato wrote: >>>> No. I am saying that that we should not be able to allow changing the >>>> colors after the memory has been allocated. To give an example, your >>>> current code would allow: >>>> >>>> 1) Setup the P2M pools or allocate RAM >>>> 2) Set the color >>>> >>>> This would render the coloring configuration moot. >>>> >>>> Whether we want to allow changing the coloring before hand is a >>>> different question and as I wrote earlier on, I don't mind if you want >>>> to forbid that. >>> >>> At the moment I'm relying on the toolstack in the sense that I know that it >>> will set colors right after domain creation and before memory allocation. >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen >>> crash, so it's mandatory to have the configuration done before any allocation. >>> I know that we shouldn't rely on the toolstack this much, but I didn't >>> find a better way. Given this assumption, looking for an already existing >>> color configuration of a domain is sufficient to avoid what you are saying. >>> >>> Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. >> >> Yes. You can... >> >>> >>> Thanks. >>> >>>>> I don't know what to check that. >>>> >>>> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) >>>> is still 0. I think for RAM, you can check d->tot_pages == 0. >> >> ... reject the call if either of the two fields are not zero. > > What I'm saying is that Xen would crash before even reaching this point if no > colors were provided. Let's say that the toolstack or whatever hypercall user > isn't calling this domctl at all (or not at the right time), then the domain > colored allocator would always return null pages since there are no colors. > We would have a crash and your if (or mine) would be useless. Really? I can believe that NULL may be returned but a crash... If that's the case, then this should be fixed. > > Let's say that now the domctl is called at the right time (no p2m, > no tot_pages, no colors) then we can set the colors and everything works. > From this point other calls to this domctl would be skipped because of your > if which is equivalent to mine (checking for colors existence). So I misunderstood the implementation of is_domain_llc_colored(). I would have thought it was based on whether the domain has colors. But instead, it is based on whether coloring is enabled globally. So I agree that the checks are not necessary today. But if is_domain_llc_colored() is changed, then this may not be correct anymore. So I think there are some clarifications required. If the 'd' will never matter, then probably is_domain_llc_colored() should be removed. If not, then we want to add the check in the domctl (or at minimum document it). > > Also bringing in checks on p2m would require arch specific code which I was > trying to avoid. Fair enough. But the first step is to make the sure the code is correct and Xen doesn't crash. We can then discuss about avoiding arch specific code. BTW, all your LLC code is implemented in arch/arm. So if it is really intended to contain zero arch specific code, then shouldn't it be implemented in common/? Cheers,
Hi Jan, On Mon, Jan 8, 2024 at 5:40 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 08.01.2024 17:31, Carlo Nonato wrote: > > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: > >> On 08/01/2024 15:18, Carlo Nonato wrote: > >>>> No. I am saying that that we should not be able to allow changing the > >>>> colors after the memory has been allocated. To give an example, your > >>>> current code would allow: > >>>> > >>>> 1) Setup the P2M pools or allocate RAM > >>>> 2) Set the color > >>>> > >>>> This would render the coloring configuration moot. > >>>> > >>>> Whether we want to allow changing the coloring before hand is a > >>>> different question and as I wrote earlier on, I don't mind if you want > >>>> to forbid that. > >>> > >>> At the moment I'm relying on the toolstack in the sense that I know that it > >>> will set colors right after domain creation and before memory allocation. > >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen > >>> crash, so it's mandatory to have the configuration done before any allocation. > >>> I know that we shouldn't rely on the toolstack this much, but I didn't > >>> find a better way. Given this assumption, looking for an already existing > >>> color configuration of a domain is sufficient to avoid what you are saying. > >>> > >>> Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. > >> > >> Yes. You can... > >> > >>> > >>> Thanks. > >>> > >>>>> I don't know what to check that. > >>>> > >>>> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > >>>> is still 0. I think for RAM, you can check d->tot_pages == 0. > >> > >> ... reject the call if either of the two fields are not zero. > > > > What I'm saying is that Xen would crash before even reaching this point if no > > colors were provided. Let's say that the toolstack or whatever hypercall user > > isn't calling this domctl at all (or not at the right time), then the domain > > colored allocator would always return null pages since there are no colors. > > We would have a crash and your if (or mine) would be useless. > > Why is it that you can't simply allocated arbitrary memory if coloring > information wasn't set up front? Aiui that'll be required anyway, as > there shouldn't be a restriction that all domains have to use coloring. If coloring is enabled all domains are colored. It's one of our first assumptions. We haven't developed something that works hybridly and supporting that would require some rework. > Jan
On Mon, 8 Jan 2024, Carlo Nonato wrote: > Hi Jan, > > On Mon, Jan 8, 2024 at 5:40 PM Jan Beulich <jbeulich@suse.com> wrote: > > > > On 08.01.2024 17:31, Carlo Nonato wrote: > > > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: > > >> On 08/01/2024 15:18, Carlo Nonato wrote: > > >>>> No. I am saying that that we should not be able to allow changing the > > >>>> colors after the memory has been allocated. To give an example, your > > >>>> current code would allow: > > >>>> > > >>>> 1) Setup the P2M pools or allocate RAM > > >>>> 2) Set the color > > >>>> > > >>>> This would render the coloring configuration moot. > > >>>> > > >>>> Whether we want to allow changing the coloring before hand is a > > >>>> different question and as I wrote earlier on, I don't mind if you want > > >>>> to forbid that. > > >>> > > >>> At the moment I'm relying on the toolstack in the sense that I know that it > > >>> will set colors right after domain creation and before memory allocation. > > >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen > > >>> crash, so it's mandatory to have the configuration done before any allocation. > > >>> I know that we shouldn't rely on the toolstack this much, but I didn't > > >>> find a better way. Given this assumption, looking for an already existing > > >>> color configuration of a domain is sufficient to avoid what you are saying. > > >>> > > >>> Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. > > >> > > >> Yes. You can... > > >> > > >>> > > >>> Thanks. > > >>> > > >>>>> I don't know what to check that. > > >>>> > > >>>> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > > >>>> is still 0. I think for RAM, you can check d->tot_pages == 0. > > >> > > >> ... reject the call if either of the two fields are not zero. > > > > > > What I'm saying is that Xen would crash before even reaching this point if no > > > colors were provided. Let's say that the toolstack or whatever hypercall user > > > isn't calling this domctl at all (or not at the right time), then the domain > > > colored allocator would always return null pages since there are no colors. > > > We would have a crash and your if (or mine) would be useless. > > > > Why is it that you can't simply allocated arbitrary memory if coloring > > information wasn't set up front? Aiui that'll be required anyway, as > > there shouldn't be a restriction that all domains have to use coloring. > > If coloring is enabled all domains are colored. It's one of our first > assumptions. We haven't developed something that works hybridly and supporting > that would require some rework. I think that's a good assumption and I wouldn't go in the direction of supporting a mix of colored and non-colored. To benefit from cache coloring, all domains need to be colored, otherwise a single non-colored domain could thrash the cache of everyone else.
On 08.01.2024 22:21, Stefano Stabellini wrote: > On Mon, 8 Jan 2024, Carlo Nonato wrote: >> Hi Jan, >> >> On Mon, Jan 8, 2024 at 5:40 PM Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 08.01.2024 17:31, Carlo Nonato wrote: >>>> On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xen.org> wrote: >>>>> On 08/01/2024 15:18, Carlo Nonato wrote: >>>>>>> No. I am saying that that we should not be able to allow changing the >>>>>>> colors after the memory has been allocated. To give an example, your >>>>>>> current code would allow: >>>>>>> >>>>>>> 1) Setup the P2M pools or allocate RAM >>>>>>> 2) Set the color >>>>>>> >>>>>>> This would render the coloring configuration moot. >>>>>>> >>>>>>> Whether we want to allow changing the coloring before hand is a >>>>>>> different question and as I wrote earlier on, I don't mind if you want >>>>>>> to forbid that. >>>>>> >>>>>> At the moment I'm relying on the toolstack in the sense that I know that it >>>>>> will set colors right after domain creation and before memory allocation. >>>>>> Calling alloc_domheap_pages() without a coloring configuration makes Xen >>>>>> crash, so it's mandatory to have the configuration done before any allocation. >>>>>> I know that we shouldn't rely on the toolstack this much, but I didn't >>>>>> find a better way. Given this assumption, looking for an already existing >>>>>> color configuration of a domain is sufficient to avoid what you are saying. >>>>>> >>>>>> Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. >>>>> >>>>> Yes. You can... >>>>> >>>>>> >>>>>> Thanks. >>>>>> >>>>>>>> I don't know what to check that. >>>>>>> >>>>>>> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) >>>>>>> is still 0. I think for RAM, you can check d->tot_pages == 0. >>>>> >>>>> ... reject the call if either of the two fields are not zero. >>>> >>>> What I'm saying is that Xen would crash before even reaching this point if no >>>> colors were provided. Let's say that the toolstack or whatever hypercall user >>>> isn't calling this domctl at all (or not at the right time), then the domain >>>> colored allocator would always return null pages since there are no colors. >>>> We would have a crash and your if (or mine) would be useless. >>> >>> Why is it that you can't simply allocated arbitrary memory if coloring >>> information wasn't set up front? Aiui that'll be required anyway, as >>> there shouldn't be a restriction that all domains have to use coloring. >> >> If coloring is enabled all domains are colored. It's one of our first >> assumptions. We haven't developed something that works hybridly and supporting >> that would require some rework. > > I think that's a good assumption and I wouldn't go in the direction of > supporting a mix of colored and non-colored. To benefit from cache > coloring, all domains need to be colored, otherwise a single non-colored > domain could thrash the cache of everyone else. In which case the tool stack not having set up coloring data first should lead to all allocation attempts failing. No crashes whatsoever. Jan
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.nonato@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) ) + 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"); + break; + default: ret = arch_do_domctl(op, d, u_domctl); break; diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index a33f9ec32b..2b12069294 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 0x00000016 +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017 /* * NB. xen_domctl.domain is an IN/OUT parameter for this operation. @@ -1190,6 +1190,12 @@ struct xen_domctl_vmtrace_op { typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); +struct xen_domctl_set_llc_colors { + /* IN LLC coloring parameters */ + unsigned int num_llc_colors; + XEN_GUEST_HANDLE_64(uint) llc_colors; +}; + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1277,6 +1283,7 @@ struct xen_domctl { #define XEN_DOMCTL_vmtrace_op 84 #define XEN_DOMCTL_get_paging_mempool_size 85 #define XEN_DOMCTL_set_paging_mempool_size 86 +#define XEN_DOMCTL_set_llc_colors 87 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1339,6 +1346,7 @@ struct xen_domctl { struct xen_domctl_vuart_op vuart_op; struct xen_domctl_vmtrace_op vmtrace_op; struct xen_domctl_paging_mempool paging_mempool; + struct xen_domctl_set_llc_colors set_llc_colors; uint8_t pad[128]; } u; }; diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h index cedd97d4b5..fa2edc8ad8 100644 --- a/xen/include/xen/llc-coloring.h +++ b/xen/include/xen/llc-coloring.h @@ -33,6 +33,9 @@ extern bool llc_coloring_enabled; void domain_llc_coloring_free(struct domain *d); void domain_dump_llc_colors(struct domain *d); +int domain_set_llc_colors_domctl(struct domain *d, + const struct xen_domctl_set_llc_colors *config); + #endif /* __COLORING_H__ */ /*