Re: [PATCH v2 1/2] common: move Linux-inherited fixed width type decls to common header

[Julien Grall <julien@xxxxxxx>]

On 09/08/2023 11:58, Jan Beulich wrote:
> On 09.08.2023 12:49, Julien Grall wrote:
> > On 04/08/2023 07:08, Jan Beulich wrote:
> > > Have these in one place, for all architectures to use. Also use the C99
> > > types as the "original" ones, and derive the Linux compatible ones
> > > (which we're trying to phase out). For __s<N>, seeing that no uses exist
> > > anymore, move them to a new Linux compatibility header (as an act of
> > > precaution - as said, we don't have any uses of these types right now).
> > >
> > > In some Flask sources inclusion of asm/byteorder.h needs moving later.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > With one remark:
> >
> > Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> Thanks.
>
> > > --- a/xen/arch/arm/include/asm/types.h
> > > +++ b/xen/arch/arm/include/asm/types.h
> > > @@ -1,18 +1,7 @@
> > >    #ifndef __ARM_TYPES_H__
> > >    #define __ARM_TYPES_H__
> > >    
> > > -typedef signed char s8;
> > > -typedef unsigned char u8;
> > > -
> > > -typedef signed short s16;
> > > -typedef unsigned short u16;
> > > -
> > > -typedef signed int s32;
> > > -typedef unsigned int u32;
> > > -
> > >    #if defined(CONFIG_ARM_32)
> > > -typedef signed long long s64;
> > > -typedef unsigned long long u64;
> > >    typedef u32 vaddr_t;
> > >    #define PRIvaddr PRIx32
> > >    #if defined(CONFIG_PHYS_ADDR_T_32)
> > > @@ -34,9 +23,9 @@ typedef u64 paddr_t;
> > >    #endif
> > I find odd that you don't add a newline before/after #endif here but...
> >
> > >    typedef u32 register_t;
> > >    #define PRIregister "08x"
> > > -#elif defined (CONFIG_ARM_64)
> > > -typedef signed long s64;
> > > -typedef unsigned long u64;
> > > +
> > > +#elif defined(CONFIG_ARM_64)
> > > +
> > >    typedef u64 vaddr_t;
> > >    #define PRIvaddr PRIx64
> > >    typedef u64 paddr_t;
> > > @@ -44,6 +33,7 @@ typedef u64 paddr_t;
> > >    #define PRIpaddr "016lx"
> > >    typedef u64 register_t;
> > >    #define PRIregister "016lx"
> > > +
> > .. you add one here. But this change looks a bit unrelated and IMHO of
> > out context.
> >
> > >    #endif
> > >    
> > >    #endif /* __ARM_TYPES_H__ */
> Hmm, I think I was mistaking the blank line after
>
> #if defined(CONFIG_PHYS_ADDR_T_32)
>
> as the matching one. Which way would you prefer me to resolve the
> inconsistency: Add the missing blank line, or drop again the three ones
> I'm presently adding?
I would be ok with adding the missing blank line.

Cheers,

--
Julien Grall