Using:

  xfree(__va(__pa(d->llc_colors)));

is an extraordinarily expensive way of writing:

  xfree((void *)d->llc_colours);

Combined with the comment indicating that this was intention, the patch should
have been rejected outright.

Correct the type of d->llc_colours by removing the bogus const on it, and
remove the cast when freeing it.

No functional change.

Fixes: 6985aa5e0c3c ("xen: extend domctl interface for cache coloring")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Carlo Nonato <carlo.nonato@minervasys.tech>
CC: Marco Solieri <marco.solieri@minervasys.tech>
---
 xen/common/llc-coloring.c | 7 ++-----
 xen/include/xen/sched.h   | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index a572f77a098a..bd1f634f0bb8 100644
--- 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. */
-- 
2.39.5

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(). Which you keep objecting to for
reasons I sadly cannot follow.

Jan

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@kernel.org>