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.balla...@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.
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