Re: [PATCH v4 04/11] xen: extend domctl interface for cache coloring

[Julien Grall <julien@xxxxxxx>]

Hi,

On 25/01/2023 16:27, Carlo Nonato wrote:
> On Tue, Jan 24, 2023 at 5:29 PM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxx/
> > Jan
--
Julien Grall