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

[Julien Grall <julien@xxxxxxx>]

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