| Message ID | 7c6aeedac626b171ed44df50ce5e3e2c76593f60.1706886631.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violation of MISRA C:2012 Rule 13.1 | expand |
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. > Function hvm_get_guest_tsc_fixed (indirectly) performs different side effects. > For example it calls hvm_get_guest_time_fixed that contains an ASSERT and calls > to spin_lock and spin_unlock. > > These side effects are not dangerous: they can be executed regardless of the > initializer list evaluation order > > This patch deviates violations using SAF commits caused by debug/logging macros and > functions. DYM "comments"? > --- a/xen/arch/arm/device.c > +++ b/xen/arch/arm/device.c > @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt, > .p2mt = p2mt, > .skip_mapping = !own_device || > (is_pci_passthrough_enabled() && > + /* SAF-3-safe effects for debugging/logging reasons are safe */ > (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)), Taking this just as example: I think the comment is too long. Just saying (leaving aside my comment higher up) "debugging/logging" would imo be sufficient. > --- 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. > --- 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. > --- a/xen/common/sched/core.c > +++ b/xen/common/sched/core.c > @@ -1521,6 +1521,7 @@ long vcpu_yield(void) > > SCHED_STAT_CRANK(vcpu_yield); > > + /* SAF-4-safe No persistent side effects */ > TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); > raise_softirq(SCHEDULE_SOFTIRQ); > return 0; > @@ -1899,6 +1900,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > break; > > + /* SAF-4-safe No persistent side effects */ > TRACE_3D(TRC_SCHED_SHUTDOWN, > current->domain->domain_id, current->vcpu_id, > sched_shutdown.reason); > @@ -1916,6 +1918,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > break; > > + /* SAF-4-safe No persistent side effects */ > TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, > d->domain_id, current->vcpu_id, sched_shutdown.reason); > For all of these iirc the suggestion was to latch current into a local variable (named "curr" by convention). Jan
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. > >> Function hvm_get_guest_tsc_fixed (indirectly) performs different side effects. >> For example it calls hvm_get_guest_time_fixed that contains an ASSERT and calls >> to spin_lock and spin_unlock. >> >> These side effects are not dangerous: they can be executed regardless of the >> initializer list evaluation order >> >> This patch deviates violations using SAF commits caused by debug/logging macros and >> functions. > > DYM "comments"? > Oh yes, sorry. >> --- a/xen/arch/arm/device.c >> +++ b/xen/arch/arm/device.c >> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt, >> .p2mt = p2mt, >> .skip_mapping = !own_device || >> (is_pci_passthrough_enabled() && >> + /* SAF-3-safe effects for debugging/logging reasons are safe */ >> (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)), > > Taking this just as example: I think the comment is too long. Just > saying (leaving aside my comment higher up) "debugging/logging" > would imo be sufficient. > Ok. >> --- 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? >> --- 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? 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. >> --- a/xen/common/sched/core.c >> +++ b/xen/common/sched/core.c >> @@ -1521,6 +1521,7 @@ long vcpu_yield(void) >> >> SCHED_STAT_CRANK(vcpu_yield); >> >> + /* SAF-4-safe No persistent side effects */ >> TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >> raise_softirq(SCHEDULE_SOFTIRQ); >> return 0; >> @@ -1899,6 +1900,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( copy_from_guest(&sched_shutdown, arg, 1) ) >> break; >> >> + /* SAF-4-safe No persistent side effects */ >> TRACE_3D(TRC_SCHED_SHUTDOWN, >> current->domain->domain_id, current->vcpu_id, >> sched_shutdown.reason); >> @@ -1916,6 +1918,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( copy_from_guest(&sched_shutdown, arg, 1) ) >> break; >> >> + /* SAF-4-safe No persistent side effects */ >> TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, >> d->domain_id, current->vcpu_id, sched_shutdown.reason); >> > > For all of these iirc the suggestion was to latch current into a local > variable (named "curr" by convention). > Ok. > Jan
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
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. > The primary purpose of my comment here was that we need to please > separate debugging from logging side effects. > Ok, I will work in that sense. >>>> --- 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/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. > Ok, but please consider that the JSON file has been precisely added to deal with these cases (avoiding __attribute__). If we are scared to use it, it becomes meaningless. @Stefano maybe your opinion could help. Another more complex option would be emulating __attribute__ (in general any property) with a comment: /* @attribute: [only_debug_effects] int foo(void); This solution implies that: - we need a script to convert these comments in ECL configurations (the script should parse the entire codebase); - regexes cannot be used (the JSON instead accepts regexes); - to be coherent, all properties that actually lives in the JSON should be moved inside these comments. This means a lot of changes. A hybrid approach is also possible. > Jan
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
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: >>>>>> 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 > 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). Maybe it is better to re-discuss the rule adoption during the next meeting?
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
On Fri, 9 Feb 2024, Jan Beulich wrote: > 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? Yes. I think Simone might be right: it looks like the cases we care about the most are the ones covered by 13.2, which appear to be a subset of the ones covered by 13.1. Assuming we accept 13.2, the question is whether there is anything left in 13.1 that is of interest. We can discuss during the next MISRA call.
On Wed, 7 Feb 2024, 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. > > > The primary purpose of my comment here was that we need to please > > separate debugging from logging side effects. > > > > Ok, I will work in that sense. > > > > > > --- 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/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. > > > > Ok, but please consider that the JSON file has been precisely added to deal > with these cases (avoiding __attribute__). If we are scared to use it, it > becomes > meaningless. > > @Stefano maybe your opinion could help. I do think the JSON file is a useful tool in our toolbox. It is also fair to say that it is one that we should be careful about using as it requires manual updates from time to time. So, I certainly consider it an option, but I prefer your other suggestion of dropping 13.1 in favor of 13.2. With that change, would the need for a deviation in this function go away?
On 09/02/24 23:13, Stefano Stabellini wrote: > On Wed, 7 Feb 2024, 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. >> >>> The primary purpose of my comment here was that we need to please >>> separate debugging from logging side effects. >>> >> >> Ok, I will work in that sense. >> >>>>>> --- 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/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. >>> >> >> Ok, but please consider that the JSON file has been precisely added to deal >> with these cases (avoiding __attribute__). If we are scared to use it, it >> becomes >> meaningless. >> >> @Stefano maybe your opinion could help. > > I do think the JSON file is a useful tool in our toolbox. It is also > fair to say that it is one that we should be careful about using as it > requires manual updates from time to time. > > So, I certainly consider it an option, but I prefer your other > suggestion of dropping 13.1 in favor of 13.2. With that change, would > the need for a deviation in this function go away? > This is only a violation of 13.1, so dropping it in favour of 13.2 it's enough.
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index 952324f85c..5539e8dfda 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -28,6 +28,22 @@ }, { "id": "SAF-3-safe", + "analyser": { + "eclair": "MC3R1.R13.1" + }, + "name": "MC3R1.R13.1: effects for debugging and logging", + "text": "Effects for debugging and loggings reasons that crash execution or produce logs are allowed in initializer lists. The evaluation order in abnormal conditions is not relevant." + }, + { + "id": "SAF-4-safe", + "analyser": { + "eclair": "MC3R1.R13.1" + }, + "name": "MC3R1.R13.1: volatile asm statements that do not perform any persistent side effect", + "text": "Volatile asm statement in an initializer list that does not perform persistent side effects is safe." + }, + { + "id": "SAF-5-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index 1f631d3274..fa331f164d 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt, .p2mt = p2mt, .skip_mapping = !own_device || (is_pci_passthrough_enabled() && + /* SAF-3-safe effects for debugging/logging reasons are safe */ (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)), .iomem_ranges = iomem_ranges, .irq_ranges = irq_ranges diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c index 6716b03561..b75538252a 100644 --- 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), + COPY_to_guest | COPY_linear); } unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from, unsigned int len) { - return copy_guest((void *)from, (vaddr_t)to, len, GVA_INFO(current), + return copy_guest((void *)from, (vaddr_t)to, len, + /* SAF-4-safe No persistent side effects */ + GVA_INFO(current), COPY_to_guest | COPY_flush_dcache | COPY_linear); } unsigned long raw_clear_guest(void *to, unsigned int len) { - return copy_guest(NULL, (vaddr_t)to, len, GVA_INFO(current), + return copy_guest(NULL, (vaddr_t)to, len, + /* SAF-4-safe No persistent side effects */ + GVA_INFO(current), COPY_to_guest | COPY_linear); } unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned int len) { - return copy_guest(to, (vaddr_t)from, len, GVA_INFO(current), + return copy_guest(to, (vaddr_t)from, len, + /* SAF-4-safe No persistent side effects */ + GVA_INFO(current), COPY_from_guest | COPY_linear); } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index e8deeb0222..19322fbb56 100644 --- 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), .msr_tsc_aux = v->arch.msrs->tsc_aux, .rax = v->arch.user_regs.rax, diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index d177c675c8..9e973fcf31 100644 --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -1521,6 +1521,7 @@ long vcpu_yield(void) SCHED_STAT_CRANK(vcpu_yield); + /* SAF-4-safe No persistent side effects */ TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); raise_softirq(SCHEDULE_SOFTIRQ); return 0; @@ -1899,6 +1900,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( copy_from_guest(&sched_shutdown, arg, 1) ) break; + /* SAF-4-safe No persistent side effects */ TRACE_3D(TRC_SCHED_SHUTDOWN, current->domain->domain_id, current->vcpu_id, sched_shutdown.reason); @@ -1916,6 +1918,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( copy_from_guest(&sched_shutdown, arg, 1) ) break; + /* SAF-4-safe No persistent side effects */ TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, d->domain_id, current->vcpu_id, sched_shutdown.reason);