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 <[email protected]>
---
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)
