Re: [XEN PATCH] arm64/vfp: address MISRA C:2012 Dir 4.3

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 23 Aug 2023, Julien Grall wrote:
> Hi Nicola,
> 
> On 23/08/2023 17:09, Nicola Vetrini wrote:
> > On 23/08/2023 16:59, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 23/08/2023 15:27, Nicola Vetrini wrote:
> > > > Directive 4.3 prescribes the following:
> > > > "Assembly language shall be encapsulated and isolated",
> > > > on the grounds of improved readability and ease of maintenance.
> > > > The Directive is violated in this case by asm code in between C code.
> > > > 
> > > > A macro is the chosen encapsulation mechanism.
> > > 
> > > I would rather prefer if we use a static inline.
> > 
> > Just to prevent an possible back and forth on a similar patch:
> > is it ok to adopt the same approach with the inline asm in
> > xen/arch/arm/arm64/lib/bitops.c in the definition of the macros
> > 'bitop' and 'testop'?
> 
> So, in the VFP I agree that moving the assembly part outside of vfp_*_state()
> makes sense even without MISRA. But I don't agree with moving the assembly
> code out as the C function is tightly coupled with the assembly code.
> 
> So this would please MISRA but IHMO would make the code more difficult to
> understand. So I think we should deviate for the bitops.
> 
> Bertrand, Stefano, what do you think?

I agree. I think bitops.c is already encapsulated and introducing
additional macros or functions is likely to make the code worse. testop
and bitop are the encapsulating functions, as you can see there is very
little else other than the asm volatile and a do/while loop.