The check uses >= to compare the total number of colors against
max_num_colors (which is ARRAY_SIZE of the colors array).  This
incorrectly rejects input that would exactly fill the array.

For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
for dom0 would fail.

Change >= to > so that exactly filling the array is permitted.

Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/common/llc-coloring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index eb7c72b24023..30c1594dac9f 100644
--- a/xen/common/llc-coloring.c
+++ b/xen/common/llc-coloring.c
@@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, unsigned int colors[],
 
         if ( end >= NR_LLC_COLORS || start > end ||
              (end - start) >= (UINT_MAX - *num_colors) ||
-             (*num_colors + (end - start + 1)) >= max_num_colors )
+             (*num_colors + (end - start + 1)) > max_num_colors )
             return -EINVAL;
 
         /* Colors are range checked in check_colors() */
-- 
2.43.0

Hi Michal,

> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> The check uses >= to compare the total number of colors against
> max_num_colors (which is ARRAY_SIZE of the colors array).  This
> incorrectly rejects input that would exactly fill the array.
> 
> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
> for dom0 would fail.
> 
> Change >= to > so that exactly filling the array is permitted.
> 
> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

On 09.04.2026 14:22, Luca Fancellu wrote:
>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> The check uses >= to compare the total number of colors against
>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>> incorrectly rejects input that would exactly fill the array.
>>
>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>> for dom0 would fail.
>>
>> Change >= to > so that exactly filling the array is permitted.
>>
>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
think a 2nd Fixes: tag may be needed here. For now I can't help the
impression though that there might have been a re-basing mistake, where
that re-base may have wanted to result in this patch dissolving into
nothing. Yet of course I'm all ears to learn otherwise.

Jan

> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.04.2026 14:22, Luca Fancellu wrote:
>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> The check uses >= to compare the total number of colors against
>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>> incorrectly rejects input that would exactly fill the array.
>>> 
>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>> for dom0 would fail.
>>> 
>>> Change >= to > so that exactly filling the array is permitted.
>>> 
>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>> 
>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
> think a 2nd Fixes: tag may be needed here. For now I can't help the
> impression though that there might have been a re-basing mistake, where
> that re-base may have wanted to result in this patch dissolving into
> nothing. Yet of course I'm all ears to learn otherwise.
> 
> Jan

Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer look.

Cheers,
Luca

Hi Jan,

> On 9 Apr 2026, at 13:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
> 
> 
> 
>> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@suse.com> wrote:
>> 
>> On 09.04.2026 14:22, Luca Fancellu wrote:
>>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>>> 
>>>> The check uses >= to compare the total number of colors against
>>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>>> incorrectly rejects input that would exactly fill the array.
>>>> 
>>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>>> for dom0 would fail.
>>>> 
>>>> Change >= to > so that exactly filling the array is permitted.
>>>> 
>>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>> 
>>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>> 
>> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
>> think a 2nd Fixes: tag may be needed here. For now I can't help the
>> impression though that there might have been a re-basing mistake, where
>> that re-base may have wanted to result in this patch dissolving into
>> nothing. Yet of course I'm all ears to learn otherwise.
>> 
>> Jan
> 
> Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer look.


I had a closer look, I feel that the patch is ok and commit cba8a584de171c8c4510709c2edc9f1cf86b21ab
was missing this corner case.

Let’s say max_num_colors = 8 (array capacity), *num_colors = 4 so we stored already 4 entries and the
next parsed range gives start = 4, end = 7: 

(*num_colors + (end - start + 1)) >= max_num_colors will compute as
(4 + (7 - 4 + 1)) >= 8 which will be
8 >= 8 that will be true and the input will be rejected, instead of being a valid entry.

Did I miss anything?

Cheers,
Luca

On 09.04.2026 15:34, Luca Fancellu wrote:
>> On 9 Apr 2026, at 13:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 09.04.2026 14:22, Luca Fancellu wrote:
>>>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>
>>>>> The check uses >= to compare the total number of colors against
>>>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>>>> incorrectly rejects input that would exactly fill the array.
>>>>>
>>>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>>>> for dom0 would fail.
>>>>>
>>>>> Change >= to > so that exactly filling the array is permitted.
>>>>>
>>>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> ---
>>>>
>>>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>>>
>>> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
>>> think a 2nd Fixes: tag may be needed here. For now I can't help the
>>> impression though that there might have been a re-basing mistake, where
>>> that re-base may have wanted to result in this patch dissolving into
>>> nothing. Yet of course I'm all ears to learn otherwise.
>>
>> Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer look.
> 
> 
> I had a closer look, I feel that the patch is ok and commit cba8a584de171c8c4510709c2edc9f1cf86b21ab
> was missing this corner case.

If anything, that part of the change there was outright wrong (and hence, as
said, a 2nd Fixes: tag [actually, see below, simply another one] is needed).
With overflow excluded,

    (*num_colors + (end - start + 1)) > max_num_colors

is the same as

   (*num_colors + (end - start)) >= max_num_colors

i.e. the state before that change, isn't it?

And yes, now that I look again I think I agree that I screwed up there. Yet
then the (imo) better fix would be to undo that change, rather than switching
from >= to > . That's one less calculation overall. Michal?

Jan

> Let’s say max_num_colors = 8 (array capacity), *num_colors = 4 so we stored already 4 entries and the
> next parsed range gives start = 4, end = 7: 
> 
> (*num_colors + (end - start + 1)) >= max_num_colors will compute as
> (4 + (7 - 4 + 1)) >= 8 which will be
> 8 >= 8 that will be true and the input will be rejected, instead of being a valid entry.
> 
> Did I miss anything?
> 
> Cheers,
> Luca
> 

On 10/04/2026 08:57, Jan Beulich wrote:
> On 09.04.2026 15:34, Luca Fancellu wrote:
>>> On 9 Apr 2026, at 13:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>>> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 09.04.2026 14:22, Luca Fancellu wrote:
>>>>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>
>>>>>> The check uses >= to compare the total number of colors against
>>>>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>>>>> incorrectly rejects input that would exactly fill the array.
>>>>>>
>>>>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>>>>> for dom0 would fail.
>>>>>>
>>>>>> Change >= to > so that exactly filling the array is permitted.
>>>>>>
>>>>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>>>>
>>>> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
>>>> think a 2nd Fixes: tag may be needed here. For now I can't help the
>>>> impression though that there might have been a re-basing mistake, where
>>>> that re-base may have wanted to result in this patch dissolving into
>>>> nothing. Yet of course I'm all ears to learn otherwise.
>>>
>>> Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer look.
>>
>>
>> I had a closer look, I feel that the patch is ok and commit cba8a584de171c8c4510709c2edc9f1cf86b21ab
>> was missing this corner case.
> 
> If anything, that part of the change there was outright wrong (and hence, as
> said, a 2nd Fixes: tag [actually, see below, simply another one] is needed).
> With overflow excluded,
> 
>     (*num_colors + (end - start + 1)) > max_num_colors
> 
> is the same as
> 
>    (*num_colors + (end - start)) >= max_num_colors
> 
> i.e. the state before that change, isn't it?
> 
> And yes, now that I look again I think I agree that I screwed up there. Yet
> then the (imo) better fix would be to undo that change, rather than switching
> from >= to > . That's one less calculation overall. Michal?
Yes, I do agree. This patch can be modified to just do:

diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index eb7c72b24023..6dc614739a98 100644
--- a/xen/common/llc-coloring.c
+++ b/xen/common/llc-coloring.c
@@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, unsigned
int colors[],

         if ( end >= NR_LLC_COLORS || start > end ||
              (end - start) >= (UINT_MAX - *num_colors) ||
-             (*num_colors + (end - start + 1)) >= max_num_colors )
+             (*num_colors + (end - start)) >= max_num_colors )
             return -EINVAL;

         /* Colors are range checked in check_colors() */

I'll do that later on.

~Michal

> On 10 Apr 2026, at 08:04, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 10/04/2026 08:57, Jan Beulich wrote:
>> On 09.04.2026 15:34, Luca Fancellu wrote:
>>>> On 9 Apr 2026, at 13:52, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>>>> On 9 Apr 2026, at 13:48, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 09.04.2026 14:22, Luca Fancellu wrote:
>>>>>>> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>> 
>>>>>>> The check uses >= to compare the total number of colors against
>>>>>>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>>>>>>> incorrectly rejects input that would exactly fill the array.
>>>>>>> 
>>>>>>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>>>>>>> for dom0 would fail.
>>>>>>> 
>>>>>>> Change >= to > so that exactly filling the array is permitted.
>>>>>>> 
>>>>>>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>>> ---
>>>>>> 
>>>>>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
>>>>> 
>>>>> Did you see Andrew's reply? If that earlier (recent) commit was wrong, I
>>>>> think a 2nd Fixes: tag may be needed here. For now I can't help the
>>>>> impression though that there might have been a re-basing mistake, where
>>>>> that re-base may have wanted to result in this patch dissolving into
>>>>> nothing. Yet of course I'm all ears to learn otherwise.
>>>> 
>>>> Oh, no I didn’t see that! Thanks for pointing that out, I will have a closer look.
>>> 
>>> 
>>> I had a closer look, I feel that the patch is ok and commit cba8a584de171c8c4510709c2edc9f1cf86b21ab
>>> was missing this corner case.
>> 
>> If anything, that part of the change there was outright wrong (and hence, as
>> said, a 2nd Fixes: tag [actually, see below, simply another one] is needed).
>> With overflow excluded,
>> 
>>    (*num_colors + (end - start + 1)) > max_num_colors
>> 
>> is the same as
>> 
>>   (*num_colors + (end - start)) >= max_num_colors
>> 
>> i.e. the state before that change, isn't it?
>> 
>> And yes, now that I look again I think I agree that I screwed up there. Yet
>> then the (imo) better fix would be to undo that change, rather than switching
>> from >= to > . That's one less calculation overall. Michal?
> Yes, I do agree. This patch can be modified to just do:
> 
> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> index eb7c72b24023..6dc614739a98 100644
> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, unsigned
> int colors[],
> 
>         if ( end >= NR_LLC_COLORS || start > end ||
>              (end - start) >= (UINT_MAX - *num_colors) ||
> -             (*num_colors + (end - start + 1)) >= max_num_colors )
> +             (*num_colors + (end - start)) >= max_num_colors )
>             return -EINVAL;
> 
>         /* Colors are range checked in check_colors() */
> 
> I'll do that later on.
> 
> ~Michal
> 

Feel free to keep my R-by for this change.

Cheers,
Luca

On 09/04/2026 12:39, Michal Orzel wrote:
> The check uses >= to compare the total number of colors against
> max_num_colors (which is ARRAY_SIZE of the colors array).  This
> incorrectly rejects input that would exactly fill the array.
This seems related to BVA as well.
>
> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
> for dom0 would fail.

Why does it fail ? Is it because the max number of colors can be 15.

- Ayan

On 09/04/2026 12:39 pm, Michal Orzel wrote:
> The check uses >= to compare the total number of colors against
> max_num_colors (which is ARRAY_SIZE of the colors array).  This
> incorrectly rejects input that would exactly fill the array.
>
> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
> for dom0 would fail.
>
> Change >= to > so that exactly filling the array is permitted.
>
> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  xen/common/llc-coloring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> index eb7c72b24023..30c1594dac9f 100644
> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, unsigned int colors[],
>  
>          if ( end >= NR_LLC_COLORS || start > end ||
>               (end - start) >= (UINT_MAX - *num_colors) ||
> -             (*num_colors + (end - start + 1)) >= max_num_colors )
> +             (*num_colors + (end - start + 1)) > max_num_colors )
>              return -EINVAL;
>  
>          /* Colors are range checked in check_colors() */

This boundary was changed by
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cba8a584de171c8c4510709c2edc9f1cf86b21ab
because it was off-by-one.

Are you saying that the analysis in that patch was wrong?

~Andrew

On 09/04/2026 13:47, Andrew Cooper wrote:
> On 09/04/2026 12:39 pm, Michal Orzel wrote:
>> The check uses >= to compare the total number of colors against
>> max_num_colors (which is ARRAY_SIZE of the colors array).  This
>> incorrectly rejects input that would exactly fill the array.
>>
>> For example, with NR_LLC_COLORS=16, specifying 1 color for Xen and 15
>> for dom0 would fail.
>>
>> Change >= to > so that exactly filling the array is permitted.
>>
>> Fixes: 95ef5ddf8a ("xen/arm: add Dom0 cache coloring support")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  xen/common/llc-coloring.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
>> index eb7c72b24023..30c1594dac9f 100644
>> --- a/xen/common/llc-coloring.c
>> +++ b/xen/common/llc-coloring.c
>> @@ -78,7 +78,7 @@ static int __init parse_color_config(const char *buf, unsigned int colors[],
>>  
>>          if ( end >= NR_LLC_COLORS || start > end ||
>>               (end - start) >= (UINT_MAX - *num_colors) ||
>> -             (*num_colors + (end - start + 1)) >= max_num_colors )
>> +             (*num_colors + (end - start + 1)) > max_num_colors )
>>              return -EINVAL;
>>  
>>          /* Colors are range checked in check_colors() */
> 
> This boundary was changed by
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cba8a584de171c8c4510709c2edc9f1cf86b21ab
> because it was off-by-one.
> 
> Are you saying that the analysis in that patch was wrong?
I examined the scenario that is a default for dom0 i.e. dom0 gets all the colors
by default. This is equivalent to setting dom0-llc-colors=0-15. If I set this, I
will get a message:
(XEN) parameter "dom0-llc-colors" has invalid value "0-15", rc=-22!

I admit that I added wrong example in commit msg.

~Michal

> 
> ~Andrew