diff mbox series

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

Message ID 20230817214356.47174-4-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series xen/arm: Some clean-up found with -Wconversion and -Warith-conversion | expand

Commit Message

Julien Grall Aug. 17, 2023, 9:43 p.m. UTC 
From: Julien Grall <jgrall@amazon.com>

The defines PSR_* are field in registers and always unsigned. So
add 'U' to clarify.

This should help with MISRA Rule 7.2.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/public/arch-arm.h | 52 +++++++++++++++++------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

Comments

Stefano Stabellini Aug. 17, 2023, 11:05 p.m. UTC | #1 
On Thu, 17 Aug 2023, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The defines PSR_* are field in registers and always unsigned. So
> add 'U' to clarify.
> 
> This should help with MISRA Rule 7.2.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


> ---
>  xen/include/public/arch-arm.h | 52 +++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c6449893e493..492819ad22c9 100644
> --- 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 */
> +#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 */
>  
>  /* 32 bit modes */
> -#define PSR_MODE_USR 0x10
> -#define PSR_MODE_FIQ 0x11
> -#define PSR_MODE_IRQ 0x12
> -#define PSR_MODE_SVC 0x13
> -#define PSR_MODE_MON 0x16
> -#define PSR_MODE_ABT 0x17
> -#define PSR_MODE_HYP 0x1a
> -#define PSR_MODE_UND 0x1b
> -#define PSR_MODE_SYS 0x1f
> +#define PSR_MODE_USR 0x10U
> +#define PSR_MODE_FIQ 0x11U
> +#define PSR_MODE_IRQ 0x12U
> +#define PSR_MODE_SVC 0x13U
> +#define PSR_MODE_MON 0x16U
> +#define PSR_MODE_ABT 0x17U
> +#define PSR_MODE_HYP 0x1aU
> +#define PSR_MODE_UND 0x1bU
> +#define PSR_MODE_SYS 0x1fU
>  
>  /* 64 bit modes */
> -#define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
> -#define PSR_MODE_EL3h 0x0d
> -#define PSR_MODE_EL3t 0x0c
> -#define PSR_MODE_EL2h 0x09
> -#define PSR_MODE_EL2t 0x08
> -#define PSR_MODE_EL1h 0x05
> -#define PSR_MODE_EL1t 0x04
> -#define PSR_MODE_EL0t 0x00
> +#define PSR_MODE_BIT  0x10U /* Set iff AArch32 */
> +#define PSR_MODE_EL3h 0x0dU
> +#define PSR_MODE_EL3t 0x0cU
> +#define PSR_MODE_EL2h 0x09U
> +#define PSR_MODE_EL2t 0x08U
> +#define PSR_MODE_EL1h 0x05U
> +#define PSR_MODE_EL1t 0x04U
> +#define PSR_MODE_EL0t 0x00U
>  
>  /*
>   * We set PSR_Z to be able to boot Linux kernel versions with an invalid
> -- 
> 2.40.1
>
Henry Wang Aug. 18, 2023, 1:50 a.m. UTC | #2 
Hi Julien,

> On Aug 18, 2023, at 05:43, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The defines PSR_* are field in registers and always unsigned. So
> add 'U' to clarify.
> 
> This should help with MISRA Rule 7.2.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Tested-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry
Jan Beulich Aug. 18, 2023, 6:33 a.m. UTC | #3 
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
Julien Grall Aug. 18, 2023, 7:39 a.m. UTC | #4 
Hi Jan,

On 18/08/2023 07:33, Jan Beulich wrote:
> 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?

Yes I did. I will update the patch.

> 
> 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 agree they are not under Xen's control. However, they are used by the 
toolstack and IIRC back then they were not available in any other headers.

Note that they are only available by the tools and the hypervisor (see 
#ifdef above).

> 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.

 From my understanding we are using the public headers to provide 
macros/defines that are used by both the toolstack and the hypervisor. 
If they are not meant to be exposed to the guest, then they will be 
protected with "#if defined(__XEN__) || defined(__XEN_TOOLS__)".

I think we are in a similar situation here. So it is not clear where 
they should be put if we need to share them between the hypervisor and 
the toolstack.

Cheers,
Jürgen Groß Aug. 18, 2023, 8 a.m. UTC | #5 
On 18.08.23 09:39, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2023 07:33, Jan Beulich wrote:
>> 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?
> 
> Yes I did. I will update the patch.
> 
>>
>> 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 agree they are not under Xen's control. However, they are used by the 
> toolstack and IIRC back then they were not available in any other headers.
> 
> Note that they are only available by the tools and the hypervisor (see #ifdef 
> above).
> 
>> 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.
> 
>  From my understanding we are using the public headers to provide macros/defines 
> that are used by both the toolstack and the hypervisor. If they are not meant to 
> be exposed to the guest, then they will be protected with "#if defined(__XEN__) 
> || defined(__XEN_TOOLS__)".
> 
> I think we are in a similar situation here. So it is not clear where they should 
> be put if we need to share them between the hypervisor and the toolstack.

What about include/xen/lib? There are headers below that linked at build time
via tools/include/Makefile to tools/include/xen/lib.


Juergen
Julien Grall Aug. 18, 2023, 8:05 a.m. UTC | #6 
Hi,

On 18/08/2023 09:00, Juergen Gross wrote:
> On 18.08.23 09:39, Julien Grall wrote:
>> Hi Jan,
>>
>> On 18/08/2023 07:33, Jan Beulich wrote:
>>> 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?
>>
>> Yes I did. I will update the patch.
>>
>>>
>>> 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 agree they are not under Xen's control. However, they are used by 
>> the toolstack and IIRC back then they were not available in any other 
>> headers.
>>
>> Note that they are only available by the tools and the hypervisor (see 
>> #ifdef above).
>>
>>> 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.
>>
>>  From my understanding we are using the public headers to provide 
>> macros/defines that are used by both the toolstack and the hypervisor. 
>> If they are not meant to be exposed to the guest, then they will be 
>> protected with "#if defined(__XEN__) || defined(__XEN_TOOLS__)".
>>
>> I think we are in a similar situation here. So it is not clear where 
>> they should be put if we need to share them between the hypervisor and 
>> the toolstack.
> 
> What about include/xen/lib? There are headers below that linked at build 
> time
> via tools/include/Makefile to tools/include/xen/lib.

To me, the question is why would we want to move PSR_* in xen/lib (or 
whatever name we decide) but all the other defines that are only used by 
the toolstack would still be in public/.

So are you suggesting to move all the tools only information in xen/lib?

Cheers,
Jan Beulich Aug. 18, 2023, 8:14 a.m. UTC | #7 
On 18.08.2023 09:39, Julien Grall wrote:
> On 18/08/2023 07:33, Jan Beulich wrote:
>> 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 agree they are not under Xen's control. However, they are used by the 
> toolstack and IIRC back then they were not available in any other headers.
> 
> Note that they are only available by the tools and the hypervisor (see 
> #ifdef above).

Yes, I did notice that (or else I would have used stronger wording).

>> 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.
> 
>  From my understanding we are using the public headers to provide 
> macros/defines that are used by both the toolstack and the hypervisor. 
> If they are not meant to be exposed to the guest, then they will be 
> protected with "#if defined(__XEN__) || defined(__XEN_TOOLS__)".
> 
> I think we are in a similar situation here. So it is not clear where 
> they should be put if we need to share them between the hypervisor and 
> the toolstack.

On x86 we simply arrange for certain hypervisor headers to be re-usable
from the toolstack. See in particular arch/x86/include/asm/x86-*.h. And
of course everything under include/xen/lib/x86/, but those are our own
definitions, not ones meant to solely express relevant hw spec aspects.

Jan
Jürgen Groß Aug. 18, 2023, 8:25 a.m. UTC | #8 
On 18.08.23 10:05, Julien Grall wrote:
> Hi,
> 
> On 18/08/2023 09:00, Juergen Gross wrote:
>> On 18.08.23 09:39, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 18/08/2023 07:33, Jan Beulich wrote:
>>>> 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?
>>>
>>> Yes I did. I will update the patch.
>>>
>>>>
>>>> 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 agree they are not under Xen's control. However, they are used by the 
>>> toolstack and IIRC back then they were not available in any other headers.
>>>
>>> Note that they are only available by the tools and the hypervisor (see #ifdef 
>>> above).
>>>
>>>> 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.
>>>
>>>  From my understanding we are using the public headers to provide 
>>> macros/defines that are used by both the toolstack and the hypervisor. If 
>>> they are not meant to be exposed to the guest, then they will be protected 
>>> with "#if defined(__XEN__) || defined(__XEN_TOOLS__)".
>>>
>>> I think we are in a similar situation here. So it is not clear where they 
>>> should be put if we need to share them between the hypervisor and the toolstack.
>>
>> What about include/xen/lib? There are headers below that linked at build time
>> via tools/include/Makefile to tools/include/xen/lib.
> 
> To me, the question is why would we want to move PSR_* in xen/lib (or whatever 
> name we decide) but all the other defines that are only used by the toolstack 
> would still be in public/.
> 
> So are you suggesting to move all the tools only information in xen/lib?

I didn't want to suggest that, especially with our general desire to switch the
tools' interfaces to stable ones.

I just wanted to point out that there are other locations available already
where such information could be shared between hypervisor and tools. Especially
information related to hardware (so not an interface we are defining) might be
a good candidate for such an alternative location.

But in the end it is not me to decide what to use. I was just giving a hint. :-)


Juergen
Julien Grall Aug. 18, 2023, 9:15 a.m. UTC | #9 
Hi Juergen,

On 18/08/2023 09:25, Juergen Gross wrote:
> On 18.08.23 10:05, Julien Grall wrote:
>> Hi,
>>
>> On 18/08/2023 09:00, Juergen Gross wrote:
>>> On 18.08.23 09:39, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 18/08/2023 07:33, Jan Beulich wrote:
>>>>> 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?
>>>>
>>>> Yes I did. I will update the patch.
>>>>
>>>>>
>>>>> 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 agree they are not under Xen's control. However, they are used by 
>>>> the toolstack and IIRC back then they were not available in any 
>>>> other headers.
>>>>
>>>> Note that they are only available by the tools and the hypervisor 
>>>> (see #ifdef above).
>>>>
>>>>> 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.
>>>>
>>>>  From my understanding we are using the public headers to provide 
>>>> macros/defines that are used by both the toolstack and the 
>>>> hypervisor. If they are not meant to be exposed to the guest, then 
>>>> they will be protected with "#if defined(__XEN__) || 
>>>> defined(__XEN_TOOLS__)".
>>>>
>>>> I think we are in a similar situation here. So it is not clear where 
>>>> they should be put if we need to share them between the hypervisor 
>>>> and the toolstack.
>>>
>>> What about include/xen/lib? There are headers below that linked at 
>>> build time
>>> via tools/include/Makefile to tools/include/xen/lib.
>>
>> To me, the question is why would we want to move PSR_* in xen/lib (or 
>> whatever name we decide) but all the other defines that are only used 
>> by the toolstack would still be in public/.
>>
>> So are you suggesting to move all the tools only information in xen/lib?
> 
> I didn't want to suggest that, especially with our general desire to 
> switch the
> tools' interfaces to stable ones.

Ok. If there are a desire to switch the tools interface to stables one. 
Then...

> 
> I just wanted to point out that there are other locations available already
> where such information could be shared between hypervisor and tools. 
> Especially
> information related to hardware (so not an interface we are defining) 
> might be
> a good candidate for such an alternative location.

... I disagree with moving PSR_* outside of the public interface because 
they are those defines are used in hypercalls. So they would be part of 
the ABI.

Cheers,
Julien Grall Aug. 18, 2023, 9:21 a.m. UTC | #10 
Hi Jan,

On 18/08/2023 09:14, Jan Beulich wrote:
> On 18.08.2023 09:39, Julien Grall wrote:
>> On 18/08/2023 07:33, Jan Beulich wrote:
>>> 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 agree they are not under Xen's control. However, they are used by the
>> toolstack and IIRC back then they were not available in any other headers.
>>
>> Note that they are only available by the tools and the hypervisor (see
>> #ifdef above).
> 
> Yes, I did notice that (or else I would have used stronger wording).
> 
>>> 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.
>>
>>   From my understanding we are using the public headers to provide
>> macros/defines that are used by both the toolstack and the hypervisor.
>> If they are not meant to be exposed to the guest, then they will be
>> protected with "#if defined(__XEN__) || defined(__XEN_TOOLS__)".
>>
>> I think we are in a similar situation here. So it is not clear where
>> they should be put if we need to share them between the hypervisor and
>> the toolstack.
> 
> On x86 we simply arrange for certain hypervisor headers to be re-usable
> from the toolstack. See in particular arch/x86/include/asm/x86-*.h. And
> of course everything under include/xen/lib/x86/, but those are our own
> definitions, not ones meant to solely express relevant hw spec aspects.

Even if they are defined by the HW, we still need them to easily create 
some hypercalls field.

If we are planning to have a stable toolstack ABI (as Juergen 
suggested), then I would find a bit odd that onewould need to include 
lib/ (or provide their own header) in order to update the fields.

Anyway, I am not planning to do any re-ordering anytime soon for the 
public. But I would be happy to take part of the discussion if there are 
a general agreement to split further and someone wants to write patches.

Cheers,
diff mbox series

Patch

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c6449893e493..492819ad22c9 100644
--- 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 */
+#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 */
 
 /* 32 bit modes */
-#define PSR_MODE_USR 0x10
-#define PSR_MODE_FIQ 0x11
-#define PSR_MODE_IRQ 0x12
-#define PSR_MODE_SVC 0x13
-#define PSR_MODE_MON 0x16
-#define PSR_MODE_ABT 0x17
-#define PSR_MODE_HYP 0x1a
-#define PSR_MODE_UND 0x1b
-#define PSR_MODE_SYS 0x1f
+#define PSR_MODE_USR 0x10U
+#define PSR_MODE_FIQ 0x11U
+#define PSR_MODE_IRQ 0x12U
+#define PSR_MODE_SVC 0x13U
+#define PSR_MODE_MON 0x16U
+#define PSR_MODE_ABT 0x17U
+#define PSR_MODE_HYP 0x1aU
+#define PSR_MODE_UND 0x1bU
+#define PSR_MODE_SYS 0x1fU
 
 /* 64 bit modes */
-#define PSR_MODE_BIT  0x10 /* Set iff AArch32 */
-#define PSR_MODE_EL3h 0x0d
-#define PSR_MODE_EL3t 0x0c
-#define PSR_MODE_EL2h 0x09
-#define PSR_MODE_EL2t 0x08
-#define PSR_MODE_EL1h 0x05
-#define PSR_MODE_EL1t 0x04
-#define PSR_MODE_EL0t 0x00
+#define PSR_MODE_BIT  0x10U /* Set iff AArch32 */
+#define PSR_MODE_EL3h 0x0dU
+#define PSR_MODE_EL3t 0x0cU
+#define PSR_MODE_EL2h 0x09U
+#define PSR_MODE_EL2t 0x08U
+#define PSR_MODE_EL1h 0x05U
+#define PSR_MODE_EL1t 0x04U
+#define PSR_MODE_EL0t 0x00U
 
 /*
  * We set PSR_Z to be able to boot Linux kernel versions with an invalid