Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 24, 2021 at 11:51:39AM +0100, Jan Beulich wrote:
> On 24.02.2021 11:26, Roger Pau Monne wrote:
> > --- /dev/null
> > +++ b/tools/firmware/hvmloader/types.h
> > @@ -0,0 +1,47 @@
> > +#ifndef _HVMLOADER_TYPES_H_
> > +#define _HVMLOADER_TYPES_H_
> > +
> > +typedef unsigned char uint8_t;
> > +typedef signed char int8_t;
> > +
> > +typedef unsigned short uint16_t;
> > +typedef signed short int16_t;
> > +
> > +typedef unsigned int uint32_t;
> > +typedef signed int int32_t;
> > +
> > +typedef unsigned long long uint64_t;
> > +typedef signed long long int64_t;
> 
> I wonder if we weren't better of not making assumptions on
> short / int / long long, and instead use
> __attribute__((__mode__(...))) or, if available, the compiler
> provided __{,U}INT*_TYPE__.

Oh, didn't know about all this fancy stuff.

Clang doens't seem to support the same mode attributes, for example
QImode is unknown, and that's just on one version of clang that I
happened to test on.

Using __{,U}INT*_TYPE__ does seem to be supported on the clang version
I've tested with, so that might be an option if it's supported
everywhere we care about. If we still need to keep the current typedef
chunk for fallback purposes then I see no real usefulness of using
__{,U}INT*_TYPE__.

> > +#define INT8_MIN        (-0x7f-1)
> > +#define INT16_MIN       (-0x7fff-1)
> > +#define INT32_MIN       (-0x7fffffff-1)
> > +#define INT64_MIN       (-0x7fffffffffffffffll-1)
> > +
> > +#define INT8_MAX        0x7f
> > +#define INT16_MAX       0x7fff
> > +#define INT32_MAX       0x7fffffff
> > +#define INT64_MAX       0x7fffffffffffffffll
> > +
> > +#define UINT8_MAX       0xff
> > +#define UINT16_MAX      0xffff
> > +#define UINT32_MAX      0xffffffffu
> > +#define UINT64_MAX      0xffffffffffffffffull
> 
> At least if going the above outlined route, I think we'd then
> also be better off not #define-ing any of these which we don't
> really use. Afaics it's really only UINTPTR_MAX which needs
> providing.

I've assumed that for consistency we would like to provide those
already. I can switch them to using __{U}INT*_{MAX,MIN}__ if we agree
that it's supported on all compilers we care about, but I would rather
not drop them. I think those might be useful in the future, and having
them already does no harm.

> > +typedef uint32_t size_t;
> 
> Like the hypervisor, we should prefer using __SIZE_TYPE__
> when available.
> 
> > +typedef uint32_t uintptr_t;
> 
> Again - use __UINTPTR_TYPE__ or, like Xen,
> __attribute__((__mode__(__pointer__))).

Let me run a gitlab test suite using the __{,U}INT*_TYPE__ stuff and
if that's fine everywhere we test then I think we can go for that if
you prefer over the current proposal?

I still think that coding them like I've done above should be fine as
we don't expect hvmloader to ever be built in a mode different than
i386?

Thanks, Roger.