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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2024 13:21, Simone Ballarin wrote:
> On 07/02/24 11:24, Jan Beulich wrote:
>> 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.
> 
> I agree for the safe cases such as single item list initializers 
> (independently
> by the number of effect contained in io_apic_read).
> In fact, I was about to propose in another patch to deviate cases like:
> 
> union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
> union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
> 
>> 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).
>>
> 
> I don't completely agree on that, this requires an in-depth comprehension
> of the code especially when complex call chains are involved. Moreover
> these deviations need to be maintained when one of the function involved 
> changes.

Right, and I didn't really mean multiple function calls here, but e.g.
multiple ++ or --.

>>>>> --- 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).
>>
> So, would be changing the SAF in:
> /* SAF-<new_id>-safe single item initializer */
> 
> OK for you?

A comment, as said, is only the least bad of what you did enumerate. But
for this code in particular I'm not a maintainer anyway, so it's not me
you need to convince. I'm taking this only as an example for discussing
underlying aspects.

Jan