Re: [XEN PATCH v2] xen: add explicit comment to identify notifier patterns

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Jun 2024, Federico Serafini wrote:
> On 21/06/24 03:13, Stefano Stabellini wrote:
> > On Thu, 20 Jun 2024, Federico Serafini wrote:
> > > On 19/06/24 13:17, Julien Grall wrote:
> > > > Hi Federico,
> > > > 
> > > > On 19/06/2024 10:29, Federico Serafini wrote:
> > > > > MISRA C Rule 16.4 states that every `switch' statement shall have a
> > > > > `default' label" and a statement or a comment prior to the
> > > > > terminating break statement.
> > > > > 
> > > > > This patch addresses some violations of the rule related to the
> > > > > "notifier pattern": a frequently-used pattern whereby only a few
> > > > > values
> > > > > are handled by the switch statement and nothing should be done for
> > > > > others (nothing to do in the default case).
> > > > > 
> > > > > Note that for function mwait_idle_cpu_init() in
> > > > > xen/arch/x86/cpu/mwait-idle.c the /* Notifier pattern. */ comment is
> > > > > not added: differently from the other functions covered in this patch,
> > > > > the default label has a return statement that does not violates Rule
> > > > > 16.4.
> > > > > 
> > > > > No functional change.
> > > > > 
> > > > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > > > ---
> > > > > Changes in v2:
> > > > > as Jan pointed out, in the v1 some patterns were not explicitly
> > > > > identified
> > > > > (https://lore.kernel.org/xen-devel/cad05a5c-e2d8-4e5d-af05-30ae6f959184@xxxxxxxxxxx/).
> > > > > 
> > > > > This version adds the /* Notifier pattern. */ to all the pattern
> > > > > present
> > > > > in
> > > > > the Xen codebase except for mwait_idle_cpu_init().
> > > > > ---
> > > > >    xen/arch/arm/cpuerrata.c                     | 1 +
> > > > >    xen/arch/arm/gic-v3-lpi.c                    | 4 ++++
> > > > >    xen/arch/arm/gic.c                           | 1 +
> > > > >    xen/arch/arm/irq.c                           | 4 ++++
> > > > >    xen/arch/arm/mmu/p2m.c                       | 1 +
> > > > >    xen/arch/arm/percpu.c                        | 1 +
> > > > >    xen/arch/arm/smpboot.c                       | 1 +
> > > > >    xen/arch/arm/time.c                          | 1 +
> > > > >    xen/arch/arm/vgic-v3-its.c                   | 2 ++
> > > > >    xen/arch/x86/acpi/cpu_idle.c                 | 4 ++++
> > > > >    xen/arch/x86/cpu/mcheck/mce.c                | 4 ++++
> > > > >    xen/arch/x86/cpu/mcheck/mce_intel.c          | 4 ++++
> > > > >    xen/arch/x86/genapic/x2apic.c                | 3 +++
> > > > >    xen/arch/x86/hvm/hvm.c                       | 1 +
> > > > >    xen/arch/x86/nmi.c                           | 1 +
> > > > >    xen/arch/x86/percpu.c                        | 3 +++
> > > > >    xen/arch/x86/psr.c                           | 3 +++
> > > > >    xen/arch/x86/smpboot.c                       | 3 +++
> > > > >    xen/common/kexec.c                           | 1 +
> > > > >    xen/common/rcupdate.c                        | 1 +
> > > > >    xen/common/sched/core.c                      | 1 +
> > > > >    xen/common/sched/cpupool.c                   | 1 +
> > > > >    xen/common/spinlock.c                        | 1 +
> > > > >    xen/common/tasklet.c                         | 1 +
> > > > >    xen/common/timer.c                           | 1 +
> > > > >    xen/drivers/cpufreq/cpufreq.c                | 1 +
> > > > >    xen/drivers/cpufreq/cpufreq_misc_governors.c | 3 +++
> > > > >    xen/drivers/passthrough/x86/hvm.c            | 3 +++
> > > > >    xen/drivers/passthrough/x86/iommu.c          | 3 +++
> > > > >    29 files changed, 59 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > > > > index 2b7101ea25..69c30aecd8 100644
> > > > > --- a/xen/arch/arm/cpuerrata.c
> > > > > +++ b/xen/arch/arm/cpuerrata.c
> > > > > @@ -730,6 +730,7 @@ static int cpu_errata_callback(struct
> > > > > notifier_block
> > > > > *nfb,
> > > > >            rc = enable_nonboot_cpu_caps(arm_errata);
> > > > >            break;
> > > > >        default:
> > > > > +        /* Notifier pattern. */
> > > > Without looking at the commit message (which may not be trivial when
> > > > committed), it is not clear to me what this is supposed to mean. Will
> > > > there
> > > > be a longer explanation in the MISRA doc? Should this be a SAF-*
> > > > comment?
> > > > 
> > > > >            break;
> > > > >        }
> > > > > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> > > > > index eb0a5535e4..4c2bd35403 100644
> > > > > --- a/xen/arch/arm/gic-v3-lpi.c
> > > > > +++ b/xen/arch/arm/gic-v3-lpi.c
> > > > > @@ -389,6 +389,10 @@ static int cpu_callback(struct notifier_block
> > > > > *nfb,
> > > > > unsigned long action,
> > > > >                printk(XENLOG_ERR "Unable to allocate the pendtable for
> > > > > CPU%lu\n",
> > > > >                       cpu);
> > > > >            break;
> > > > > +
> > > > > +    default:
> > > > > +        /* Notifier pattern. */
> > > > > +        break;
> > > > 
> > > > Skimming through v1, it was pointed out that gic-v3-lpi may miss some
> > > > cases.
> > > > 
> > > > Let me start with that I understand this patch is technically not
> > > > changing
> > > > anything. However, it gives us an opportunity to check the notifier
> > > > pattern.
> > > > 
> > > > Has anyone done any proper investigation? If so, what was the outcome?
> > > > If
> > > > not, have we identified someone to do it?
> > > > 
> > > > The same question will apply for place where you add "default".
> > > 
> > > Yes, I also think this could be an opportunity to check the pattern
> > > but no one has yet been identified to do this.
> > 
> > I don't think I understand Julien's question and/or your answer.
> > 
> > Is the question whether someone has done an analysis to make sure this
> > patch covers all notifier patters in the xen codebase?
> 
> I think Jan and Julien's concerns are about the fact that my patch
> takes for granted that all the switch statements are doing the right
> thing: someone should investigate the notifier patterns to confirm that
> their are handling the different cases correctly.

That's really difficult to do, even for the maintainers of the code in
question.

And by not taking this patch we are exposing ourselves to more safety
risks because we cannot make R16.4 blocking.


> > If so, I expect that you have done an analysis simply by basing this
> > patch on the 16.4 violations reported by ECLAIR?
> 
> The previous version of the patch was based only on the reports of
> ECLAIR but Jan said "you left out some patterns, why?".
> 
> So, this version of the patch adds the comment for all the notifier
> patterns I found using git grep "struct notifier_block \*"
> (a superset of the ones reported by ECLAIR because some of them are in
> files excluded from the analysis or deviated).

I think this patch is a step in the right direction. It doesn't prevent
anyone in the community from making expert evaluations on whether the
pattern is implemented correctly.

Honestly, I don't see another way to make progress on this, except for
maybe deviating project-wide "struct notifier_block". But that's
conceptually the same thing as this patch.


Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>