Re: [PATCH] misra: fix violations in macros GVA_INFO, TRACE_TIME

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>> 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