| Message ID | 20210224102641.89455-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hvmloader: drop usage of system headers | expand |
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__. > +#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. > +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__))). > +#define bool _Bool > +#define true 1 > +#define false 0 > +#define __bool_true_false_are_defined 1 > + > +typedef __builtin_va_list va_list; > +#define va_copy(dest, src) __builtin_va_copy((dest), (src)) > +#define va_start(ap, last) __builtin_va_start((ap), (last)) Nit: Perhaps better omit the unnecessary inner parentheses? Jan
Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"):
> 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__.
This code is only ever going to be for 32-bit x86, so I think the way
Roger did it is fine.
Doing it the other way, to cope with this file being used with
compiler settings where the above set of types is wrong, would also
imply more complex definitions of INT32_MIN et al.
> > +#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 disagree. Providing the full set now gets them all properly
reviewe and reduces the burden on future work.
> > +typedef uint32_t size_t;
I would be inclined to provide ssize_t too but maybe hvmloader will
never need it.
> Like the hypervisor, we should prefer using __SIZE_TYPE__
> when available.
I disagree.
> > +typedef uint32_t uintptr_t;
>
> Again - use __UINTPTR_TYPE__ or, like Xen,
> __attribute__((__mode__(__pointer__))).
I disagree.
> > +#define bool _Bool
> > +#define true 1
> > +#define false 0
> > +#define __bool_true_false_are_defined 1
> > +
> > +typedef __builtin_va_list va_list;
> > +#define va_copy(dest, src) __builtin_va_copy((dest), (src))
> > +#define va_start(ap, last) __builtin_va_start((ap), (last))
>
> Nit: Perhaps better omit the unnecessary inner parentheses?
We should definitely keep the inner parentheses. I don't want to
start carefully reasoning about precisely which inner parentheses are
necesary for macro argument parsing correctness.
Ian.
On 24.02.2021 12:07, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"): >> 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__. > > This code is only ever going to be for 32-bit x86, so I think the way > Roger did it is fine. It is technically correct at this point in time, from all we can tell. I can't see any reason though why a compiler might not support wider int or, in particular, long long. hvmloader, unlike most of the rest of the tools, is a freestanding binary and hence not tied to any particular ABI the compiler used may have been built for. > Doing it the other way, to cope with this file being used with > compiler settings where the above set of types is wrong, would also > imply more complex definitions of INT32_MIN et al. Well, that's only as far as the use of number suffixes goes. The values used won't change, as these constants describe fixed width types. >>> +typedef uint32_t size_t; > > I would be inclined to provide ssize_t too but maybe hvmloader will > never need it. > >> Like the hypervisor, we should prefer using __SIZE_TYPE__ >> when available. > > I disagree. May I ask why? There is a reason providing of these types did get added to (at least) gcc. One argument against this would be above mentioned independence on any ABI the compiler would be built for, but I'd buy that only if above we indeed used __attribute__((__mode__())), as that's the only way to achieve such independence. IOW imo if we stick to what is there now for {,u}int<N>_t, we should use __SIZE_TYPE__ here. If we used the mode attribute approach there, using uint32_t here would indeed be better. >>> +typedef uint32_t uintptr_t; >> >> Again - use __UINTPTR_TYPE__ or, like Xen, >> __attribute__((__mode__(__pointer__))). > > I disagree. The same question / considerations apply here then. >>> +#define bool _Bool >>> +#define true 1 >>> +#define false 0 >>> +#define __bool_true_false_are_defined 1 >>> + >>> +typedef __builtin_va_list va_list; >>> +#define va_copy(dest, src) __builtin_va_copy((dest), (src)) >>> +#define va_start(ap, last) __builtin_va_start((ap), (last)) >> >> Nit: Perhaps better omit the unnecessary inner parentheses? > > We should definitely keep the inner parentheses. I don't want to > start carefully reasoning about precisely which inner parentheses are > necesary for macro argument parsing correctness. Can you give me an example of when the inner parentheses would be needed? I don't think they're needed no matter whether (taking the example here) __builtin_va_...() were functions or macros. They would of course be needed if the identifiers were part of expressions beyond the mere function invocation. We've been trying to eliminate such in the hypervisor part of the tree, and since hvmloader is more closely related to the hypervisor than the tools (see also its maintainership), I think we would want to do so here, too. But of course if there are cases where such parentheses are really needed, we'd want (need) to change our approach in hypervisor code as well. The primary reason why I've been advocating to avoid them is that, as long as they're not needed for anything, they harm readability and increase the risk of mistakes like the one that had led to XSA-316. Jan
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.
On 24/02/2021 10:26, Roger Pau Monne wrote: > Instead create a private types.h header that contains the set of types > that are required by hvmloader. Replace include occurrences of std* > headers with type.h. Note that including types.h directly is not > required in util.c because it already includes util.h which in turn > includes the newly created types.h. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> At what point do we just declare Alpine broken? These are all freestanding headers, an explicitly available for use, in the way we use them. There are substantial portability costs for making changes like this, which takes us from standards compliant C to GCC-isms-only. ~Andrew
On Wed, Feb 24, 2021 at 12:11:45PM +0000, Andrew Cooper wrote: > On 24/02/2021 10:26, Roger Pau Monne wrote: > > Instead create a private types.h header that contains the set of types > > that are required by hvmloader. Replace include occurrences of std* > > headers with type.h. Note that including types.h directly is not > > required in util.c because it already includes util.h which in turn > > includes the newly created types.h. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > At what point do we just declare Alpine broken? These are all > freestanding headers, an explicitly available for use, in the way we use > them. The headers are available in Alpine, but they seem to be specifically tied to the native bitness of the OS, which is causing us the issues. So they are freestanding AFAICT, it's just that the bitness they use is not portable. Thanks, Roger.
On 24.02.2021 13:01, Roger Pau Monné wrote: > 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. Oh, these modes have been available even in gcc 3.x. I thought Clang was claiming to be 4.4(?) compatible. > 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__. Fair point. And they're available from 4.5 onwards only. So just __SIZE_TYPE__ has been available for long enough. As said in reply to Ian I think we at least want to use that one (and I guess in the hypervisor we may want to drop the fallback). Jan
On 24/02/2021 11:07, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"): >> Like the hypervisor, we should prefer using __SIZE_TYPE__ >> when available. > I disagree. size_t is obnoxious in the C spec. It might not be the largest native word size on the processor, and in some 64bit environments, it really is 32 bits. It cannot be correctly derived from a standard type, and must come from the compiler, because it critical that it matches what the compiler generates for the sizeof operator. Posix being fairly sane prohibits environments where the maximum object size is smaller than the address size, which is why aliasing it to unsigned long works in the common case. ~Andrew
Andrew Cooper writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"):
> At what point do we just declare Alpine broken? These are all
> freestanding headers, an explicitly available for use, in the way we use
> them.
There is IMO nothing wrong with Alpine here. Alpine amd64 simply does
not support compilation of 32-bit x86 userland binaries.
But that's OK for us. Xen doe not require the execution of any 32-bit
userland binaries. hvmloader is not a userland binary.
As Roger said on irc
13:35 <royger> but requiring a compiler that supports generating
i386 code doens't imply that we also have a libc for it?
> There are substantial portability costs for making changes like this,
> which takes us from standards compliant C to GCC-isms-only.
Since we are defining our out standalone environment for hvmloader, we
are in the position of the C *implementor*. Compilers have features
(like __builtin_va*) that are helpful for implementing standard C
features like stdarg.h and indeed stdint.h.
Or to put it another way, GCC does not, by itself, provide (in
Standard C terms) a "freestanding implementation". Arguably GCC ought
to provide stdint.h et al but in practice it doing so causes more
trouble as it gets in the way of the implentors of hosted
implementations.
The conclusion is simply that we must provide for ourselves any
apsects of a "freestanding implementation" that we care about. (And
then we get to decide for ourselves how much the internal API should
look like Standard C. Defining the Standard C type names is
definitely IMO advisable as it makes the bulk of the code sensible to
read.)
Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"):
> On 24.02.2021 12:07, Ian Jackson wrote:
> > This code is only ever going to be for 32-bit x86, so I think the way
> > Roger did it is fine.
>
> It is technically correct at this point in time, from all we can
> tell. I can't see any reason though why a compiler might not
> support wider int or, in particular, long long.
Our requirement for hvmloader is that we have an ILP32 LL64 compiler
which generates 32-bit x86 machine code. That is what "gcc -m32"
means. Whether future compiler(s) might exist which can provide ILP32
LLP64 (and what type uint64_t is on such a compiler) is not relevant.
> > Doing it the other way, to cope with this file being used with
> > compiler settings where the above set of types is wrong, would also
> > imply more complex definitions of INT32_MIN et al.
>
> Well, that's only as far as the use of number suffixes goes. The
> values used won't change, as these constants describe fixed width
> types.
So the definitions would need to contain casts.
> >> Like the hypervisor, we should prefer using __SIZE_TYPE__
> >> when available.
> >
> > I disagree.
>
> May I ask why? There is a reason providing of these types did get
> added to (at least) gcc.
__SIZE_TYPE__ is provided by the compiler to the libc implementor. It
is one of those facilities like __builtin_va*. The bulk of the code
in hvmloader should not use this kind of thing. It should use plain
size_t.
As for the new header in hvmloader, it does not matter whether it uses
__SIZE_TYPE__ or some other type which is known to be 32-bit, since
this code is definitely only ever for 32-bit x86.
> One argument against this would be above mentioned independence
> on any ABI the compiler would be built for, but I'd buy that only
> if above we indeed used __attribute__((__mode__())), as that's
> the only way to achieve such independence.
We don't want or need to support building hvmloader with a differnet
ABI.
> >> Nit: Perhaps better omit the unnecessary inner parentheses?
> >
> > We should definitely keep the inner parentheses. I don't want to
> > start carefully reasoning about precisely which inner parentheses are
> > necesary for macro argument parsing correctness.
>
> Can you give me an example of when the inner parentheses would be
> needed? I don't think they're needed no matter whether (taking the
> example here) __builtin_va_...() were functions or macros. They
> would of course be needed if the identifiers were part of
> expressions beyond the mere function invocation.
You mention the situation where the parentheses would be needed
yourself.
> We've been trying to eliminate such in the hypervisor part of the
> tree,
Really ? Well I think that is in exceedingly poor taste. I can't
claim that it's objectively wrong and this is a question of style so I
won't insist on it.
> The primary reason why I've been advocating to avoid them is that,
> as long as they're not needed for anything, they harm readability
> and increase the risk of mistakes like the one that had led to
> XSA-316.
I looked again at XSA-316. I don't want to open another can of worms.
It will suffice to say that I don't share your view on the root cause.
Ian.
On 24.02.2021 15:33, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"): >> At what point do we just declare Alpine broken? These are all >> freestanding headers, an explicitly available for use, in the way we use >> them. > > There is IMO nothing wrong with Alpine here. Alpine amd64 simply does > not support compilation of 32-bit x86 userland binaries. > > But that's OK for us. Xen doe not require the execution of any 32-bit > userland binaries. hvmloader is not a userland binary. > > As Roger said on irc > > 13:35 <royger> but requiring a compiler that supports generating > i386 code doens't imply that we also have a libc for it? > >> There are substantial portability costs for making changes like this, >> which takes us from standards compliant C to GCC-isms-only. > > Since we are defining our out standalone environment for hvmloader, we > are in the position of the C *implementor*. Compilers have features > (like __builtin_va*) that are helpful for implementing standard C > features like stdarg.h and indeed stdint.h. > > Or to put it another way, GCC does not, by itself, provide (in > Standard C terms) a "freestanding implementation". Arguably GCC ought > to provide stdint.h et al but in practice it doing so causes more > trouble as it gets in the way of the implentors of hosted > implementations. But gcc _does_ provide a stdint.h. > Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"): >> On 24.02.2021 12:07, Ian Jackson wrote: >>> This code is only ever going to be for 32-bit x86, so I think the way >>> Roger did it is fine. >> >> It is technically correct at this point in time, from all we can >> tell. I can't see any reason though why a compiler might not >> support wider int or, in particular, long long. > > Our requirement for hvmloader is that we have an ILP32 LL64 compiler > which generates 32-bit x86 machine code. That is what "gcc -m32" > means. I'm not sure about the last statement; I'm pretty sure we don't check that we have such a compiler (in tools/configure). > Whether future compiler(s) might exist which can provide ILP32 > LLP64 (and what type uint64_t is on such a compiler) is not relevant. > >>> Doing it the other way, to cope with this file being used with >>> compiler settings where the above set of types is wrong, would also >>> imply more complex definitions of INT32_MIN et al. >> >> Well, that's only as far as the use of number suffixes goes. The >> values used won't change, as these constants describe fixed width >> types. > > So the definitions would need to contain casts. Which they can't, as that would make them unusable in preprocessor directives. >>>> Like the hypervisor, we should prefer using __SIZE_TYPE__ >>>> when available. >>> >>> I disagree. >> >> May I ask why? There is a reason providing of these types did get >> added to (at least) gcc. > > __SIZE_TYPE__ is provided by the compiler to the libc implementor. It > is one of those facilities like __builtin_va*. The bulk of the code > in hvmloader should not use this kind of thing. It should use plain > size_t. > > As for the new header in hvmloader, it does not matter whether it uses > __SIZE_TYPE__ or some other type which is known to be 32-bit, since > this code is definitely only ever for 32-bit x86. From a compiler perspective, "32-bit" and "x86" needs further pairing with an OS, as it's the OS which defines the ABI. This is why further up I did say "It is technically correct at this point in time, from all we can tell" - we imply that all OSes we want to be able to build on provide a suitable ABI, so we can use their compilers. >> One argument against this would be above mentioned independence >> on any ABI the compiler would be built for, but I'd buy that only >> if above we indeed used __attribute__((__mode__())), as that's >> the only way to achieve such independence. > > We don't want or need to support building hvmloader with a differnet > ABI. > >>>> Nit: Perhaps better omit the unnecessary inner parentheses? >>> >>> We should definitely keep the inner parentheses. I don't want to >>> start carefully reasoning about precisely which inner parentheses are >>> necesary for macro argument parsing correctness. >> >> Can you give me an example of when the inner parentheses would be >> needed? I don't think they're needed no matter whether (taking the >> example here) __builtin_va_...() were functions or macros. They >> would of course be needed if the identifiers were part of >> expressions beyond the mere function invocation. > > You mention the situation where the parentheses would be needed > yourself. Okay, if that would have been your example, then since there are no further expressions involved here you agree parentheses aren't needed here? JAn
diff --git a/tools/firmware/hvmloader/32bitbios_support.c b/tools/firmware/hvmloader/32bitbios_support.c index e726946a7b..32b5c4c4ad 100644 --- a/tools/firmware/hvmloader/32bitbios_support.c +++ b/tools/firmware/hvmloader/32bitbios_support.c @@ -20,7 +20,7 @@ * this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <inttypes.h> +#include "types.h" #include <xen/libelf/elfstructs.h> #ifdef __sun__ #include <sys/machelf.h> diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h index 844120bc87..510d5b5c79 100644 --- a/tools/firmware/hvmloader/config.h +++ b/tools/firmware/hvmloader/config.h @@ -1,8 +1,7 @@ #ifndef __HVMLOADER_CONFIG_H__ #define __HVMLOADER_CONFIG_H__ -#include <stdint.h> -#include <stdbool.h> +#include "types.h" enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt }; extern enum virtual_vga virtual_vga; diff --git a/tools/firmware/hvmloader/hypercall.h b/tools/firmware/hvmloader/hypercall.h index 5368c30720..788f699565 100644 --- a/tools/firmware/hvmloader/hypercall.h +++ b/tools/firmware/hvmloader/hypercall.h @@ -31,7 +31,7 @@ #ifndef __HVMLOADER_HYPERCALL_H__ #define __HVMLOADER_HYPERCALL_H__ -#include <stdint.h> +#include "types.h" #include <xen/xen.h> #include "config.h" diff --git a/tools/firmware/hvmloader/mp_tables.c b/tools/firmware/hvmloader/mp_tables.c index d207ecbf00..76790a9a1e 100644 --- a/tools/firmware/hvmloader/mp_tables.c +++ b/tools/firmware/hvmloader/mp_tables.c @@ -27,7 +27,7 @@ * this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <stdint.h> +#include "types.h" #include "config.h" /* number of non-processor MP table entries */ diff --git a/tools/firmware/hvmloader/option_rom.h b/tools/firmware/hvmloader/option_rom.h index 0fefe0812a..7988aa29ec 100644 --- a/tools/firmware/hvmloader/option_rom.h +++ b/tools/firmware/hvmloader/option_rom.h @@ -1,7 +1,7 @@ #ifndef __HVMLOADER_OPTION_ROM_H__ #define __HVMLOADER_OPTION_ROM_H__ -#include <stdint.h> +#include "types.h" struct option_rom_header { uint8_t signature[2]; /* "\x55\xaa" */ diff --git a/tools/firmware/hvmloader/pir_types.h b/tools/firmware/hvmloader/pir_types.h index 9f9259c2e1..9efcdcf94b 100644 --- a/tools/firmware/hvmloader/pir_types.h +++ b/tools/firmware/hvmloader/pir_types.h @@ -23,7 +23,7 @@ #ifndef PIR_TYPES_H #define PIR_TYPES_H -#include <stdint.h> +#include "types.h" #define NR_PIR_SLOTS 6 diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c index 97a054e9e3..5821c85c30 100644 --- a/tools/firmware/hvmloader/smbios.c +++ b/tools/firmware/hvmloader/smbios.c @@ -19,7 +19,7 @@ * Authors: Andrew D. Ball <aball@us.ibm.com> */ -#include <stdint.h> +#include "types.h" #include <xen/xen.h> #include <xen/version.h> #include "smbios_types.h" diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h index 7c648ece71..439c3fb247 100644 --- a/tools/firmware/hvmloader/smbios_types.h +++ b/tools/firmware/hvmloader/smbios_types.h @@ -25,7 +25,7 @@ #ifndef SMBIOS_TYPES_H #define SMBIOS_TYPES_H -#include <stdint.h> +#include "types.h" /* SMBIOS entry point -- must be written to a 16-bit aligned address between 0xf0000 and 0xfffff. diff --git a/tools/firmware/hvmloader/types.h b/tools/firmware/hvmloader/types.h new file mode 100644 index 0000000000..3d765f2c60 --- /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; + +#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 + +typedef uint32_t size_t; +typedef uint32_t uintptr_t; + +#define UINTPTR_MAX UINT32_MAX + +#define bool _Bool +#define true 1 +#define false 0 +#define __bool_true_false_are_defined 1 + +typedef __builtin_va_list va_list; +#define va_copy(dest, src) __builtin_va_copy((dest), (src)) +#define va_start(ap, last) __builtin_va_start((ap), (last)) +#define va_end(ap) __builtin_va_end(ap) +#define va_arg __builtin_va_arg + +#endif diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index 7da144b0bb..2df84482ab 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -24,7 +24,6 @@ #include "vnuma.h" #include <acpi2_0.h> #include <libacpi.h> -#include <stdint.h> #include <xen/xen.h> #include <xen/memory.h> #include <xen/sched.h> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 4f0baade0e..285a1d23c4 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -1,10 +1,7 @@ #ifndef __HVMLOADER_UTIL_H__ #define __HVMLOADER_UTIL_H__ -#include <stdarg.h> -#include <stdint.h> -#include <stddef.h> -#include <stdbool.h> +#include "types.h" #include <xen/xen.h> #include <xen/hvm/hvm_info_table.h> #include "e820.h"
Instead create a private types.h header that contains the set of types that are required by hvmloader. Replace include occurrences of std* headers with type.h. Note that including types.h directly is not required in util.c because it already includes util.h which in turn includes the newly created types.h. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/firmware/hvmloader/32bitbios_support.c | 2 +- tools/firmware/hvmloader/config.h | 3 +- tools/firmware/hvmloader/hypercall.h | 2 +- tools/firmware/hvmloader/mp_tables.c | 2 +- tools/firmware/hvmloader/option_rom.h | 2 +- tools/firmware/hvmloader/pir_types.h | 2 +- tools/firmware/hvmloader/smbios.c | 2 +- tools/firmware/hvmloader/smbios_types.h | 2 +- tools/firmware/hvmloader/types.h | 47 ++++++++++++++++++++ tools/firmware/hvmloader/util.c | 1 - tools/firmware/hvmloader/util.h | 5 +-- 11 files changed, 56 insertions(+), 14 deletions(-) create mode 100644 tools/firmware/hvmloader/types.h