Re: [PATCH v2] docs/misra: document the expected sizes of integer types

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2024 02:46, Stefano Stabellini wrote:
> On Wed, 20 Mar 2024, Jan Beulich wrote:
>>> - the public interface is described in a C header so it makes sense for
>>>   the corresponding implementation to be in C
>>>
>>> - the C entry point is often both the entry point in C and also common
>>>   code
>>>
>>> - depending on the architecture, there is typically always some minimal
>>>   assembly entry code to prepare the environment before we can jump into
>>>   C-land; still one wouldn't consider those minimal and routine assembly
>>>   operations to be a meaningful hypercall entry point corresponding to
>>>   the C declaration in the public headers
>>>
>>> - as per MISRA and also general good practice, we need the declaration
>>>   in the public header files to match the definition in C
>>
>> Throughout, but especially with this last point, I feel there's confusion
>> (not sure on which side): There are no declarations of hypercall functions
>> in the public headers. Adding declarations there for the C entry points in
>> Xen would actually be wrong, as we don't provide such functions anywhere
>> (to consumers of the ABI).
> 
> I am copy/pasting text from sched.h:
> 
>  * The prototype for this hypercall is:
>  * ` long HYPERVISOR_sched_op(enum sched_op cmd, void *arg, ...)
>  *
>  * @cmd == SCHEDOP_??? (scheduler operation).
>  * @arg == Operation-specific extra argument(s), as described below.
>  * ...  == Additional Operation-specific extra arguments, described below.
>  *
> 
> from event_channel.h:
> 
>  * ` enum neg_errnoval
>  * ` HYPERVISOR_event_channel_op(enum event_channel_op cmd, void *args)
>  * `
>  * @cmd  == EVTCHNOP_* (event-channel operation).
>  * @args == struct evtchn_* Operation-specific extra arguments (NULL if none).
> 
> These are the hypercall declarations in public headers. Although they
> are comments, they are the only description of the ABI that we have (as
> far as I know). They are in C and use C types. 

>From their use of enum alone they don't qualify as declarations. They're
imo merely meant to provide minimal guidelines.

>>>>>>> We have two options:
>>>>>>>
>>>>>>> 1) we go with this document, and we clarify that even if we specify
>>>>>>>   "unsigned int", we actually mean a 32-bit integer
>>>>>>>
>>>>>>> 2) we change all our public ABIs and C hypercall entry points to use
>>>>>>>    fixed-size types (e.g. s/unsigned int/uint32_t/g)
>>>>>>>
>>>>>>> 2) is preferred because it is clearer but it is more work. So I went
>>>>>>> with 1). I also thought you would like 1) more.
>>>>>>
>>>>>> For ABIs (i.e. structures) we ought to be making that change anyway.
>>>>>> Leaving basic types in there is latently buggy.
>>>>>
>>>>> I am glad we agree :-)
>>>>>
>>>>> It is just that I also consinder the C hypercall entry points as part of
>>>>> the ABI
>>>>>
>>>>>
>>>>>> I'm happy to see a document like this added, for the purpose described
>>>>>> above. But to me 1) and 2) and largely independent of one another.
>>>>>
>>>>> Good that you are also happy with a document like this.
>>>>>
>>>>> The remaining question is: what about the rest of the C functions in Xen
>>>>> that are certainly not part of an ABI?
>>>>
>>>> As per above - anything internal isn't part of the ABI, C entry points
>>>> for hypercall handlers included. All we need to ensure is that we consume
>>>> the data according to what the ABI sets forth.
>>>
>>> It doesn't look like we'll convince one another on this point. But let
>>> me try another way.
>>>
>>> In my view, having mismatched types between declaration and definition
>>> and having non-fixed-width types in C hypercall entry points is really
>>> bad for a number of reasons, among them:
>>> - correctness
>>> - risk of ABI breakage
>>> - mismatch of declaration and definition
>>
>> What mismatches are you talking about? There's nothing mismatched now,
>> and there cannot be any mismatch, because the consumers of the ABI don't
>> call Xen functions directly.
> 
> Let me make an example:
> 
> - public header saying enum event_channel_op cmd
> - <assembly>
> - do_event_channel_op(int cmd, ...)
> 
> Do you think this is all good?
> 
> There are two pretty serious problems here:
> - enum and int are not the same type

See above. The issue I have with this is use of plain "int". Technically
that's not a problem either, but aiui we're aiming to use "unsigned int"
when negative values aren't possible.

And note that it was in 2012 when "int" there was changed to "enum", in an
effort to document things better.

> - enum and int are not fixed-width

Which I don't view as a problem, thanks to the assembly sitting in between.

> Don't you think it should be:
> 
> - public header saying uint32_t cmd in a comment
> - <assembly>
> - do_something_op(uint32_t cmd, ...)

The public header should say whatever is best suited to not misguide
people writing actual prototypes for their functions. I wouldn't mind
uint32_t being stated there. That has no influence whatsoever on
do_<something>_op(), though.

> Or possibly unsigned long depending on the parameter.

You're contradicting yourself: You mean to advocate for fixed-width types,
yet then you suggest "unsigned long". Perhaps because you realized that
there's no single fixed-width type fitting "unsigned long" for all
architectures. xen_ulong_t would likely come closest, but would - aiui -
still not be suitable for Arm32 when used in hypercall (handler)
prototypes; it's suitable for use (again) only in structure definitions.

Jan