Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Nov 2024, Jan Beulich wrote:
> On 14.11.2024 03:34, Stefano Stabellini wrote:
> > On Wed, 13 Nov 2024, Jan Beulich wrote:
> >> On 13.11.2024 04:15, Stefano Stabellini wrote:
> >>> It is challenging to create a solution that satisfies everyone for this
> >>> patch. However, we should add R8.3 to the clean list as soon as possible
> >>> to enable rule blocking in GitLab-CI. Failing to do so risks introducing
> >>> regressions, as recently occurred, undoing the significant efforts made
> >>> by Bugseng and the community over the past year.
> >>>
> >>> Unless there is a specific counterproposal, let us proceed with
> >>> committing this patch.
> >>
> >> Well, I find this odd. We leave things sit in limbo for months and then
> >> want to go ahead with a controversial solution? Rather than actually
> >> (and finally) sorting out the underlying disagreement (of which there
> >> are actually two sufficiently separate parts)? Plus ...
> > 
> > The reason is that several MISRA regressions were recently introduced.
> > These regressions could have been easily detected by GitLab CI if we had
> > marked the rules as clean. I believe we should expedite accepting the
> > fixes and marking the rules as clean. We can always adjust the fixes or
> > deviations later to better suit our preferences. In my opinion, we
> > should prioritize marking the rules as clean.
> > 
> > 
> >>> On Mon, 24 Jun 2024, Jan Beulich wrote:
> >>>> On 21.06.2024 22:58, Andrew Cooper wrote:
> >>>>> Right now, the non-compat declaration and definition of do_multicall()
> >>>>> differing types for the nr_calls parameter.
> >>>>>
> >>>>> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for 
> >>>>> the
> >>>>> first 128bit architecture (RISC-V looks as if it might get there first).
> >>>>>
> >>>>> Worse, the type chosen here has a side effect of truncating the guest
> >>>>> parameter, because Xen still doesn't have a clean hypercall ABI 
> >>>>> definition.
> >>>>>
> >>>>> Switch uniformly to using unsigned long.
> >>>>
> >>>> And re-raising all the same question again: Why not uniformly unsigned 
> >>>> int?
> >>>> Or uint32_t?
> >>
> >> ... this question of mine effectively represents a concrete alternative
> >> proposal (or even two, if you like).
> >>
> >> The two parts where there appears to be disagreement are:
> >> 1) When to (not) use fixed width types, as presently outlined in
> >>    ./CODING_STYLE.
> >> 2) How to type C function parameters called solely from assembly code (of
> >>    which the hypercall handlers are a subset).
> >>
> >> And maybe
> >> 2b) How to best express such function parameters when they're (sometimes)
> >>     shared between native and compat handlers.
> >>
> >> Of course 2) is affected by, as Andrew validly says, there not being a
> >> formally clean ABI definition.
> >>
> >> My fear is that if this gets committed as is, it'll be used as a handle to
> >> force in further similarly questionable / controversial changes to other
> >> hypercall handlers. Which is why I think the controversy needs sorting out
> >> first (which admittedly is hard when the ABI is fuzzy).
> > 
> > While I appreciate your concern, as you know, aligning on the topics
> > above takes time. I do not believe it is in the interest of the
> > community, both contributors and reviewers, to delay marking this rule
> > as clean. Honestly, I do not mind how it gets marked as clean, as long
> > as we do it soon.
> > 
> > Additionally, please keep in mind that the Xen Project tends to have a
> > long memory. As a result, there is usually little risk of the so-called
> > "slippery slope" problem.
> > 
> > If you prefer a deviation I am OK with that too. I just want 8.3 as
> > clean :-) 
> 
> No, please no deviations when we can avoid them. Since it feels like it's
> always (going to be?) me to give in when there is such disagreement, why
> don't I do so here as well: Go ahead.

Thanks Jan, I really appreciate this