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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 29 Apr 2025, Jan Beulich wrote:
> 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.

I read all your comments to this patch and suggestions, while some of
them are good, I think this patch is better left as it is.

So I'll give my Reviewed-by. If you disagree it should go in as is, I'd
ask a third Maintainer to voice their opinion.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>