[for-4.13] xen: Drop bogus BOOLEAN definitions, TRUE and FALSE
diff mbox series

Message ID 20191111202443.7154-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • [for-4.13] xen: Drop bogus BOOLEAN definitions, TRUE and FALSE
Related show

Commit Message

Andrew Cooper Nov. 11, 2019, 8:24 p.m. UTC
actypes.h and efidef.h both define BOOLEAN as unsigned char, which is buggy in
combination with logic such as "BOOLEAN b = (a & 0x100);"  Redefine BOOLEAN as
bool instead, which doesn't truncate.

Both also define TRUE and FALSE, with actypes.h being extra rude and replacing
whatever exists thus far.  Drop all uses of TRUE and FALSE, replacing them
with true/false respectively, and drop the declarations.

Also drop the pointless conditional declaration of NULL while cleaning this
up.

Finally, correct all the comments which which were found by sed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Juergen Gross <jgross@suse.com>

This is based on top of Rogers patch adjusting part of io_apic.c

Compile tested on ARM, fully texted on x86.

RFC for 4.13 - I thought I'd got all of the boolean truncation bugs back in
4.8 but clearly not...
---
 xen/arch/x86/io_apic.c               | 12 ++++++------
 xen/arch/x86/x86_64/mm.c             |  2 +-
 xen/common/kexec.c                   |  6 +++---
 xen/common/timer.c                   |  4 ++--
 xen/drivers/acpi/tables/tbfadt.c     |  4 ++--
 xen/drivers/passthrough/vtd/utils.c  |  2 +-
 xen/include/acpi/acconfig.h          |  2 +-
 xen/include/acpi/actypes.h           | 20 ++------------------
 xen/include/asm-arm/arm64/efibind.h  |  2 +-
 xen/include/asm-arm/regs.h           |  2 +-
 xen/include/asm-x86/regs.h           |  2 +-
 xen/include/asm-x86/x86_64/efibind.h |  2 +-
 xen/include/efi/efidef.h             | 11 +----------
 xen/include/xen/mm.h                 |  2 +-
 xen/include/xen/sched.h              |  2 +-
 15 files changed, 25 insertions(+), 50 deletions(-)

Comments

Stefano Stabellini Nov. 11, 2019, 8:40 p.m. UTC | #1
On Mon, 11 Nov 2019, Andrew Cooper wrote:
> actypes.h and efidef.h both define BOOLEAN as unsigned char, which is buggy in
> combination with logic such as "BOOLEAN b = (a & 0x100);"  Redefine BOOLEAN as
> bool instead, which doesn't truncate.
> 
> Both also define TRUE and FALSE, with actypes.h being extra rude and replacing
> whatever exists thus far.  Drop all uses of TRUE and FALSE, replacing them
> with true/false respectively, and drop the declarations.
> 
> Also drop the pointless conditional declaration of NULL while cleaning this
> up.
> 
> Finally, correct all the comments which which were found by sed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> This is based on top of Rogers patch adjusting part of io_apic.c
> 
> Compile tested on ARM, fully texted on x86.
> 
> RFC for 4.13 - I thought I'd got all of the boolean truncation bugs back in
> 4.8 but clearly not...
> ---
>  xen/arch/x86/io_apic.c               | 12 ++++++------
>  xen/arch/x86/x86_64/mm.c             |  2 +-
>  xen/common/kexec.c                   |  6 +++---
>  xen/common/timer.c                   |  4 ++--
>  xen/drivers/acpi/tables/tbfadt.c     |  4 ++--
>  xen/drivers/passthrough/vtd/utils.c  |  2 +-
>  xen/include/acpi/acconfig.h          |  2 +-
>  xen/include/acpi/actypes.h           | 20 ++------------------
>  xen/include/asm-arm/arm64/efibind.h  |  2 +-
>  xen/include/asm-arm/regs.h           |  2 +-
>  xen/include/asm-x86/regs.h           |  2 +-
>  xen/include/asm-x86/x86_64/efibind.h |  2 +-
>  xen/include/efi/efidef.h             | 11 +----------
>  xen/include/xen/mm.h                 |  2 +-
>  xen/include/xen/sched.h              |  2 +-
>  15 files changed, 25 insertions(+), 50 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 732b57995c..6517eb5ae9 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -285,7 +285,7 @@ static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int p
>      {
>          /* If vector is unknown, read it from the IO-APIC */
>          if ( vector == IRQ_VECTOR_UNASSIGNED )
> -            vector = __ioapic_read_entry(apic, pin, TRUE).vector;
> +            vector = __ioapic_read_entry(apic, pin, true).vector;
>  
>          *(IO_APIC_BASE(apic)+16) = vector;
>      }
> @@ -296,28 +296,28 @@ static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int p
>          struct IO_APIC_route_entry entry;
>          bool need_to_unmask = false;
>  
> -        entry = __ioapic_read_entry(apic, pin, TRUE);
> +        entry = __ioapic_read_entry(apic, pin, true);
>  
>          if ( ! entry.mask )
>          {
>              /* If entry is not currently masked, mask it and make
>               * a note to unmask it later */
>              entry.mask = 1;
> -            __ioapic_write_entry(apic, pin, TRUE, entry);
> +            __ioapic_write_entry(apic, pin, true, entry);
>              need_to_unmask = true;
>          }
>  
>          /* Flip the trigger mode to edge and back */
>          entry.trigger = 0;
> -        __ioapic_write_entry(apic, pin, TRUE, entry);
> +        __ioapic_write_entry(apic, pin, true, entry);
>          entry.trigger = 1;
> -        __ioapic_write_entry(apic, pin, TRUE, entry);
> +        __ioapic_write_entry(apic, pin, true, entry);
>  
>          if ( need_to_unmask )
>          {
>              /* Unmask if neccesary */
>              entry.mask = 0;
> -            __ioapic_write_entry(apic, pin, TRUE, entry);
> +            __ioapic_write_entry(apic, pin, true, entry);
>          }
>      }
>  }
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index fa55f3474e..e9d7b80caf 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>  }
>  
>  
> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
> +/* Returns true if given descriptor is valid for GDT or LDT. */
>  int check_descriptor(const struct domain *dom, seg_desc_t *d)
>  {
>      u32 a = d->a, b = d->b;
> diff --git a/xen/common/kexec.c b/xen/common/kexec.c
> index a262cc5a18..8e7540f605 100644
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -33,7 +33,7 @@
>  #include <compat/kexec.h>
>  #endif
>  
> -bool_t kexecing = FALSE;
> +bool kexecing = false;
>  
>  /* Memory regions to store the per cpu register state etc. on a crash. */
>  typedef struct { Elf_Note * start; size_t size; } crash_note_range_t;
> @@ -379,7 +379,7 @@ void kexec_crash(void)
>      if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
>          return;
>  
> -    kexecing = TRUE;
> +    kexecing = true;
>  
>      if ( kexec_common_shutdown() != 0 )
>          return;
> @@ -395,7 +395,7 @@ static long kexec_reboot(void *_image)
>  {
>      struct kexec_image *image = _image;
>  
> -    kexecing = TRUE;
> +    kexecing = true;
>  
>      kexec_common_shutdown();
>      machine_reboot_kexec(image);
> diff --git a/xen/common/timer.c b/xen/common/timer.c
> index 645206a989..29f8f40f88 100644
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -100,7 +100,7 @@ static void up_heap(struct timer **heap, unsigned int pos)
>  }
>  
>  
> -/* Delete @t from @heap. Return TRUE if new top of heap. */
> +/* Delete @t from @heap. Return true if new top of heap. */
>  static int remove_from_heap(struct timer **heap, struct timer *t)
>  {
>      unsigned int sz = heap_metadata(heap)->size;
> @@ -127,7 +127,7 @@ static int remove_from_heap(struct timer **heap, struct timer *t)
>  }
>  
>  
> -/* Add new entry @t to @heap. Return TRUE if new top of heap. */
> +/* Add new entry @t to @heap. Return true if new top of heap. */
>  static int add_to_heap(struct timer **heap, struct timer *t)
>  {
>      unsigned int sz = heap_metadata(heap)->size;
> diff --git a/xen/drivers/acpi/tables/tbfadt.c b/xen/drivers/acpi/tables/tbfadt.c
> index f11fd5a900..6f30aede9c 100644
> --- a/xen/drivers/acpi/tables/tbfadt.c
> +++ b/xen/drivers/acpi/tables/tbfadt.c
> @@ -250,9 +250,9 @@ void __init acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 lengt
>  
>  	/* Take a copy of the Hardware Reduced flag */
>  
> -	acpi_gbl_reduced_hardware = FALSE;
> +	acpi_gbl_reduced_hardware = false;
>  	if (acpi_gbl_FADT.flags & ACPI_FADT_HW_REDUCED) {
> -		acpi_gbl_reduced_hardware = TRUE;
> +		acpi_gbl_reduced_hardware = true;
>  	}
>  
>  	/*
> diff --git a/xen/drivers/passthrough/vtd/utils.c b/xen/drivers/passthrough/vtd/utils.c
> index 7552dd8e0c..4531581846 100644
> --- a/xen/drivers/passthrough/vtd/utils.c
> +++ b/xen/drivers/passthrough/vtd/utils.c
> @@ -281,7 +281,7 @@ void vtd_dump_iommu_info(unsigned char key)
>              for ( i = 0; i <= reg_01.bits.entries; i++ )
>              {
>                  struct IO_APIC_route_entry rte =
> -                    __ioapic_read_entry(apic, i, TRUE);
> +                    __ioapic_read_entry(apic, i, true);
>  
>                  remap = (struct IO_APIC_route_remap_entry *) &rte;
>                  if ( !remap->format )
> diff --git a/xen/include/acpi/acconfig.h b/xen/include/acpi/acconfig.h
> index 422f29c06c..f0330fb990 100644
> --- a/xen/include/acpi/acconfig.h
> +++ b/xen/include/acpi/acconfig.h
> @@ -87,7 +87,7 @@
>   * Should the subsystem abort the loading of an ACPI table if the
>   * table checksum is incorrect?
>   */
> -#define ACPI_CHECKSUM_ABORT             FALSE
> +#define ACPI_CHECKSUM_ABORT             false
>  
>  /******************************************************************************
>   *
> diff --git a/xen/include/acpi/actypes.h b/xen/include/acpi/actypes.h
> index f3e95abc3a..4aad815f7b 100644
> --- a/xen/include/acpi/actypes.h
> +++ b/xen/include/acpi/actypes.h
> @@ -124,7 +124,7 @@
>   *
>   ******************************************************************************/
>  
> -typedef unsigned char BOOLEAN;
> +typedef bool BOOLEAN;
>  typedef unsigned char UINT8;
>  typedef unsigned short UINT16;
>  typedef COMPILER_DEPENDENT_UINT64 UINT64;
> @@ -260,22 +260,6 @@ typedef acpi_native_uint acpi_size;
>   *
>   ******************************************************************************/
>  
> -/* Logical defines and NULL */
> -
> -#ifdef FALSE
> -#undef FALSE
> -#endif
> -#define FALSE                           (1 == 0)
> -
> -#ifdef TRUE
> -#undef TRUE
> -#endif
> -#define TRUE                            (1 == 1)
> -
> -#ifndef NULL
> -#define NULL                            (void *) 0
> -#endif
> -
>  /*
>   * Mescellaneous types
>   */
> @@ -503,7 +487,7 @@ typedef u32 acpi_event_type;
>   * Event Status - Per event
>   * -------------
>   * The encoding of acpi_event_status is illustrated below.
> - * Note that a set bit (1) indicates the property is TRUE
> + * Note that a set bit (1) indicates the property is true
>   * (e.g. if bit 0 is set then the event is enabled).
>   * +-------------+-+-+-+
>   * |   Bits 31:3 |2|1|0|
> diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
> index 2b0bf40bf2..08ab70e668 100644
> --- a/xen/include/asm-arm/arm64/efibind.h
> +++ b/xen/include/asm-arm/arm64/efibind.h
> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>  #define POST_CODE(_Data)
>  
>  
> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
>  
>  //
>  // Pointers must be aligned to these address to function
> diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
> index 0e3e56b452..f93e1d42b0 100644
> --- a/xen/include/asm-arm/regs.h
> +++ b/xen/include/asm-arm/regs.h
> @@ -53,7 +53,7 @@ static inline bool guest_mode(const struct cpu_user_regs *r)
>      ASSERT(diff < STACK_SIZE);
>      /* If not a guest frame, it must be a hypervisor frame. */
>      ASSERT((diff == 0) || hyp_mode(r));
> -    /* Return TRUE if it's a guest frame. */
> +    /* Return true if it's a guest frame. */
>      return (diff == 0);
>  }
>  
> diff --git a/xen/include/asm-x86/regs.h b/xen/include/asm-x86/regs.h
> index 725a664e0a..679c38bb76 100644
> --- a/xen/include/asm-x86/regs.h
> +++ b/xen/include/asm-x86/regs.h
> @@ -11,7 +11,7 @@
>      ASSERT(diff < STACK_SIZE);                                                \
>      /* If not a guest frame, it must be a hypervisor frame. */                \
>      ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                        \
> -    /* Return TRUE if it's a guest frame. */                                  \
> +    /* Return true if it's a guest frame. */                                  \
>      (diff == 0);                                                              \
>  })
>  
> diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h
> index b013db175d..2b7001f8f4 100644
> --- a/xen/include/asm-x86/x86_64/efibind.h
> +++ b/xen/include/asm-x86/x86_64/efibind.h
> @@ -127,7 +127,7 @@ typedef uint64_t   UINTN;
>  #ifdef EFI_NT_EMULATOR
>      #define BREAKPOINT()        __asm { int 3 }
>  #else
> -    #define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
> +    #define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
>  #endif
>  
>  //
> diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
> index 86a7e111bf..fe1750de51 100644
> --- a/xen/include/efi/efidef.h
> +++ b/xen/include/efi/efidef.h
> @@ -22,16 +22,7 @@ Revision History
>  
>  typedef UINT16          CHAR16;
>  typedef UINT8           CHAR8;
> -typedef UINT8           BOOLEAN;
> -
> -#ifndef TRUE
> -    #define TRUE    ((BOOLEAN) 1)
> -    #define FALSE   ((BOOLEAN) 0)
> -#endif
> -
> -#ifndef NULL
> -    #define NULL    ((VOID *) 0)
> -#endif
> +typedef bool            BOOLEAN;
>  
>  typedef UINTN           EFI_STATUS;
>  typedef UINT64          EFI_LBA;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 8d0ddfb60c..2b5ae8cae4 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -607,7 +607,7 @@ int __must_check donate_page(struct domain *d, struct page_info *page,
>  #define RAM_TYPE_UNUSABLE     0x00000004
>  #define RAM_TYPE_ACPI         0x00000008
>  #define RAM_TYPE_UNKNOWN      0x00000010
> -/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
> +/* true if the whole page at @mfn is of the requested RAM type(s) above. */
>  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
>  /* Returns the page type(s). */
>  unsigned int page_get_ram_type(mfn_t mfn);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 9f7bc69293..c43d9311aa 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -552,7 +552,7 @@ static inline bool is_system_domain(const struct domain *d)
>  
>  /*
>   * Use this when you don't have an existing reference to @d. It returns
> - * FALSE if @d is being destroyed.
> + * false if @d is being destroyed.
>   */
>  static always_inline int get_domain(struct domain *d)
>  {
> -- 
> 2.11.0
>
Jan Beulich Nov. 12, 2019, 8:35 a.m. UTC | #2
On 11.11.2019 21:24, Andrew Cooper wrote:
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>  }
>  
>  
> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
> +/* Returns true if given descriptor is valid for GDT or LDT. */
>  int check_descriptor(const struct domain *dom, seg_desc_t *d)

Wouldn't changes like this one better be accompanied by also adjusting
the return type of the function (there are more examples further down
in common/timer.c)?

> --- a/xen/include/asm-arm/arm64/efibind.h
> +++ b/xen/include/asm-arm/arm64/efibind.h
> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>  #define POST_CODE(_Data)
>  
>  
> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32

You do realize that this and other EFI headers (and perhaps also
ACPI ones) are largely verbatim imports from other projects,
updating of which will become less straightforward by such
replacements? When pulling in the EFI ones I intentionally did not
fiddle with them more than absolutely necessary.

If it wasn't for this, I'd have ack-ed the patch despite the other
remark above.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -607,7 +607,7 @@ int __must_check donate_page(struct domain *d, struct page_info *page,
>  #define RAM_TYPE_UNUSABLE     0x00000004
>  #define RAM_TYPE_ACPI         0x00000008
>  #define RAM_TYPE_UNKNOWN      0x00000010
> -/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
> +/* true if the whole page at @mfn is of the requested RAM type(s) above. */
>  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);

In other comments I already wasn't sure about such replacements, but
let them be. Here, however, you violate coding style by using "true"
instead of "True" (the function returning "int" for now doesn't even
allow the excuse of meaning the identifier rather than the word).

Jan
Wei Liu Nov. 12, 2019, 10:42 a.m. UTC | #3
On Mon, Nov 11, 2019 at 08:24:43PM +0000, Andrew Cooper wrote:
> actypes.h and efidef.h both define BOOLEAN as unsigned char, which is buggy in
> combination with logic such as "BOOLEAN b = (a & 0x100);"  Redefine BOOLEAN as
> bool instead, which doesn't truncate.
> 
> Both also define TRUE and FALSE, with actypes.h being extra rude and replacing
> whatever exists thus far.  Drop all uses of TRUE and FALSE, replacing them
> with true/false respectively, and drop the declarations.
> 
> Also drop the pointless conditional declaration of NULL while cleaning this
> up.
> 
> Finally, correct all the comments which which were found by sed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>
Andrew Cooper Nov. 12, 2019, 1:39 p.m. UTC | #4
On 12/11/2019 08:35, Jan Beulich wrote:
> On 11.11.2019 21:24, Andrew Cooper wrote:
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>>  }
>>  
>>  
>> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
>> +/* Returns true if given descriptor is valid for GDT or LDT. */
>>  int check_descriptor(const struct domain *dom, seg_desc_t *d)
> Wouldn't changes like this one better be accompanied by also adjusting
> the return type of the function (there are more examples further down
> in common/timer.c)?

No.  That is an unrelated change.

If I were flush with free time then I might consider doing this and
substantially increase the test burden.

As it stands, this request is scope creep.

>
>> --- a/xen/include/asm-arm/arm64/efibind.h
>> +++ b/xen/include/asm-arm/arm64/efibind.h
>> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>>  #define POST_CODE(_Data)
>>  
>>  
>> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
>> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
> You do realize that this and other EFI headers (and perhaps also
> ACPI ones) are largely verbatim imports from other projects,
> updating of which will become less straightforward by such
> replacements? When pulling in the EFI ones I intentionally did not
> fiddle with them more than absolutely necessary.

Yes, and?

It is unacceptable for the acpi headers to forcibly redefine anything in
their scope, and its definition of va_args is downright dangerous.

All junk like this in header files does nothing but waste space and
compiler effort during compilation, and leave people with an slim chance
of shooting themselves in the foot.

How many times do these get touched?  (Rhetorical question.  The answer
is once (me, clang build fix) since their introduction, 8, 9 and 10
years ago).

For the 30s of effort required to tweak once-in-a-blue-moon patches
which touch these headers, trimming the junk is a no-brainer.

>
> If it wasn't for this, I'd have ack-ed the patch despite the other
> remark above.
>
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -607,7 +607,7 @@ int __must_check donate_page(struct domain *d, struct page_info *page,
>>  #define RAM_TYPE_UNUSABLE     0x00000004
>>  #define RAM_TYPE_ACPI         0x00000008
>>  #define RAM_TYPE_UNKNOWN      0x00000010
>> -/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
>> +/* true if the whole page at @mfn is of the requested RAM type(s) above. */
>>  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
> In other comments I already wasn't sure about such replacements, but
> let them be. Here, however, you violate coding style by using "true"
> instead of "True" (the function returning "int" for now doesn't even
> allow the excuse of meaning the identifier rather than the word).

Fixed.

~Andrew
Jan Beulich Nov. 12, 2019, 2:03 p.m. UTC | #5
On 12.11.2019 14:39, Andrew Cooper wrote:
> On 12/11/2019 08:35, Jan Beulich wrote:
>> On 11.11.2019 21:24, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/x86_64/mm.c
>>> +++ b/xen/arch/x86/x86_64/mm.c
>>> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>>>  }
>>>  
>>>  
>>> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
>>> +/* Returns true if given descriptor is valid for GDT or LDT. */
>>>  int check_descriptor(const struct domain *dom, seg_desc_t *d)
>> Wouldn't changes like this one better be accompanied by also adjusting
>> the return type of the function (there are more examples further down
>> in common/timer.c)?
> 
> No.  That is an unrelated change.
> 
> If I were flush with free time then I might consider doing this and
> substantially increase the test burden.
> 
> As it stands, this request is scope creep.

The other alternative would have been to ask for scope reduction,
i.e. leave alone such comments (to avoid the resulting visual
disconnect between comment and actual data type). Anyway - it was
just a question I wanted to raise, not a request for further work
on your part.

>>> --- a/xen/include/asm-arm/arm64/efibind.h
>>> +++ b/xen/include/asm-arm/arm64/efibind.h
>>> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>>>  #define POST_CODE(_Data)
>>>  
>>>  
>>> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
>>> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
>> You do realize that this and other EFI headers (and perhaps also
>> ACPI ones) are largely verbatim imports from other projects,
>> updating of which will become less straightforward by such
>> replacements? When pulling in the EFI ones I intentionally did not
>> fiddle with them more than absolutely necessary.
> 
> Yes, and?
> 
> It is unacceptable for the acpi headers to forcibly redefine anything in
> their scope, and its definition of va_args is downright dangerous.
> 
> All junk like this in header files does nothing but waste space and
> compiler effort during compilation, and leave people with an slim chance
> of shooting themselves in the foot.

Well, on one hand I'm with you. But then I dare to guess that the
people having written the headers the way they are also aren't
completely un-knowledgeable, i.e. did so for a reason. This seems
(I'm sorry to say it this bluntly) once again a case where you
appear to not be willing to accept other thinking than your own.
It is therefore one thing to get rid of TRUE/FALSE _outside_ of
such headers (where it would better never have been introduced),
and another to modify these more or less verbatim imported headers
themselves.

> How many times do these get touched?  (Rhetorical question.  The answer
> is once (me, clang build fix) since their introduction, 8, 9 and 10
> years ago).
> 
> For the 30s of effort required to tweak once-in-a-blue-moon patches
> which touch these headers, trimming the junk is a no-brainer.

Well, I agree that for just _this_ change it's not a big deal.
But any such approach doesn't scale: What we allow ourselves to do
once we may then easily allow ourselves to do another time, and
then dozens more times. Once that has happened, the effort needed
to do a re-sync may become non-negligible.

Bottom line - I'm half convinced and willing to give my ack, but
I'm not convinced you truly thought through the longer term
consequences. I'd therefore be far happier to see this patch
split into a non-controversial part (anything that's not tied to
the ACPI and EFI header imports), an ACPI, and an EFI part.

Jan
Andrew Cooper Dec. 6, 2019, 9:02 p.m. UTC | #6
On 12/11/2019 14:03, Jan Beulich wrote:
> On 12.11.2019 14:39, Andrew Cooper wrote:
>> On 12/11/2019 08:35, Jan Beulich wrote:
>>> On 11.11.2019 21:24, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/x86_64/mm.c
>>>> +++ b/xen/arch/x86/x86_64/mm.c
>>>> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned long base)
>>>>  }
>>>>  
>>>>  
>>>> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */
>>>> +/* Returns true if given descriptor is valid for GDT or LDT. */
>>>>  int check_descriptor(const struct domain *dom, seg_desc_t *d)
>>> Wouldn't changes like this one better be accompanied by also adjusting
>>> the return type of the function (there are more examples further down
>>> in common/timer.c)?
>> No.  That is an unrelated change.
>>
>> If I were flush with free time then I might consider doing this and
>> substantially increase the test burden.
>>
>> As it stands, this request is scope creep.
> The other alternative would have been to ask for scope reduction,
> i.e. leave alone such comments (to avoid the resulting visual
> disconnect between comment and actual data type). Anyway - it was
> just a question I wanted to raise, not a request for further work
> on your part.
>
>>>> --- a/xen/include/asm-arm/arm64/efibind.h
>>>> +++ b/xen/include/asm-arm/arm64/efibind.h
>>>> @@ -107,7 +107,7 @@ typedef uint64_t   UINTN;
>>>>  #define POST_CODE(_Data)
>>>>  
>>>>  
>>>> -#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
>>>> +#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
>>> You do realize that this and other EFI headers (and perhaps also
>>> ACPI ones) are largely verbatim imports from other projects,
>>> updating of which will become less straightforward by such
>>> replacements? When pulling in the EFI ones I intentionally did not
>>> fiddle with them more than absolutely necessary.
>> Yes, and?
>>
>> It is unacceptable for the acpi headers to forcibly redefine anything in
>> their scope, and its definition of va_args is downright dangerous.
>>
>> All junk like this in header files does nothing but waste space and
>> compiler effort during compilation, and leave people with an slim chance
>> of shooting themselves in the foot.
> Well, on one hand I'm with you. But then I dare to guess that the
> people having written the headers the way they are also aren't
> completely un-knowledgeable, i.e. did so for a reason.

Just because there may have been a reason, doesn't mean the reason is
compatible with Xens codebase, today.

> This seems
> (I'm sorry to say it this bluntly) once again a case where you
> appear to not be willing to accept other thinking than your own.

I might not care if this was confined to a private.h in a subdirectly
which was never edited.

But it is not.  The actively dangerous constructs in these header files
are included all over the Xen codebase, just waiting to shoot someone in
the foot.

Xen is not bound by whatever decisions these projects made more than a
decade ago.  We do not need to take the headers verbatim, and there are
good reasons to specifically not take them verbatim.

> It is therefore one thing to get rid of TRUE/FALSE _outside_ of
> such headers (where it would better never have been introduced),
> and another to modify these more or less verbatim imported headers
> themselves.

The fact that their use has crept outside demonstrates why they should
be deleted entirely.  The constructs are buggy, and the will creep again
in the future.

Turning TRUE/FALSE/BOOL into a compile error is by far the best way to
increase the health of the codebase.

>> How many times do these get touched?  (Rhetorical question.  The answer
>> is once (me, clang build fix) since their introduction, 8, 9 and 10
>> years ago).
>>
>> For the 30s of effort required to tweak once-in-a-blue-moon patches
>> which touch these headers, trimming the junk is a no-brainer.
> Well, I agree that for just _this_ change it's not a big deal.
> But any such approach doesn't scale: What we allow ourselves to do
> once we may then easily allow ourselves to do another time, and
> then dozens more times. Once that has happened, the effort needed
> to do a re-sync may become non-negligible.

There are perfectly easy ways to do this with negligible effort, as I
frequently do with other routine XenServer work.  (The git
locally-modified tracking is especially good for this, even for pulling
a small delta out of a substantially modified file.)



> Bottom line - I'm half convinced and willing to give my ack, but
> I'm not convinced you truly thought through the longer term
> consequences. I'd therefore be far happier to see this patch
> split into a non-controversial part (anything that's not tied to
> the ACPI and EFI header imports), an ACPI, and an EFI part.

I do not want to writing the same patch again in $N years time because
review and CI missed it creeping back in.

I don't think this is an unreasonable position to take.

~Andrew
Jan Beulich Dec. 9, 2019, 12:11 p.m. UTC | #7
On 06.12.2019 22:02, Andrew Cooper wrote:
> On 12/11/2019 14:03, Jan Beulich wrote:
>> Bottom line - I'm half convinced and willing to give my ack, but
>> I'm not convinced you truly thought through the longer term
>> consequences. I'd therefore be far happier to see this patch
>> split into a non-controversial part (anything that's not tied to
>> the ACPI and EFI header imports), an ACPI, and an EFI part.
> 
> I do not want to writing the same patch again in $N years time because
> review and CI missed it creeping back in.
> 
> I don't think this is an unreasonable position to take.

It for sure isn't. Yet I also don't think though my request how to
split things is. By asking for the split I'm implying that we may
still reach agreement on the controversial parts, faod. Sadly once
again there are no other opinions helping to sort which route may
be the overall preferred one.

Jan
Julien Grall Dec. 9, 2019, 12:38 p.m. UTC | #8
Hi Jan,

On 09/12/2019 12:11, Jan Beulich wrote:
> On 06.12.2019 22:02, Andrew Cooper wrote:
>> On 12/11/2019 14:03, Jan Beulich wrote:
>>> Bottom line - I'm half convinced and willing to give my ack, but
>>> I'm not convinced you truly thought through the longer term
>>> consequences. I'd therefore be far happier to see this patch
>>> split into a non-controversial part (anything that's not tied to
>>> the ACPI and EFI header imports), an ACPI, and an EFI part.
>>
>> I do not want to writing the same patch again in $N years time because
>> review and CI missed it creeping back in.
>>
>> I don't think this is an unreasonable position to take.
> 
> It for sure isn't. Yet I also don't think though my request how to
> split things is. By asking for the split I'm implying that we may
> still reach agreement on the controversial parts, faod. Sadly once
> again there are no other opinions helping to sort which route may
> be the overall preferred one.

Well, for a first, I don't think I have seen any explicit request for 
opinion so far and not all the relevant maintainers have been CCed here.

In general, I tend to stay clear from argument between you and Andrew to 
avoid been dragged into bikeshedding.

Anyway, while I understand that we want to keep as close as upstream, 
those headers are not resync very often and the changes are minimal. The 
long term consequence is not about resync but keeping bogus code that 
can be used by everyone.

So the patch itself is a good step forward to make Xen better.

Cheers,
Andrew Cooper Jan. 6, 2020, 7:01 p.m. UTC | #9
On 09/12/2019 12:11, Jan Beulich wrote:
> On 06.12.2019 22:02, Andrew Cooper wrote:
>> On 12/11/2019 14:03, Jan Beulich wrote:
>>> Bottom line - I'm half convinced and willing to give my ack, but
>>> I'm not convinced you truly thought through the longer term
>>> consequences. I'd therefore be far happier to see this patch
>>> split into a non-controversial part (anything that's not tied to
>>> the ACPI and EFI header imports), an ACPI, and an EFI part.
>> I do not want to writing the same patch again in $N years time because
>> review and CI missed it creeping back in.
>>
>> I don't think this is an unreasonable position to take.
> It for sure isn't. Yet I also don't think though my request how to
> split things is. By asking for the split I'm implying that we may
> still reach agreement on the controversial parts, faod. Sadly once
> again there are no other opinions helping to sort which route may
> be the overall preferred one.

As Julien points out (and you not responded to his email) you have at no
point actually requested a split.  You merely suggested that you might
be willing to ack 1/3 the patch, with the overt implication that
objections would continue on the rest.

This deadlock has gone on far too long.  Therefore, you have until the
end of the week to either:

1) Agree in principle that you will accept this patch (modulo tidy-up)
split into 3 - i.e. that I won't be wasting my time doing so, or

2) Provide a concrete alternative which retains the property of being
impossible for buggy constructs to find their way back into the codebase.

~Andrew
Jan Beulich Jan. 7, 2020, 8:35 a.m. UTC | #10
On 06.01.2020 20:01, Andrew Cooper wrote:
> On 09/12/2019 12:11, Jan Beulich wrote:
>> On 06.12.2019 22:02, Andrew Cooper wrote:
>>> On 12/11/2019 14:03, Jan Beulich wrote:
>>>> Bottom line - I'm half convinced and willing to give my ack, but
>>>> I'm not convinced you truly thought through the longer term
>>>> consequences. I'd therefore be far happier to see this patch
>>>> split into a non-controversial part (anything that's not tied to
>>>> the ACPI and EFI header imports), an ACPI, and an EFI part.
>>> I do not want to writing the same patch again in $N years time because
>>> review and CI missed it creeping back in.
>>>
>>> I don't think this is an unreasonable position to take.
>> It for sure isn't. Yet I also don't think though my request how to
>> split things is. By asking for the split I'm implying that we may
>> still reach agreement on the controversial parts, faod. Sadly once
>> again there are no other opinions helping to sort which route may
>> be the overall preferred one.
> 
> As Julien points out (and you not responded to his email) you have at no
> point actually requested a split.  You merely suggested that you might
> be willing to ack 1/3 the patch, with the overt implication that
> objections would continue on the rest.

Please read again the second sentence of my earlier reply still
visible in context above. (As an aside, I've not seen a message
from Julien to the effect of me not having requested a split. All
I have in my inbox is him pointing out that I didn't explicitly
ask for other people's opinions.)

> This deadlock has gone on far too long.  Therefore, you have until the
> end of the week to either:

I'm sorry, but very much NO to the underlying principle. It is so
much more frequently that my patches are stuck on you dropping a
discussion in the middle (or not even replying to patches in the
first place) that I don't think you're in a legitimate position
to set this kind of an ultimatum (leaving aside that from my
perspective there's no deadlock here at all, as per the part of
my original reply that you've left in context above).

> 1) Agree in principle that you will accept this patch (modulo tidy-up)
> split into 3 - i.e. that I won't be wasting my time doing so, or

Please read again the first sentence of my earlier reply still
visible in context above. The split request was really because
the changes to the cloned sources / headers want a different
justification (for example, "buggy" is not a valid attribution
for BOOLEAN to be a typedef of unsigned char - what can be buggy
as a result are wrong uses of variables of this type, just like
was the case for our original bool_t - and it's instead a very
legitimate means to arrange for code to be re-usable across a
wide range of used C compilers) than the ones to sources that
we don't consider "cloned" (anymore). In particular, as
expressed, I'd like the descriptions to make at least implicitly
clear that the longer term consequences have been considered: A
dozen small changes like this one are as impactful to later
re-sync attempts as is one big change. However much I'm in favor
of not taking pure black-and-white perspectives, I'm afraid
there's not much of an alternative here.

> 2) Provide a concrete alternative which retains the property of being
> impossible for buggy constructs to find their way back into the codebase.

I stand by my original statement: If you don't want to waste time,
submit just the uncontroversial third. The concrete alternative is
for people (submitters even more than reviewers) to be disciplined:
Don't use what you call "buggy" constructs in unsuitable contexts.

I'd be willing to withdraw my (half) objection (seeing that you've
got Stefano's ack and Wei's R-b) if we could settle the underlying
discussion: If I'm not mistaken, in some cases you (and others)
have asked for clones of foreign repo sources to stay as close to
the original as possible (sorry, don't have an example to hand),
yet here you're going the opposite direction. Quoting from an
earlier reply of yours: "We do not need to take the headers
verbatim, and there are good reasons to specifically not take them
verbatim." I agree, but there should be clear guidelines (if not
rules). I.e. the decision which way to go for any individual piece
of code should ideally be independent of the person doing the
actual import or change (and - long term, as correcting past
mistakes does take time - be consistent across the code base).

Perhaps this would best be made a topic for the community call
(iirc scheduled for later this week). I'll try to remember to put
it up as a topic.

I'm sorry, it's clear this isn't exactly the reply you've been
wanting.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 732b57995c..6517eb5ae9 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -285,7 +285,7 @@  static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int p
     {
         /* If vector is unknown, read it from the IO-APIC */
         if ( vector == IRQ_VECTOR_UNASSIGNED )
-            vector = __ioapic_read_entry(apic, pin, TRUE).vector;
+            vector = __ioapic_read_entry(apic, pin, true).vector;
 
         *(IO_APIC_BASE(apic)+16) = vector;
     }
@@ -296,28 +296,28 @@  static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int p
         struct IO_APIC_route_entry entry;
         bool need_to_unmask = false;
 
-        entry = __ioapic_read_entry(apic, pin, TRUE);
+        entry = __ioapic_read_entry(apic, pin, true);
 
         if ( ! entry.mask )
         {
             /* If entry is not currently masked, mask it and make
              * a note to unmask it later */
             entry.mask = 1;
-            __ioapic_write_entry(apic, pin, TRUE, entry);
+            __ioapic_write_entry(apic, pin, true, entry);
             need_to_unmask = true;
         }
 
         /* Flip the trigger mode to edge and back */
         entry.trigger = 0;
-        __ioapic_write_entry(apic, pin, TRUE, entry);
+        __ioapic_write_entry(apic, pin, true, entry);
         entry.trigger = 1;
-        __ioapic_write_entry(apic, pin, TRUE, entry);
+        __ioapic_write_entry(apic, pin, true, entry);
 
         if ( need_to_unmask )
         {
             /* Unmask if neccesary */
             entry.mask = 0;
-            __ioapic_write_entry(apic, pin, TRUE, entry);
+            __ioapic_write_entry(apic, pin, true, entry);
         }
     }
 }
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index fa55f3474e..e9d7b80caf 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1077,7 +1077,7 @@  long do_set_segment_base(unsigned int which, unsigned long base)
 }
 
 
-/* Returns TRUE if given descriptor is valid for GDT or LDT. */
+/* Returns true if given descriptor is valid for GDT or LDT. */
 int check_descriptor(const struct domain *dom, seg_desc_t *d)
 {
     u32 a = d->a, b = d->b;
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index a262cc5a18..8e7540f605 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -33,7 +33,7 @@ 
 #include <compat/kexec.h>
 #endif
 
-bool_t kexecing = FALSE;
+bool kexecing = false;
 
 /* Memory regions to store the per cpu register state etc. on a crash. */
 typedef struct { Elf_Note * start; size_t size; } crash_note_range_t;
@@ -379,7 +379,7 @@  void kexec_crash(void)
     if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
         return;
 
-    kexecing = TRUE;
+    kexecing = true;
 
     if ( kexec_common_shutdown() != 0 )
         return;
@@ -395,7 +395,7 @@  static long kexec_reboot(void *_image)
 {
     struct kexec_image *image = _image;
 
-    kexecing = TRUE;
+    kexecing = true;
 
     kexec_common_shutdown();
     machine_reboot_kexec(image);
diff --git a/xen/common/timer.c b/xen/common/timer.c
index 645206a989..29f8f40f88 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -100,7 +100,7 @@  static void up_heap(struct timer **heap, unsigned int pos)
 }
 
 
-/* Delete @t from @heap. Return TRUE if new top of heap. */
+/* Delete @t from @heap. Return true if new top of heap. */
 static int remove_from_heap(struct timer **heap, struct timer *t)
 {
     unsigned int sz = heap_metadata(heap)->size;
@@ -127,7 +127,7 @@  static int remove_from_heap(struct timer **heap, struct timer *t)
 }
 
 
-/* Add new entry @t to @heap. Return TRUE if new top of heap. */
+/* Add new entry @t to @heap. Return true if new top of heap. */
 static int add_to_heap(struct timer **heap, struct timer *t)
 {
     unsigned int sz = heap_metadata(heap)->size;
diff --git a/xen/drivers/acpi/tables/tbfadt.c b/xen/drivers/acpi/tables/tbfadt.c
index f11fd5a900..6f30aede9c 100644
--- a/xen/drivers/acpi/tables/tbfadt.c
+++ b/xen/drivers/acpi/tables/tbfadt.c
@@ -250,9 +250,9 @@  void __init acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 lengt
 
 	/* Take a copy of the Hardware Reduced flag */
 
-	acpi_gbl_reduced_hardware = FALSE;
+	acpi_gbl_reduced_hardware = false;
 	if (acpi_gbl_FADT.flags & ACPI_FADT_HW_REDUCED) {
-		acpi_gbl_reduced_hardware = TRUE;
+		acpi_gbl_reduced_hardware = true;
 	}
 
 	/*
diff --git a/xen/drivers/passthrough/vtd/utils.c b/xen/drivers/passthrough/vtd/utils.c
index 7552dd8e0c..4531581846 100644
--- a/xen/drivers/passthrough/vtd/utils.c
+++ b/xen/drivers/passthrough/vtd/utils.c
@@ -281,7 +281,7 @@  void vtd_dump_iommu_info(unsigned char key)
             for ( i = 0; i <= reg_01.bits.entries; i++ )
             {
                 struct IO_APIC_route_entry rte =
-                    __ioapic_read_entry(apic, i, TRUE);
+                    __ioapic_read_entry(apic, i, true);
 
                 remap = (struct IO_APIC_route_remap_entry *) &rte;
                 if ( !remap->format )
diff --git a/xen/include/acpi/acconfig.h b/xen/include/acpi/acconfig.h
index 422f29c06c..f0330fb990 100644
--- a/xen/include/acpi/acconfig.h
+++ b/xen/include/acpi/acconfig.h
@@ -87,7 +87,7 @@ 
  * Should the subsystem abort the loading of an ACPI table if the
  * table checksum is incorrect?
  */
-#define ACPI_CHECKSUM_ABORT             FALSE
+#define ACPI_CHECKSUM_ABORT             false
 
 /******************************************************************************
  *
diff --git a/xen/include/acpi/actypes.h b/xen/include/acpi/actypes.h
index f3e95abc3a..4aad815f7b 100644
--- a/xen/include/acpi/actypes.h
+++ b/xen/include/acpi/actypes.h
@@ -124,7 +124,7 @@ 
  *
  ******************************************************************************/
 
-typedef unsigned char BOOLEAN;
+typedef bool BOOLEAN;
 typedef unsigned char UINT8;
 typedef unsigned short UINT16;
 typedef COMPILER_DEPENDENT_UINT64 UINT64;
@@ -260,22 +260,6 @@  typedef acpi_native_uint acpi_size;
  *
  ******************************************************************************/
 
-/* Logical defines and NULL */
-
-#ifdef FALSE
-#undef FALSE
-#endif
-#define FALSE                           (1 == 0)
-
-#ifdef TRUE
-#undef TRUE
-#endif
-#define TRUE                            (1 == 1)
-
-#ifndef NULL
-#define NULL                            (void *) 0
-#endif
-
 /*
  * Mescellaneous types
  */
@@ -503,7 +487,7 @@  typedef u32 acpi_event_type;
  * Event Status - Per event
  * -------------
  * The encoding of acpi_event_status is illustrated below.
- * Note that a set bit (1) indicates the property is TRUE
+ * Note that a set bit (1) indicates the property is true
  * (e.g. if bit 0 is set then the event is enabled).
  * +-------------+-+-+-+
  * |   Bits 31:3 |2|1|0|
diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h
index 2b0bf40bf2..08ab70e668 100644
--- a/xen/include/asm-arm/arm64/efibind.h
+++ b/xen/include/asm-arm/arm64/efibind.h
@@ -107,7 +107,7 @@  typedef uint64_t   UINTN;
 #define POST_CODE(_Data)
 
 
-#define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
+#define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
 
 //
 // Pointers must be aligned to these address to function
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 0e3e56b452..f93e1d42b0 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -53,7 +53,7 @@  static inline bool guest_mode(const struct cpu_user_regs *r)
     ASSERT(diff < STACK_SIZE);
     /* If not a guest frame, it must be a hypervisor frame. */
     ASSERT((diff == 0) || hyp_mode(r));
-    /* Return TRUE if it's a guest frame. */
+    /* Return true if it's a guest frame. */
     return (diff == 0);
 }
 
diff --git a/xen/include/asm-x86/regs.h b/xen/include/asm-x86/regs.h
index 725a664e0a..679c38bb76 100644
--- a/xen/include/asm-x86/regs.h
+++ b/xen/include/asm-x86/regs.h
@@ -11,7 +11,7 @@ 
     ASSERT(diff < STACK_SIZE);                                                \
     /* If not a guest frame, it must be a hypervisor frame. */                \
     ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                        \
-    /* Return TRUE if it's a guest frame. */                                  \
+    /* Return true if it's a guest frame. */                                  \
     (diff == 0);                                                              \
 })
 
diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h
index b013db175d..2b7001f8f4 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -127,7 +127,7 @@  typedef uint64_t   UINTN;
 #ifdef EFI_NT_EMULATOR
     #define BREAKPOINT()        __asm { int 3 }
 #else
-    #define BREAKPOINT()        while (TRUE);    // Make it hang on Bios[Dbg]32
+    #define BREAKPOINT()        while (true);    // Make it hang on Bios[Dbg]32
 #endif
 
 //
diff --git a/xen/include/efi/efidef.h b/xen/include/efi/efidef.h
index 86a7e111bf..fe1750de51 100644
--- a/xen/include/efi/efidef.h
+++ b/xen/include/efi/efidef.h
@@ -22,16 +22,7 @@  Revision History
 
 typedef UINT16          CHAR16;
 typedef UINT8           CHAR8;
-typedef UINT8           BOOLEAN;
-
-#ifndef TRUE
-    #define TRUE    ((BOOLEAN) 1)
-    #define FALSE   ((BOOLEAN) 0)
-#endif
-
-#ifndef NULL
-    #define NULL    ((VOID *) 0)
-#endif
+typedef bool            BOOLEAN;
 
 typedef UINTN           EFI_STATUS;
 typedef UINT64          EFI_LBA;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8d0ddfb60c..2b5ae8cae4 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -607,7 +607,7 @@  int __must_check donate_page(struct domain *d, struct page_info *page,
 #define RAM_TYPE_UNUSABLE     0x00000004
 #define RAM_TYPE_ACPI         0x00000008
 #define RAM_TYPE_UNKNOWN      0x00000010
-/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
+/* true if the whole page at @mfn is of the requested RAM type(s) above. */
 int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
 /* Returns the page type(s). */
 unsigned int page_get_ram_type(mfn_t mfn);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 9f7bc69293..c43d9311aa 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -552,7 +552,7 @@  static inline bool is_system_domain(const struct domain *d)
 
 /*
  * Use this when you don't have an existing reference to @d. It returns
- * FALSE if @d is being destroyed.
+ * false if @d is being destroyed.
  */
 static always_inline int get_domain(struct domain *d)
 {