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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: Simone Ballarin <simone.ballarin@bugseng.com>,
	 xen-devel@lists.xenproject.org, consulting@bugseng.com,
	 sstabellini@kernel.org,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1
Date: Wed, 18 Oct 2023 18:01:26 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2310181759310.965337@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <41d82896-5471-4eaa-8bdd-a192e28d5546@xen.org>

On Wed, 18 Oct 2023, 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@bugseng.com>
> > 
> > ---
> > 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.
> 
> In fact, if I am not mistaken, both is_pci_passthrough_enabled() and
> device_get_class() could be marked as 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 am guessing that's because current is a global variable? But only
reading (not writing) a global variable should be OK?