xen/arch/arm/guestcopy.c | 12 ++++++++---- xen/common/sched/core.c | 11 ++++++----- 2 files changed, 14 insertions(+), 9 deletions(-)
MISRA Rule 13.1: Initializer lists shall not contain persistent side
effects.
The violations occur because both the `GVA_INFO` and `TRACE_TIME` macro
expansions include expressions with persistent side effects introduced
via inline assembly.
In the case of `GVA_INFO`, the issue stems from the initializer list
containing a direct call to `current`, which evaluates to
`this_cpu(curr_vcpu)` and involves persistent side effects via the
`asm` statement. To resolve this, the side-effect-producing expression
is computed in a separate statement prior to the macro initialization:
struct vcpu *current_vcpu = current;
The computed value is passed into the `GVA_INFO(current_vcpu)` macro,
ensuring that the initializer is clean and free of such side effects.
Similarly, the `TRACE_TIME` macro violates this rule when accessing
expressions like `current->vcpu_id` and `current->domain->domain_id`,
which also depend on `current` and inline assembly. To fix this, the
value of `current` is assigned to a temporary variable:
struct vcpu *v = current;
This temporary variable is then used to access `domain_id` and `vcpu_id`.
This ensures that the arguments passed to the `TRACE_TIME` macro are
simple expressions free of persistent side effects.
Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Test CI pipeline:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1959339335
---
xen/arch/arm/guestcopy.c | 12 ++++++++----
xen/common/sched/core.c | 11 ++++++-----
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 497e785ec4..f483908510 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/common/sched/core.c b/xen/common/sched/core.c
index 4c77ea4b8d..a2c53dca14 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1514,7 +1514,7 @@ static long do_poll(const struct sched_poll *sched_poll)
/* Voluntarily yield the processor for this allocation. */
long vcpu_yield(void)
{
- struct vcpu * v=current;
+ struct vcpu *v = current;
spinlock_t *lock;
rcu_read_lock(&sched_res_rculock);
@@ -1527,7 +1527,7 @@ long vcpu_yield(void)
SCHED_STAT_CRANK(vcpu_yield);
- TRACE_TIME(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
+ TRACE_TIME(TRC_SCHED_YIELD, v->domain->domain_id, v->vcpu_id);
raise_softirq(SCHEDULE_SOFTIRQ);
return 0;
}
@@ -1899,6 +1899,7 @@ typedef long ret_t;
ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
ret_t ret = 0;
+ struct vcpu *v = current;
switch ( cmd )
{
@@ -1922,8 +1923,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&sched_shutdown, arg, 1) )
break;
- TRACE_TIME(TRC_SCHED_SHUTDOWN, current->domain->domain_id,
- current->vcpu_id, sched_shutdown.reason);
+ TRACE_TIME(TRC_SCHED_SHUTDOWN, v->domain->domain_id,
+ v->vcpu_id, sched_shutdown.reason);
ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
break;
@@ -1938,7 +1939,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&sched_shutdown, arg, 1) )
break;
- TRACE_TIME(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, current->vcpu_id,
+ TRACE_TIME(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, v->vcpu_id,
sched_shutdown.reason);
spin_lock(&d->shutdown_lock);
--
2.43.0On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: > MISRA Rule 13.1: Initializer lists shall not contain persistent side > effects. > > The violations occur because both the `GVA_INFO` and `TRACE_TIME` macro > expansions include expressions with persistent side effects introduced > via inline assembly. > > In the case of `GVA_INFO`, the issue stems from the initializer list > containing a direct call to `current`, which evaluates to > `this_cpu(curr_vcpu)` and involves persistent side effects via the > `asm` statement. To resolve this, the side-effect-producing expression > is computed in a separate statement prior to the macro initialization: > > struct vcpu *current_vcpu = current; > > The computed value is passed into the `GVA_INFO(current_vcpu)` macro, > ensuring that the initializer is clean and free of such side effects. > > Similarly, the `TRACE_TIME` macro violates this rule when accessing > expressions like `current->vcpu_id` and `current->domain->domain_id`, > which also depend on `current` and inline assembly. To fix this, the > value of `current` is assigned to a temporary variable: > > struct vcpu *v = current; > > This temporary variable is then used to access `domain_id` and `vcpu_id`. > This ensures that the arguments passed to the `TRACE_TIME` macro are > simple expressions free of persistent side effects. > > Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> The macro `current` specifically does not (and must not) have side effects. It is expected to behave like a plain `struct vcpu *current;` variable, and what Eclair is noticing is the thread-local machinery under this_cpu() (or in x86's case, get_current()). In ARM's case, it's literally reading the hardware thread pointer register. Can anything be done to tell Eclair that `this_cpu()` specifically does not have side effects? The only reason that GVA_INFO() and TRACE_TIME() are picked out is because they both contain embedded structure initialisation, and this is is actually an example where trying to comply with MISRA interferes with what is otherwise a standard pattern in Xen. ~Andrew
On 31.07.2025 17:37, Andrew Cooper wrote: > On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >> MISRA Rule 13.1: Initializer lists shall not contain persistent side >> effects. >> >> The violations occur because both the `GVA_INFO` and `TRACE_TIME` macro >> expansions include expressions with persistent side effects introduced >> via inline assembly. >> >> In the case of `GVA_INFO`, the issue stems from the initializer list >> containing a direct call to `current`, which evaluates to >> `this_cpu(curr_vcpu)` and involves persistent side effects via the >> `asm` statement. To resolve this, the side-effect-producing expression >> is computed in a separate statement prior to the macro initialization: >> >> struct vcpu *current_vcpu = current; >> >> The computed value is passed into the `GVA_INFO(current_vcpu)` macro, >> ensuring that the initializer is clean and free of such side effects. >> >> Similarly, the `TRACE_TIME` macro violates this rule when accessing >> expressions like `current->vcpu_id` and `current->domain->domain_id`, >> which also depend on `current` and inline assembly. To fix this, the >> value of `current` is assigned to a temporary variable: >> >> struct vcpu *v = current; >> >> This temporary variable is then used to access `domain_id` and `vcpu_id`. >> This ensures that the arguments passed to the `TRACE_TIME` macro are >> simple expressions free of persistent side effects. >> >> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> > > The macro `current` specifically does not (and must not) have side > effects. It is expected to behave like a plain `struct vcpu *current;` > variable, and what Eclair is noticing is the thread-local machinery > under this_cpu() (or in x86's case, get_current()). > > In ARM's case, it's literally reading the hardware thread pointer > register. Can anything be done to tell Eclair that `this_cpu()` > specifically does not have side effects? > > The only reason that GVA_INFO() and TRACE_TIME() are picked out is > because they both contain embedded structure initialisation, and this is > is actually an example where trying to comply with MISRA interferes with > what is otherwise a standard pattern in Xen. Irrespective of what you say, some of the changes here were eliminating multiple adjacent uses of current, which - iirc - often the compiler can't fold via CSE. Jan
On 31/07/2025 4:58 pm, Jan Beulich wrote: > On 31.07.2025 17:37, Andrew Cooper wrote: >> On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >>> MISRA Rule 13.1: Initializer lists shall not contain persistent side >>> effects. >>> >>> The violations occur because both the `GVA_INFO` and `TRACE_TIME` macro >>> expansions include expressions with persistent side effects introduced >>> via inline assembly. >>> >>> In the case of `GVA_INFO`, the issue stems from the initializer list >>> containing a direct call to `current`, which evaluates to >>> `this_cpu(curr_vcpu)` and involves persistent side effects via the >>> `asm` statement. To resolve this, the side-effect-producing expression >>> is computed in a separate statement prior to the macro initialization: >>> >>> struct vcpu *current_vcpu = current; >>> >>> The computed value is passed into the `GVA_INFO(current_vcpu)` macro, >>> ensuring that the initializer is clean and free of such side effects. >>> >>> Similarly, the `TRACE_TIME` macro violates this rule when accessing >>> expressions like `current->vcpu_id` and `current->domain->domain_id`, >>> which also depend on `current` and inline assembly. To fix this, the >>> value of `current` is assigned to a temporary variable: >>> >>> struct vcpu *v = current; >>> >>> This temporary variable is then used to access `domain_id` and `vcpu_id`. >>> This ensures that the arguments passed to the `TRACE_TIME` macro are >>> simple expressions free of persistent side effects. >>> >>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> >> The macro `current` specifically does not (and must not) have side >> effects. It is expected to behave like a plain `struct vcpu *current;` >> variable, and what Eclair is noticing is the thread-local machinery >> under this_cpu() (or in x86's case, get_current()). >> >> In ARM's case, it's literally reading the hardware thread pointer >> register. Can anything be done to tell Eclair that `this_cpu()` >> specifically does not have side effects? >> >> The only reason that GVA_INFO() and TRACE_TIME() are picked out is >> because they both contain embedded structure initialisation, and this is >> is actually an example where trying to comply with MISRA interferes with >> what is otherwise a standard pattern in Xen. > Irrespective of what you say, some of the changes here were eliminating > multiple adjacent uses of current, which - iirc - often the compiler > can't fold via CSE. Where we have mixed usage, sure. (I'm sure I've got a branch somewhere trying to add some more pure/const around to try and help out here, but I can't find it, and don't recall it being a major improvement either.) The real problem here is that there are a *very few* number of contexts where Eclair refuses to tolerate the use of `current` citing side effects, despite there being no side effects. That is the thing that breaks the principle of least surprise, and we ought to fix it by making Eclair happy with `current` everywhere, rather than force people to learn that 2 macros can't have a `current` in their parameter list. ~Andrew
On 2025-07-31 18:05, Andrew Cooper wrote: > On 31/07/2025 4:58 pm, Jan Beulich wrote: >> On 31.07.2025 17:37, Andrew Cooper wrote: >>> On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >>>> MISRA Rule 13.1: Initializer lists shall not contain persistent side >>>> effects. >>>> >>>> The violations occur because both the `GVA_INFO` and `TRACE_TIME` >>>> macro >>>> expansions include expressions with persistent side effects >>>> introduced >>>> via inline assembly. >>>> >>>> In the case of `GVA_INFO`, the issue stems from the initializer list >>>> containing a direct call to `current`, which evaluates to >>>> `this_cpu(curr_vcpu)` and involves persistent side effects via the >>>> `asm` statement. To resolve this, the side-effect-producing >>>> expression >>>> is computed in a separate statement prior to the macro >>>> initialization: >>>> >>>> struct vcpu *current_vcpu = current; >>>> >>>> The computed value is passed into the `GVA_INFO(current_vcpu)` >>>> macro, >>>> ensuring that the initializer is clean and free of such side >>>> effects. >>>> >>>> Similarly, the `TRACE_TIME` macro violates this rule when accessing >>>> expressions like `current->vcpu_id` and >>>> `current->domain->domain_id`, >>>> which also depend on `current` and inline assembly. To fix this, the >>>> value of `current` is assigned to a temporary variable: >>>> >>>> struct vcpu *v = current; >>>> >>>> This temporary variable is then used to access `domain_id` and >>>> `vcpu_id`. >>>> This ensures that the arguments passed to the `TRACE_TIME` macro are >>>> simple expressions free of persistent side effects. >>>> >>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> >>> The macro `current` specifically does not (and must not) have side >>> effects. It is expected to behave like a plain `struct vcpu >>> *current;` >>> variable, and what Eclair is noticing is the thread-local machinery >>> under this_cpu() (or in x86's case, get_current()). >>> >>> In ARM's case, it's literally reading the hardware thread pointer >>> register. Can anything be done to tell Eclair that `this_cpu()` >>> specifically does not have side effects? >>> >>> The only reason that GVA_INFO() and TRACE_TIME() are picked out is >>> because they both contain embedded structure initialisation, and this >>> is >>> is actually an example where trying to comply with MISRA interferes >>> with >>> what is otherwise a standard pattern in Xen. >> Irrespective of what you say, some of the changes here were >> eliminating >> multiple adjacent uses of current, which - iirc - often the compiler >> can't fold via CSE. > > Where we have mixed usage, sure. (I'm sure I've got a branch somewhere > trying to add some more pure/const around to try and help out here, but > I can't find it, and don't recall it being a major improvement either.) > > The real problem here is that there are a *very few* number of contexts > where Eclair refuses to tolerate the use of `current` citing side > effects, despite there being no side effects. > > That is the thing that breaks the principle of least surprise, and we > ought to fix it by making Eclair happy with `current` everywhere, > rather > than force people to learn that 2 macros can't have a `current` in > their > parameter list. > I'll take a look. Likely yes, by adding a handful of properties. There are subtleties, though. -- Nicola Vetrini, B.Sc. Software Engineer BUGSENG (https://bugseng.com) LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 7/31/25 19:09, Nicola Vetrini wrote: > On 2025-07-31 18:05, Andrew Cooper wrote: >> On 31/07/2025 4:58 pm, Jan Beulich wrote: >>> On 31.07.2025 17:37, Andrew Cooper wrote: >>>> On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >>>>> MISRA Rule 13.1: Initializer lists shall not contain persistent side >>>>> effects. >>>>> >>>>> The violations occur because both the `GVA_INFO` and `TRACE_TIME` >>>>> macro >>>>> expansions include expressions with persistent side effects introduced >>>>> via inline assembly. >>>>> >>>>> In the case of `GVA_INFO`, the issue stems from the initializer list >>>>> containing a direct call to `current`, which evaluates to >>>>> `this_cpu(curr_vcpu)` and involves persistent side effects via the >>>>> `asm` statement. To resolve this, the side-effect-producing expression >>>>> is computed in a separate statement prior to the macro initialization: >>>>> >>>>> struct vcpu *current_vcpu = current; >>>>> >>>>> The computed value is passed into the `GVA_INFO(current_vcpu)` macro, >>>>> ensuring that the initializer is clean and free of such side effects. >>>>> >>>>> Similarly, the `TRACE_TIME` macro violates this rule when accessing >>>>> expressions like `current->vcpu_id` and `current->domain->domain_id`, >>>>> which also depend on `current` and inline assembly. To fix this, the >>>>> value of `current` is assigned to a temporary variable: >>>>> >>>>> struct vcpu *v = current; >>>>> >>>>> This temporary variable is then used to access `domain_id` and >>>>> `vcpu_id`. >>>>> This ensures that the arguments passed to the `TRACE_TIME` macro are >>>>> simple expressions free of persistent side effects. >>>>> >>>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> >>>> The macro `current` specifically does not (and must not) have side >>>> effects. It is expected to behave like a plain `struct vcpu *current;` >>>> variable, and what Eclair is noticing is the thread-local machinery >>>> under this_cpu() (or in x86's case, get_current()). >>>> >>>> In ARM's case, it's literally reading the hardware thread pointer >>>> register. Can anything be done to tell Eclair that `this_cpu()` >>>> specifically does not have side effects? >>>> >>>> The only reason that GVA_INFO() and TRACE_TIME() are picked out is >>>> because they both contain embedded structure initialisation, and >>>> this is >>>> is actually an example where trying to comply with MISRA interferes >>>> with >>>> what is otherwise a standard pattern in Xen. >>> Irrespective of what you say, some of the changes here were eliminating >>> multiple adjacent uses of current, which - iirc - often the compiler >>> can't fold via CSE. >> >> Where we have mixed usage, sure. (I'm sure I've got a branch somewhere >> trying to add some more pure/const around to try and help out here, but >> I can't find it, and don't recall it being a major improvement either.) >> >> The real problem here is that there are a *very few* number of contexts >> where Eclair refuses to tolerate the use of `current` citing side >> effects, despite there being no side effects. >> >> That is the thing that breaks the principle of least surprise, and we >> ought to fix it by making Eclair happy with `current` everywhere, rather >> than force people to learn that 2 macros can't have a `current` in their >> parameter list. >> > > I'll take a look. Likely yes, by adding a handful of properties. There > are subtleties, though. > Hi, Nicola. Did you have a chance to try configure Eclair to ignore this macro `this_cpu()`? Thanks. Dmytro
On 2025-08-05 13:49, Dmytro Prokopchuk1 wrote: > On 7/31/25 19:09, Nicola Vetrini wrote: >> On 2025-07-31 18:05, Andrew Cooper wrote: >>> On 31/07/2025 4:58 pm, Jan Beulich wrote: >>>> On 31.07.2025 17:37, Andrew Cooper wrote: >>>>> On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >>>>>> MISRA Rule 13.1: Initializer lists shall not contain persistent >>>>>> side >>>>>> effects. >>>>>> >>>>>> The violations occur because both the `GVA_INFO` and `TRACE_TIME` >>>>>> macro >>>>>> expansions include expressions with persistent side effects >>>>>> introduced >>>>>> via inline assembly. >>>>>> >>>>>> In the case of `GVA_INFO`, the issue stems from the initializer >>>>>> list >>>>>> containing a direct call to `current`, which evaluates to >>>>>> `this_cpu(curr_vcpu)` and involves persistent side effects via the >>>>>> `asm` statement. To resolve this, the side-effect-producing >>>>>> expression >>>>>> is computed in a separate statement prior to the macro >>>>>> initialization: >>>>>> >>>>>> struct vcpu *current_vcpu = current; >>>>>> >>>>>> The computed value is passed into the `GVA_INFO(current_vcpu)` >>>>>> macro, >>>>>> ensuring that the initializer is clean and free of such side >>>>>> effects. >>>>>> >>>>>> Similarly, the `TRACE_TIME` macro violates this rule when >>>>>> accessing >>>>>> expressions like `current->vcpu_id` and >>>>>> `current->domain->domain_id`, >>>>>> which also depend on `current` and inline assembly. To fix this, >>>>>> the >>>>>> value of `current` is assigned to a temporary variable: >>>>>> >>>>>> struct vcpu *v = current; >>>>>> >>>>>> This temporary variable is then used to access `domain_id` and >>>>>> `vcpu_id`. >>>>>> This ensures that the arguments passed to the `TRACE_TIME` macro >>>>>> are >>>>>> simple expressions free of persistent side effects. >>>>>> >>>>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> >>>>> The macro `current` specifically does not (and must not) have side >>>>> effects. It is expected to behave like a plain `struct vcpu >>>>> *current;` >>>>> variable, and what Eclair is noticing is the thread-local machinery >>>>> under this_cpu() (or in x86's case, get_current()). >>>>> >>>>> In ARM's case, it's literally reading the hardware thread pointer >>>>> register. Can anything be done to tell Eclair that `this_cpu()` >>>>> specifically does not have side effects? >>>>> >>>>> The only reason that GVA_INFO() and TRACE_TIME() are picked out is >>>>> because they both contain embedded structure initialisation, and >>>>> this is >>>>> is actually an example where trying to comply with MISRA interferes >>>>> with >>>>> what is otherwise a standard pattern in Xen. >>>> Irrespective of what you say, some of the changes here were >>>> eliminating >>>> multiple adjacent uses of current, which - iirc - often the compiler >>>> can't fold via CSE. >>> >>> Where we have mixed usage, sure. (I'm sure I've got a branch >>> somewhere >>> trying to add some more pure/const around to try and help out here, >>> but >>> I can't find it, and don't recall it being a major improvement >>> either.) >>> >>> The real problem here is that there are a *very few* number of >>> contexts >>> where Eclair refuses to tolerate the use of `current` citing side >>> effects, despite there being no side effects. >>> >>> That is the thing that breaks the principle of least surprise, and we >>> ought to fix it by making Eclair happy with `current` everywhere, >>> rather >>> than force people to learn that 2 macros can't have a `current` in >>> their >>> parameter list. >>> >> >> I'll take a look. Likely yes, by adding a handful of properties. There >> are subtleties, though. >> > > Hi, Nicola. > > Did you have a chance to try configure Eclair to ignore this macro > `this_cpu()`? > Hi Dmytro, I'm on it, I needed to handle other tasks first. > Thanks. > Dmytro -- Nicola Vetrini, B.Sc. Software Engineer BUGSENG (https://bugseng.com) LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 2025-08-05 15:22, Nicola Vetrini wrote: > On 2025-08-05 13:49, Dmytro Prokopchuk1 wrote: >> On 7/31/25 19:09, Nicola Vetrini wrote: >>> On 2025-07-31 18:05, Andrew Cooper wrote: >>>> On 31/07/2025 4:58 pm, Jan Beulich wrote: >>>>> On 31.07.2025 17:37, Andrew Cooper wrote: >>>>>> On 31/07/2025 4:16 pm, Dmytro Prokopchuk1 wrote: >>>>>>> MISRA Rule 13.1: Initializer lists shall not contain persistent >>>>>>> side >>>>>>> effects. >>>>>>> >>>>>>> The violations occur because both the `GVA_INFO` and `TRACE_TIME` >>>>>>> macro >>>>>>> expansions include expressions with persistent side effects >>>>>>> introduced >>>>>>> via inline assembly. >>>>>>> >>>>>>> In the case of `GVA_INFO`, the issue stems from the initializer >>>>>>> list >>>>>>> containing a direct call to `current`, which evaluates to >>>>>>> `this_cpu(curr_vcpu)` and involves persistent side effects via >>>>>>> the >>>>>>> `asm` statement. To resolve this, the side-effect-producing >>>>>>> expression >>>>>>> is computed in a separate statement prior to the macro >>>>>>> initialization: >>>>>>> >>>>>>> struct vcpu *current_vcpu = current; >>>>>>> >>>>>>> The computed value is passed into the `GVA_INFO(current_vcpu)` >>>>>>> macro, >>>>>>> ensuring that the initializer is clean and free of such side >>>>>>> effects. >>>>>>> >>>>>>> Similarly, the `TRACE_TIME` macro violates this rule when >>>>>>> accessing >>>>>>> expressions like `current->vcpu_id` and >>>>>>> `current->domain->domain_id`, >>>>>>> which also depend on `current` and inline assembly. To fix this, >>>>>>> the >>>>>>> value of `current` is assigned to a temporary variable: >>>>>>> >>>>>>> struct vcpu *v = current; >>>>>>> >>>>>>> This temporary variable is then used to access `domain_id` and >>>>>>> `vcpu_id`. >>>>>>> This ensures that the arguments passed to the `TRACE_TIME` macro >>>>>>> are >>>>>>> simple expressions free of persistent side effects. >>>>>>> >>>>>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com> >>>>>> The macro `current` specifically does not (and must not) have side >>>>>> effects. It is expected to behave like a plain `struct vcpu >>>>>> *current;` >>>>>> variable, and what Eclair is noticing is the thread-local >>>>>> machinery >>>>>> under this_cpu() (or in x86's case, get_current()). >>>>>> >>>>>> In ARM's case, it's literally reading the hardware thread pointer >>>>>> register. Can anything be done to tell Eclair that `this_cpu()` >>>>>> specifically does not have side effects? >>>>>> >>>>>> The only reason that GVA_INFO() and TRACE_TIME() are picked out is >>>>>> because they both contain embedded structure initialisation, and >>>>>> this is >>>>>> is actually an example where trying to comply with MISRA >>>>>> interferes >>>>>> with >>>>>> what is otherwise a standard pattern in Xen. >>>>> Irrespective of what you say, some of the changes here were >>>>> eliminating >>>>> multiple adjacent uses of current, which - iirc - often the >>>>> compiler >>>>> can't fold via CSE. >>>> >>>> Where we have mixed usage, sure. (I'm sure I've got a branch >>>> somewhere >>>> trying to add some more pure/const around to try and help out here, >>>> but >>>> I can't find it, and don't recall it being a major improvement >>>> either.) >>>> >>>> The real problem here is that there are a *very few* number of >>>> contexts >>>> where Eclair refuses to tolerate the use of `current` citing side >>>> effects, despite there being no side effects. >>>> >>>> That is the thing that breaks the principle of least surprise, and >>>> we >>>> ought to fix it by making Eclair happy with `current` everywhere, >>>> rather >>>> than force people to learn that 2 macros can't have a `current` in >>>> their >>>> parameter list. >>>> >>> >>> I'll take a look. Likely yes, by adding a handful of properties. >>> There >>> are subtleties, though. >>> >> >> Hi, Nicola. >> >> Did you have a chance to try configure Eclair to ignore this macro >> `this_cpu()`? >> > > Hi Dmytro, > > I'm on it, I needed to handle other tasks first. > A solution has been devised by extending ECLAIR. The runner will be updated with the latest ECLAIR version, and as a result a couple of other patches will be submitted to adapt for it. >> Thanks. >> Dmytro -- Nicola Vetrini, B.Sc. Software Engineer BUGSENG (https://bugseng.com) LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 31.07.2025 17:16, Dmytro Prokopchuk1 wrote:
> --- 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;
The commonly used name for this kind of variable is curr. Also wherever
you introduce one anew, it would preferably be pointer-to-const when
possible. (This isn't a request to rename or re-type existing variables
right here.)
> @@ -1899,6 +1899,7 @@ typedef long ret_t;
> ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> {
> ret_t ret = 0;
> + struct vcpu *v = current;
I wonder if this wasn't better introduced in the two scopes that actually
need it.
> @@ -1922,8 +1923,8 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( copy_from_guest(&sched_shutdown, arg, 1) )
> break;
>
> - TRACE_TIME(TRC_SCHED_SHUTDOWN, current->domain->domain_id,
> - current->vcpu_id, sched_shutdown.reason);
> + TRACE_TIME(TRC_SCHED_SHUTDOWN, v->domain->domain_id,
> + v->vcpu_id, sched_shutdown.reason);
> ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
>
> break;
> @@ -1938,7 +1939,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( copy_from_guest(&sched_shutdown, arg, 1) )
> break;
>
> - TRACE_TIME(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, current->vcpu_id,
> + TRACE_TIME(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, v->vcpu_id,
> sched_shutdown.reason);
A few lines up from here there's another use of current that you then
would also want to replace by curr.
Jan
On 31.07.25 17:32, Jan Beulich wrote:
> On 31.07.2025 17:16, Dmytro Prokopchuk1 wrote:
>> --- 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;
>
> The commonly used name for this kind of variable is curr. Also wherever
> you introduce one anew, it would preferably be pointer-to-const when
> possible. (This isn't a request to rename or re-type existing variables
> right here.)
>
>> @@ -1899,6 +1899,7 @@ typedef long ret_t;
>> ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> {
>> ret_t ret = 0;
>> + struct vcpu *v = current;
>
> I wonder if this wasn't better introduced in the two scopes that actually
> need it.
I'd be in favor of keeping it in function scope, but with replacing all
references of "current" in this function with "v". Doing the same in
vcpu_yield() shows the following code reductions:
bloat-o-meter core-orig.o core.o
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-160 (-160)
Function old new delta
vcpu_yield 455 449 -6
do_sched_op 1216 1139 -77
compat_sched_op 1211 1134 -77
Total: Before=36174, After=36014, chg -0.44%
Dmytro, with Andrew's response regarding the Misra "violation" with "current",
its up to you whether you want to send another patch for common/sched/core.c.
In case you don't want to do that, please tell me and I'll send that patch.
Juergen
© 2016 - 2025 Red Hat, Inc.