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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.