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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

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.

> Jan