Re: [XEN PATCH v4 1/4] xen: add SAF deviation for debugging and logging effects

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2024 11:03, Simone Ballarin wrote:
> On 06/02/24 13:04, Jan Beulich wrote:
>> On 02.02.2024 16:16, Simone Ballarin wrote:
>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>
>>> Effects caused by debug/logging macros and functions (like ASSERT, 
>>> __bad_atomic_size,
>>> LOG, etc ...) that crash execution or produce logs are not dangerous in 
>>> initializer
>>> lists. The evaluation order in abnormal conditions is not relevant. 
>>> Evaluation order
>>> of logging effects is always safe.
>>
>> I thought I said so before: When talking of just logging, evaluation order
>> may very well have a impact on correctness. Therefore we shouldn't mix
>> debugging and logging.
> 
> My general feeling was that changes like the following one are not supported 
> by
> the community:
> 
> - x = { .field1 = function_with_logs_effects() /*other eventual code*/ };
> + int field1 = function_with_logs_effects();
> + x = { .field1 = field1 /*other eventual code*/};
> 
> so I tried to deviate as much as possible.
> 
> If having log effects is a good reason to do changes like the above, I can
> propose a patch in that sense.

Just to avoid misunderstandings: I'm not advocating for changes like the
one you outline above. I simply consider the rule too strict: There's
nothing at risk when there's just a single operation with side effects
in an initializer. Even when there are multiple such operations, whether
there's anything at risk depends on whether any of the side effects
actually collide. In a number of cases the compiler would actually warn
(and thus, due to -Werror, the build would fail).

The primary purpose of my comment here was that we need to please
separate debugging from logging side effects.

>>> --- a/xen/arch/arm/guestcopy.c
>>> +++ b/xen/arch/arm/guestcopy.c
>>> @@ -110,26 +110,34 @@ 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)
>>>   {
>>>       return copy_guest((void *)from, (vaddr_t)to, len,
>>> -                      GVA_INFO(current), COPY_to_guest | COPY_linear);
>>> +                      /* SAF-4-safe No persistent side effects */
>>> +                      GVA_INFO(current),
>>
>> I _still_ think this leaves ambiguity. The more that you need to look
>> up GVA_INFO() to recognize what this is about.
> 
> 
> Just to recap: here the point is that current reads a register with a 
> volatile asm, so the
> violation is in the expansion of GVA_INFO(current). Both GVA_INFO and current 
> taken alone
> are completely fine, so this is the only place where a SAF comment can be 
> placed.
> 
> The exapansion is:
> ((copy_info_t) { .gva = { ((*({ unsigned long __ptr; __asm__ ("" : 
> "=r"(__ptr) : "0"(&
>    per_cpu__curr_vcpu)); (typeof(&per_cpu__curr_vcpu)) (__ptr + (({ uint64_t 
> _r; asm volatile("mrs  %0, ""TPIDR_EL2" : "=r"
>    (_r)); _r; }))); }))) } }), (1U << 1) | (1U << 2));
> 
> My proposals are:
> 1) address the violation moving the current expansion outside (extra 
> variable);
> 2) put a more detailed comment to avoid the ambiguity;
> 3) use an ECL deviation for GVA_INFO(current).
> 
> Do you have any preference or proposal?

Imo 3 is not an option at all. Probably 1 wouldn't be too bad here, but
I still wouldn't like it (as matching a general pattern I try to avoid:
introducing local variables that are used just once and don't meaningfully
improve e.g. readability). Therefore out of what you list, 2 would remain.
But I'm not happy with a comment here either - as per above, there's
nothing that can go wrong here as long as there's only a single construct
with side effect(s).

>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -800,6 +800,7 @@ static int cf_check hvm_save_cpu_ctxt(struct vcpu *v, 
>>> hvm_domain_context_t *h)
>>>   {
>>>       struct segment_register seg;
>>>       struct hvm_hw_cpu ctxt = {
>>> +        /* SAF-3-safe effects for debugging/logging reasons are safe */
>>>           .tsc = hvm_get_guest_tsc_fixed(v, v->domain->arch.hvm.sync_tsc),
>>
>> A prereq for this imo is that the function take const struct vcpu *.
>> But I'm not sure that'll suffice. The function can change at any time,
>> rendering the comment here stale perhaps without anyone noticing.
>>
> 
> IMO It isn't a strict prereq, but it would make everything more clear.
> 
> In any case, apart adding the const, I do not see other easy solutions.
> Would you give me your ack if I change the function signature?

Well, as said: I'm not sure that'll suffice.

> Another possible solutions would be documenting the function in the new
> JSON file with a special attribute like only_debug_effect. Of course,
> this still requires keeping the JSON up to date in case of changes.

Exactly. So wouldn't really help.

In any event I'd like to ask that you consider splitting up this patch,
such that you won't need multiple acks for any of the parts. That'll
also allow focusing on one aspect at a time in reviews.

Jan