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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-04-29 08:14, 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.
There is a guarantee by the MISRA committee to never reuse rule ids, and 
the SAF mechanism offers a centralized place where the actual rule ID 
for each tool is specified, so I don't foresee problems in that regard. 
In practical terms, I think it is very unlikely that this rule will 
change in any way (e.g. it is identical up until MISRA C:2025, published 
in March).
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253