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

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

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.

I know that this kind of side-effect can be easily deviated, but 
considering the easy fix, I've decided to move the call outside.
By the way, I totally agree with you if you prefer to deviate.

> In fact, if I am not mistaken, both is_pci_passthrough_enabled() and 
> device_get_class() could be marked as pure.
Considering the ASSERT, I do not think we can put the attribute pure.

> >       /*
> >        * 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.

> >       return copy_guest((void *)from, (vaddr_t)to, len,
> > -                      GVA_INFO(current), COPY_to_guest | COPY_linear);
> > +                      GVA_INFO(current_vcpu), COPY_to_guest | 
> > COPY_linear);
> >   }
> >   unsigned long raw_copy_to_guest_flush_dcache(void *to, const void 
> > *from,
> >                                                unsigned int len)
> >   {
> > -    return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current),
> > +    struct vcpu *current_vcpu = current;
> > +    return copy_guest((void *)from, (vaddr_t)to, len, 
> > GVA_INFO(current_vcpu),
> >                         COPY_to_guest | COPY_flush_dcache | COPY_linear);
> >   }
> >   unsigned long raw_clear_guest(void *to, unsigned int len)
> >   {
> > -    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current),
> > +    struct vcpu *current_vcpu = current;
> > +    return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current_vcpu),
> >                         COPY_to_guest | COPY_linear);
> >   }
> >   unsigned long raw_copy_from_guest(void *to, const void __user *from,
> >                                     unsigned int len)
> >   {
> > -    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current),
> > +    struct vcpu *current_vcpu = current;
> > +    return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current_vcpu),
> >                         COPY_from_guest | COPY_linear);
> >   }
> > diff --git a/xen/arch/arm/include/asm/current.h 
> > b/xen/arch/arm/include/asm/current.h
> > index 6973eeb1d1..a66e28fefb 100644
> > --- a/xen/arch/arm/include/asm/current.h
> > +++ b/xen/arch/arm/include/asm/current.h
> > @@ -28,7 +28,7 @@ struct cpu_info {
> >       uint32_t flags;
> >   };
> > -static inline struct cpu_info *get_cpu_info(void)
> > +static inline __attribute_pure__ struct cpu_info *get_cpu_info(void)
> >   {
> >   #ifdef __clang__
> >       unsigned long sp;
> Cheers,
--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)