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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan