Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Julien Grall <julien@xxxxxxx>]

Hi Simone,

On 19/10/2023 08:34, Simone Ballarin wrote:
> On 18/10/23 17:03, Julien Grall wrote:
> > Hi,
> >
> > On 18/10/2023 15:18, Simone Ballarin wrote:
> > > Rule 13.1: Initializer lists shall not contain persistent side effects
> > >
> > > This patch moves expressions with side-effects into new variables before
> > > the initializer lists.
> > Looking at the code, I feel the commit message should be a bit more 
> > verbose because they are no apparent side-effects.
> > > Function calls do not necessarily have side-effects, in these cases the
> > > GCC pure or const attributes are added when possible.
> > You are only adding pure in this patch.
> >
> > > No functional changes.
> > >
> > > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > >
> > > ---
> > > Function attributes pure and const do not need to be added as GCC
> > > attributes, they can be added using ECLAIR configurations.
> > > ---
> > >   xen/arch/arm/device.c              |  6 +++---
> > >   xen/arch/arm/guestcopy.c           | 12 ++++++++----
> > >   xen/arch/arm/include/asm/current.h |  2 +-
> > >   3 files changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > > index 1f631d3274..e9be078415 100644
> > > --- a/xen/arch/arm/device.c
> > > +++ b/xen/arch/arm/device.c
> > > @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
> > > dt_device_node *dev, p2m_type_t p2mt,
> > >       int res;
> > >       paddr_t addr, size;
> > >       bool own_device = !dt_device_for_passthrough(dev);
> > > +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
> > > +                             device_get_class(dev) == 
> > > DEVICE_PCI_HOSTBRIDGE;
> > The commit message suggests that the code is moved because there are 
> > side-effects. But none of them should have any side-effects.
> device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
> a side-effect.
Just to confirm my understanding, the side-effect here would be the fact 
that it traps and then panic(). IOW, if the trap was hypothetically 
replaced by a while (1), then it would not be an issue. is it correct?
I can see two solutions:
  1) Remove the ASSERT(). It is only here to make the NULL dereference 
obvious in debug build. That said, the stack trace for a NULL 
dereference would still be as clear.
  2) Replace the ASSERT with a proper check if ( !dev ) return 
DEVICE_UNKONWN. AFAIU, we would not be able to add a 
ASSERT_UNREACHABLE() because this would be again a perceived side-effect.
The former feels a bit circumventing MISRA as the side effect is 
technically still present. Just hidden. But I do prefer over 2).
> > >       /*
> > >        * We want to avoid mapping the MMIO in dom0 for the following 
> > > cases:
> > >        *   - The device is owned by dom0 (i.e. it has been flagged for
> > > @@ -329,9 +331,7 @@ int handle_device(struct domain *d, struct 
> > > dt_device_node *dev, p2m_type_t p2mt,
> > >       struct map_range_data mr_data = {
> > >           .d = d,
> > >           .p2mt = p2mt,
> > > -        .skip_mapping = !own_device ||
> > > -                        (is_pci_passthrough_enabled() &&
> > > -                        (device_get_class(dev) == 
> > > DEVICE_PCI_HOSTBRIDGE)),
> > > +        .skip_mapping = !own_device || dev_is_hostbridge,
> > >           .iomem_ranges = iomem_ranges,
> > >           .irq_ranges = irq_ranges
> > >       };
> > > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> > > index 6716b03561..3ec6743bf6 100644
> > > --- a/xen/arch/arm/guestcopy.c
> > > +++ b/xen/arch/arm/guestcopy.c
> > > @@ -109,27 +109,31 @@ static unsigned long copy_guest(void *buf, 
> > > uint64_t addr, unsigned int len,
> > >   unsigned long raw_copy_to_guest(void *to, const void *from, 
> > > unsigned int len)
> > >   {
> > > +    struct vcpu *current_vcpu = current;
> > It is not clear to me what is the perceived side effect here and the 
> > others below. Can you clarify?
> I will use the pure attribute.
So x86 is using a function to define current. But AFAICT this is not the 
case on Arm. So how would you add the pure?
If it is by adding a function, then I would first like to understand 
which part 'current' is currently perceived as a side-effect.
Cheers,

--
Julien Grall