Re: [PATCH 1/4] xen/cache-col: Remove bogus cast in domain_llc_coloring_free()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 25 Jul 2025, Jan Beulich wrote:
> On 24.07.2025 18:23, Andrew Cooper wrote:
> > --- a/xen/common/llc-coloring.c
> > +++ b/xen/common/llc-coloring.c
> > @@ -309,11 +309,8 @@ int domain_set_llc_colors(struct domain *d,
> >  
> >  void domain_llc_coloring_free(struct domain *d)
> >  {
> > -    if ( !llc_coloring_enabled || d->llc_colors == default_colors )
> > -        return;
> > -
> > -    /* free pointer-to-const using __va(__pa()) */
> > -    xfree(__va(__pa(d->llc_colors)));
> > +    if ( d->llc_colors != default_colors )
> > +        xfree(d->llc_colors);
> >  }
> >  
> >  int __init domain_set_llc_colors_from_str(struct domain *d, const char 
> > *str)
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index fe53d4fab7ba..df23411869e6 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -649,7 +649,7 @@ struct domain
> >  
> >  #ifdef CONFIG_LLC_COLORING
> >      unsigned int num_llc_colors;
> > -    const unsigned int *llc_colors;
> > +    unsigned int *llc_colors;
> >  #endif
> >  
> >      /* Console settings. */
> 
> Ah yes, I see. Yet no, I don't agree. The only sane course of action
> to avoid odd transformations like the above (without using casts to
> cast away const-ness) is to finally make xfree() et al take pointers
> to const void. Arguments towards why this makes sense were given
> before; I don't think they need repeating. Dropping the const here is
> rather undesirable: Once set, the colors shouldn't be altered anymore.
> Pointers like this hence want to be pointer-to-const, to make
> accidental modification less likely. Which in turn calls for the
> mentioned adjustment to xfree().

I agree that "Once set, the colors shouldn't be altered anymore.
Pointers like this hence want to be pointer-to-const, to make accidental
modification less likely".

However, I also think that the following is worse than risking
accidental unwanted modifications of llc_colors:

    /* free pointer-to-const using __va(__pa()) */
    xfree(__va(__pa(d->llc_colors)));

So in my opinion this patch is good. If/when xfree becomes able to deal
with const pointers, then we can change llc_colors to be const again.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>