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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien and Jan,

On Thu, Jan 26, 2023 at 11:21 AM Julien Grall <julien@xxxxxxx> wrote:
>
> 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/

Really nice. Thanks to both.

> >
> >> Jan
>
> --
> Julien Grall