| Message ID | 31a926a09dfcef43d13a402fd3b235aeba48009d.1697638210.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violations of Rule 13.1 | expand |
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? > 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,
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?
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@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. 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, >
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.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. >> > > 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,
On 19/10/23 10:19, Julien Grall wrote: > 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.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. >>> >> >> 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? > No, it isn't. A infinite loop is a side effect. > 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. Removing the ASSERT just to make MISRA happy does not sound good to me. > 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. > Replacing it with a proper check can be a solution, but I still prefer to add a deviation or move the invocation outside the initializer list. > 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. > Yes, sorry I was looking to the wrong definition. In ARM the problem is the presence of a *volatile* ASM. Please take a look here: https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} > Cheers, >
Hi, On 19/10/2023 09:43, Simone Ballarin wrote: > On 19/10/23 10:19, Julien Grall wrote: >> 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.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. >>>> >>> >>> 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? > > > No, it isn't. A infinite loop is a side effect. I am not sure why. There are no change of state here. > >> 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. > > Removing the ASSERT just to make MISRA happy does not sound good to me. I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it because it has no value here (we still have stack track if there are any issue). > >> 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. >> > > Replacing it with a proper check can be a solution, but I still prefer > to add a deviation or move the invocation outside the initializer list. In general, I am not in favor of adding deviation if we can avoid them because the code can changed and therefore either moot the deviation or hide any other issue. [...] > Yes, sorry I was looking to the wrong definition. > In ARM the problem is the presence of a *volatile* ASM. > Please take a look here: > > https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} Ok. So the problem is the READ_SYSREG(...). Is there a way we can encapsulate the call so we don't need to use your propose trick or deviate at every use of 'current'? Cheers,
On 19/10/23 12:11, Julien Grall wrote: > Hi, > > On 19/10/2023 09:43, Simone Ballarin wrote: >> On 19/10/23 10:19, Julien Grall wrote: >>> 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.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. >>>>> >>>> >>>> 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? > >> >> No, it isn't. A infinite loop is a side effect. > > I am not sure why. There are no change of state here. > >> >>> 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. >> >> Removing the ASSERT just to make MISRA happy does not sound good to me. > > I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it > because it has no value here (we still have stack track if there are any > issue). > >> >>> 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. >>> >> >> Replacing it with a proper check can be a solution, but I still prefer >> to add a deviation or move the invocation outside the initializer list. > > In general, I am not in favor of adding deviation if we can avoid them > because the code can changed and therefore either moot the deviation or > hide any other issue. > Ok, I will proceed with option 1. > [...] > >> Yes, sorry I was looking to the wrong definition. >> In ARM the problem is the presence of a *volatile* ASM. >> Please take a look here: >> >> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} > > Ok. So the problem is the READ_SYSREG(...). Is there a way we can > encapsulate the call so we don't need to use your propose trick or > deviate at every use of 'current'? > The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside the initializer (there are no advantages in wrapping it on a function if the function cannot be declared pure). The proposed solution seems to me the cleanest way do to it. I do not see any other acceptable solutions. > Cheers, >
Hi, On 19/10/2023 12:10, Simone Ballarin wrote: > On 19/10/23 12:11, Julien Grall wrote: >> Hi, >> >> On 19/10/2023 09:43, Simone Ballarin wrote: >>> On 19/10/23 10:19, Julien Grall wrote: >>>> 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.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. >>>>>> >>>>> >>>>> 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? > >>> >>> No, it isn't. A infinite loop is a side effect. >> >> I am not sure why. There are no change of state here. >> >>> >>>> 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. >>> >>> Removing the ASSERT just to make MISRA happy does not sound good to me. >> >> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it >> because it has no value here (we still have stack track if there are >> any issue). >> >>> >>>> 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. >>>> >>> >>> Replacing it with a proper check can be a solution, but I still >>> prefer to add a deviation or move the invocation outside the >>> initializer list. >> >> In general, I am not in favor of adding deviation if we can avoid them >> because the code can changed and therefore either moot the deviation >> or hide any other issue. >> > > Ok, I will proceed with option 1. > >> [...] >> >>> Yes, sorry I was looking to the wrong definition. >>> In ARM the problem is the presence of a *volatile* ASM. >>> Please take a look here: >>> >>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} >> >> Ok. So the problem is the READ_SYSREG(...). Is there a way we can >> encapsulate the call so we don't need to use your propose trick or >> deviate at every use of 'current'? >> > > The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside > the initializer (there are no advantages in wrapping it on a function > if the function cannot be declared pure). I was thinking that maybe it could help to deviate. > > The proposed solution seems to me the cleanest way do to it. I do not > see any other acceptable solutions. I have some concern with the proposal (they are most likely matter of taste). We usually use this trick when 'current' is used multiple time to save process (using 'current' is not cost free). But in this case, this is renaming without any apparent benefits. If we wanted to go down this route, then this would likely want a comment. At which point we should just deviate. I will have a think if there are something else we can do. Could we consider to not address it for now? Cheers,
On 19/10/23 14:30, Julien Grall wrote: > Hi, > > On 19/10/2023 12:10, Simone Ballarin wrote: >> On 19/10/23 12:11, Julien Grall wrote: >>> Hi, >>> >>> On 19/10/2023 09:43, Simone Ballarin wrote: >>>> On 19/10/23 10:19, Julien Grall wrote: >>>>> 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.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. >>>>>>> >>>>>> >>>>>> 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? > >>>> >>>> No, it isn't. A infinite loop is a side effect. >>> >>> I am not sure why. There are no change of state here. >>> >>>> >>>>> 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. >>>> >>>> Removing the ASSERT just to make MISRA happy does not sound good to me. >>> >>> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested >>> it because it has no value here (we still have stack track if there >>> are any issue). >>> >>>> >>>>> 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. >>>>> >>>> >>>> Replacing it with a proper check can be a solution, but I still >>>> prefer to add a deviation or move the invocation outside the >>>> initializer list. >>> >>> In general, I am not in favor of adding deviation if we can avoid >>> them because the code can changed and therefore either moot the >>> deviation or hide any other issue. >>> >> >> Ok, I will proceed with option 1. >> >>> [...] >>> >>>> Yes, sorry I was looking to the wrong definition. >>>> In ARM the problem is the presence of a *volatile* ASM. >>>> Please take a look here: >>>> >>>> https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} >>> >>> Ok. So the problem is the READ_SYSREG(...). Is there a way we can >>> encapsulate the call so we don't need to use your propose trick or >>> deviate at every use of 'current'? >>> >> >> The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside >> the initializer (there are no advantages in wrapping it on a function >> if the function cannot be declared pure). > > I was thinking that maybe it could help to deviate. > >> >> The proposed solution seems to me the cleanest way do to it. I do not >> see any other acceptable solutions. > > I have some concern with the proposal (they are most likely matter of > taste). > > We usually use this trick when 'current' is used multiple time to save > process (using 'current' is not cost free). But in this case, this is > renaming without any apparent benefits. > > If we wanted to go down this route, then this would likely want a > comment. At which point we should just deviate. > > I will have a think if there are something else we can do. Could we > consider to not address it for now? > I totally agree. > Cheers, >
On Thu, 19 Oct 2023, Simone Ballarin wrote: > On 19/10/23 14:30, Julien Grall wrote: > > Hi, > > > > On 19/10/2023 12:10, Simone Ballarin wrote: > > > On 19/10/23 12:11, Julien Grall wrote: > > > > Hi, > > > > > > > > On 19/10/2023 09:43, Simone Ballarin wrote: > > > > > On 19/10/23 10:19, Julien Grall wrote: > > > > > > 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.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. > > > > > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > No, it isn't. A infinite loop is a side effect. > > > > > > > > I am not sure why. There are no change of state here. > > > > > > > > > > > > > > > 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. > > > > > > > > > > Removing the ASSERT just to make MISRA happy does not sound good to > > > > > me. > > > > > > > > I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it > > > > because it has no value here (we still have stack track if there are any > > > > issue). > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > Replacing it with a proper check can be a solution, but I still prefer > > > > > to add a deviation or move the invocation outside the initializer > > > > > list. > > > > > > > > In general, I am not in favor of adding deviation if we can avoid them > > > > because the code can changed and therefore either moot the deviation or > > > > hide any other issue. > > > > > > > > > > Ok, I will proceed with option 1. > > > > > > > [...] > > > > > > > > > Yes, sorry I was looking to the wrong definition. > > > > > In ARM the problem is the presence of a *volatile* ASM. > > > > > Please take a look here: > > > > > > > > > > https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}} > > > > > > > > Ok. So the problem is the READ_SYSREG(...). Is there a way we can > > > > encapsulate the call so we don't need to use your propose trick or > > > > deviate at every use of 'current'? > > > > > > > > > > The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside > > > the initializer (there are no advantages in wrapping it on a function > > > if the function cannot be declared pure). > > > > I was thinking that maybe it could help to deviate. > > > > > > > > The proposed solution seems to me the cleanest way do to it. I do not see > > > any other acceptable solutions. > > > > I have some concern with the proposal (they are most likely matter of > > taste). > > > > We usually use this trick when 'current' is used multiple time to save > > process (using 'current' is not cost free). But in this case, this is > > renaming without any apparent benefits. > > > > If we wanted to go down this route, then this would likely want a comment. > > At which point we should just deviate. > > > > I will have a think if there are something else we can do. Could we consider > > to not address it for now? > > > > I totally agree. I am wondering if we could deviate "current" because even with the implementation using volatile we know it doesn't have "side effects" in the sense of changing things for other code outside the function.
Hi Stefano, On 19/10/2023 19:30, Stefano Stabellini wrote: >>> We usually use this trick when 'current' is used multiple time to save >>> process (using 'current' is not cost free). But in this case, this is >>> renaming without any apparent benefits. >>> >>> If we wanted to go down this route, then this would likely want a comment. >>> At which point we should just deviate. >>> >>> I will have a think if there are something else we can do. Could we consider >>> to not address it for now? >>> >> >> I totally agree. > > I am wondering if we could deviate "current" because even with the > implementation using volatile we know it doesn't have "side effects" in > the sense of changing things for other code outside the function. I will let Simone to confirm whether it is possible to do it from a technical point of view. Leaving the technical part aside, is the only violations are the one in this patch? If so, I don't think it makes sense to deviate 'current' globally. It would be better to have local deviations. Cheers,
On 20/10/23 10:28, Julien Grall wrote: > Hi Stefano, > > On 19/10/2023 19:30, Stefano Stabellini wrote: >>>> We usually use this trick when 'current' is used multiple time to save >>>> process (using 'current' is not cost free). But in this case, this is >>>> renaming without any apparent benefits. >>>> >>>> If we wanted to go down this route, then this would likely want a >>>> comment. >>>> At which point we should just deviate. >>>> >>>> I will have a think if there are something else we can do. Could we >>>> consider >>>> to not address it for now? >>>> >>> >>> I totally agree. >> >> I am wondering if we could deviate "current" because even with the >> implementation using volatile we know it doesn't have "side effects" in >> the sense of changing things for other code outside the function. > > I will let Simone to confirm whether it is possible to do it from a > technical point of view. Yes, it is possible. > > Leaving the technical part aside, is the only violations are the one in > this patch? If so, I don't think it makes sense to deviate 'current' > globally. It would be better to have local deviations. > > Cheers, > For this specific case I agree in using a local deviation. In the next version of the patch, I will use a SAF comment.
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; /* * 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; 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;
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. Function calls do not necessarily have side-effects, in these cases the GCC pure or const attributes are added when possible. 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(-)