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

[Julien Grall <julien@xxxxxxx>]

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.
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?
>       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,

--
Julien Grall