Re: [PATCH 3/3] xen/public: arch-arm: All PSR_* defines should be unsigned

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: Julien Grall <jgrall@amazon.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 3/3] xen/public: arch-arm: All PSR_* defines should be unsigned
Date: Fri, 18 Aug 2023 08:33:57 +0200	[thread overview]
Message-ID: <93ccb2c0-fa2c-3e84-35bf-3d86aef7a2bd@suse.com> (raw)
In-Reply-To: <20230817214356.47174-4-julien@xen.org>

On 17.08.2023 23:43, Julien Grall wrote:
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -339,36 +339,36 @@ typedef uint64_t xen_callback_t;
>  
>  /* PSR bits (CPSR, SPSR) */
>  
> -#define PSR_THUMB       (1<<5)        /* Thumb Mode enable */
> -#define PSR_FIQ_MASK    (1<<6)        /* Fast Interrupt mask */
> -#define PSR_IRQ_MASK    (1<<7)        /* Interrupt mask */
> -#define PSR_ABT_MASK    (1<<8)        /* Asynchronous Abort mask */
> -#define PSR_BIG_ENDIAN  (1<<9)        /* arm32: Big Endian Mode */
> -#define PSR_DBG_MASK    (1<<9)        /* arm64: Debug Exception mask */
> -#define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> -#define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
> -#define PSR_Z           (1<<30)       /* Zero condition flag */
> +#define PSR_THUMB       (1U <<5)      /* Thumb Mode enable */
> +#define PSR_FIQ_MASK    (1U <<6)      /* Fast Interrupt mask */
> +#define PSR_IRQ_MASK    (1U <<7)      /* Interrupt mask */
> +#define PSR_ABT_MASK    (1U <<8)      /* Asynchronous Abort mask */

Nit: Did you mean to insert blanks also on the rhs of the <<, like you ...

> +#define PSR_BIG_ENDIAN  (1U << 9)     /* arm32: Big Endian Mode */
> +#define PSR_DBG_MASK    (1U << 9)     /* arm64: Debug Exception mask */
> +#define PSR_IT_MASK     (0x0600fc00U) /* Thumb If-Then Mask */
> +#define PSR_JAZELLE     (1U << 24)    /* Jazelle Mode */
> +#define PSR_Z           (1U << 30)    /* Zero condition flag */

... did everywhere here?

As an aside I wonder why they're here: They look like definitions of
processor registers, which aren't under our (Xen's) control. I ask in
part because the presence of such constants may then be taken as
justification to add similar things in new ports. Yet such definitions
shouldn't be put here.

Jan