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

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

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.

--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)