Re: [PATCH v4 1/4] xen/riscv: introduce asm/types.h header file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2023 12:23, Oleksii wrote:
> On Tue, 2023-01-17 at 11:04 +0100, Jan Beulich wrote:
>> On 17.01.2023 10:29, Oleksii wrote:
>>> On Mon, 2023-01-16 at 15:59 +0100, Jan Beulich wrote:
>>>> On 16.01.2023 15:39, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>>> ---
>>>>> Changes in V4:
>>>>>     - Clean up types in <asm/types.h> and remain only
>>>>> necessary.
>>>>>       The following types was removed as they are defined in
>>>>> <xen/types.h>:
>>>>>       {__|}{u|s}{8|16|32|64}
>>>>
>>>> For one you still typedef u32 and u64. And imo correctly so,
>>>> until we
>>>> get around to move the definition basic types into xen/types.h.
>>>> Plus
>>>> I can't see how things build for you: xen/types.h expects
>>>> __{u,s}<N>
>>> It builds because nothing is used <xen/types.h> now so that's why I
>>> missed them but you are right __{u,s}<N> should be backed.
>>> It looks like {__,}{u,s}{8,16,32} are the same for all available in
>>> Xen
>>> architectures so probably can I move them to <xen/types.h> instead
>>> of
>>> remain them in <asm/types.h>?
>>
>> This next step isn't quite as obvious, i.e. has room for being
>> contentious. In particular deriving fixed width types from C basic
>> types is setting us up for future problems (especially in the
>> context of RISC-V think of RV128). Therefore, if we touch and
>> generalize this, I'd like to sanitize things at the same time.
>>
>> I'd then prefer to typedef {u,}int<N>_t by using either the "mode"
>> attribute (requiring us to settle on a prereq of there always being
>> 8 bits per char) or the compiler supplied __{U,}INT<N>_TYPE__
>> (taking gcc 4.7 as a prereq; didn't check clang yet). Both would
>> allow {u,}int64_t to also be put in the common header. Yet if e.g.
>> a prereq assumption faced opposition, some other approach would
>> need to be found. Plus using either of the named approaches has
>> issues with the printf() format specifiers, for which I'm yet to
>> figure out a solution (or maybe someone else knows a good way to
>> deal with that; using compiler provided headers isn't an option
>> afaict, as gcc provides stdint.h but not inttypes.h, but maybe
>> glibc's simplistic approach is good enough - they're targeting
>> far more architectures than we do and get away with that).
>>
> Thanks for the explanation.
> 
> If back to RISCV's <asm/types.h> it looks that the v2 of the patch
> (https://lore.kernel.org/xen-devel/ca2674739cfa71cae0bf084a7b471ad4518026d3.1673278109.git.oleksii.kurochko@xxxxxxxxx/)
> is the best one option now because as I can see some work is going on
> around <xen/types.h> and keeping the minimal set of types now will
> allow us to not remove unneeded types for RISCV's port from asm/types.h
> in the future.

Well, as said before, I'd prefer if that header wasn't populated piecemeal,
but ...

> Even more based on your patch [
> https://lists.xen.org/archives/html/xen-devel/2023-01/msg00720.html ]
> RISCV's <asm/types.h> can be empty for this patch series.

... leaving it empty for now would of course be fine. Once the basic
fixed width types are needed, imo they ought to be populated all in one
got. But maybe by then we've managed to have that stuff in xen/types.h.

Jan