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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2024 10:25, Simone Ballarin wrote:
> On 07/02/24 13:40, Jan Beulich wrote:
>> 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:
>>>>>>> --- 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.
> 
> I was generally thinking about the comments of this series, and I've
> just realised that many of them can be summarized by the following sentence:
> 
> "We do not want changes to address violations of R13.1 that are not also 
> violations
> of R13.2"
> 
> MC3R1.R13.2   rule    The value of an expression and its persistent side 
> effects shall be the same under all permitted evaluation orders
> MC3R1.R13.1   rule    Initializer lists shall not contain persistent side 
> effects
> 
> At this point, my proposal is to remove R13.1 from the coding standard and add
> R13.2 (eventually limiting its scope to initializer lists).

I'm afraid I don't understand the "eventually limiting" part.

> Maybe it is better to re-discuss the rule adoption during the next meeting?

Perhaps best. Stefano, could you take note of this?

Jan