Re: [XEN v2 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32

[Julien Grall <julien@xxxxxxx>]

On 03/11/2022 12:35, Michal Orzel wrote:
> Hi Ayan,
>
> On 31/10/2022 16:13, Ayan Kumar Halder wrote:
> > Refer "Arm IHI 0069H ID020922",
> > 12.7.1 - Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
> > 12.7.2 - Interrupt Controller Hyp Active Priorities Group1 Registers 0-3
> Commit msg like this is not really beneficial as it requires someone to have
> this spec and only tells that such registers exist on AArch32. It is missing 
> some
> information e.g. what is the purpose of defining them.
+1

> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> > ---
> >
> > Changes from :-
> > v1 - 1. Moved coproc register definition to asm/cpregs.h.
> >
> >   xen/arch/arm/include/asm/arm32/sysregs.h |  1 -
> >   xen/arch/arm/include/asm/cpregs.h        | 11 +++++++++++
> >   2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
> > b/xen/arch/arm/include/asm/arm32/sysregs.h
> > index 8a9a014bef..1b2915a526 100644
> > --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> > +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> > @@ -81,7 +81,6 @@
> >
> >   /* MVFR2 is not defined on ARMv7 */
> >   #define MVFR2_MAYBE_UNDEFINED
> > -
Spurious change.

> >   #endif /* __ASSEMBLY__ */
> >
> >   #endif /* __ASM_ARM_ARM32_SYSREGS_H */
> > diff --git a/xen/arch/arm/include/asm/cpregs.h 
> > b/xen/arch/arm/include/asm/cpregs.h
> > index 4421dd49ac..bfabee0bc3 100644
> > --- a/xen/arch/arm/include/asm/cpregs.h
> > +++ b/xen/arch/arm/include/asm/cpregs.h
> > @@ -404,6 +404,17 @@
> >   #define ICH_LRC14_EL2              __LRC8_EL2(6)
> >   #define ICH_LRC15_EL2              __LRC8_EL2(7)
> >
> > +#define __AP0Rx_EL2(x)            ___CP32(p15,4,c12,c8,x)
> > +#define ICH_AP0R0_EL2             __AP0Rx_EL2(0)
> > +#define ICH_AP0R1_EL2             __AP0Rx_EL2(1)
> > +#define ICH_AP0R2_EL2             __AP0Rx_EL2(2)
> > +#define ICH_AP0R3_EL2             __AP0Rx_EL2(3)
> > +
> > +#define __AP1Rx_EL2(x)            ___CP32(p15,4,c12,c9,x)
> > +#define ICH_AP1R0_EL2             __AP1Rx_EL2(0)
> > +#define ICH_AP1R1_EL2             __AP1Rx_EL2(1)
> > +#define ICH_AP1R2_EL2             __AP1Rx_EL2(2)
> > +#define ICH_AP1R3_EL2             __AP1Rx_EL2(3)
> This might need to be re-aligned after you fix patch no. 7.
> Then, you can add:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall