[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v4 1/4] xen: add SAF deviation for debugging and logging effects
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |