Re: [PATCH v1 1/2] xen/page_alloc: address violation of Rule 14.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2025 01:21, Stefano Stabellini wrote:
> On Mon, 28 Apr 2025, Jan Beulich wrote:
>> On 26.04.2025 02:00, victorm.lira@xxxxxxx wrote:
>>> From: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>
>>> MISRA C Rule 14.3 states that "Controlling expressions shall not be
>>> invariant".
>>>
>>> Add a SAF comment to deviate the rule for build configurations without
>>> CONFIG_LLC_COLORING enabled.
>>
>> I was surprised by this supposedly being the only violation. And indeed it
>> wasn't very hard to find more. For example, we have a number of
>> "while ( num_online_cpus() > 1 && ... )", which become compile-time
>> constant (false) when NR_CPUS=1.
> 
> Uhm, I did run a special scan for this and I can confirm no other
> violations are detected.

Because of it being only one single configuration that's being scanned. I did
point out before that this is a problem for anyone wanting to certify the
hypervisor in a (perhaps just slightly) different configuration.

>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2038,6 +2038,7 @@ static struct page_info 
>>> *alloc_color_heap_page(unsigned int memflags,
>>>
>>>      spin_lock(&heap_lock);
>>>
>>> +    /* SAF-14-safe MISRA C R14.3 condition always false without 
>>> LLC_COLORING */
>>>      for ( i = 0; i < domain_num_llc_colors(d); i++ )
>>>      {
>>>          unsigned long free = free_colored_pages[domain_llc_color(d, i)];
>>
>> Hmm, this way the deviation applies even when LLC_COLORING=y.
> 
> Yes but in the LLC_COLORING=y case it is harmless. Do you have something
> else in mind?

What if, perhaps by mistake, domain_num_llc_colors() becomes constant 0 in
yet another configuration? (I don't expect this would work, but in principle
the comment ought to be inside an #ifdef.)

>> As to the comment wording - looks like we're pretty inconsistent with that
>> right now. I, for one, don't think the Misra rule needs (re)stating there;
>> the SAF index points at all the data that's needed if one cares about the
>> specifics of the deviation.
> 
> Do you prefer:
> 
> /* SAF-14-safe */

That's too short. All I'm asking for is to drop the (imprecise) rule
reference. Noticing only now: It being imprecise may make the comment go
stale if we move to a newer Misra spec, as the rule number may be different
then.

Jan