Re: [XEN v2 09/12] xen/Arm: GICv3: Define GIC registers for AArch32

[Julien Grall <julien@xxxxxxx>]

On 03/11/2022 15:08, Michal Orzel wrote:
> Hi Ayan,
>
> On 31/10/2022 16:13, Ayan Kumar Halder wrote:
> >
> The title is a bit ambiguous given that the previous patches were also defining 
> GIC registers.
> Maybe adding "remaining" would result in a better commit title.
>
> > Refer "Arm IHI 0069H ID020922"
> > 12.5.23 ICC_SGI1R, Interrupt Controller Software Generated Interrupt
> > Group 1 Register
> > 12.5.12 ICC_HSRE, Interrupt Controller Hyp System Register Enable register
> > 12.7.10 ICH_VTR, Interrupt Controller VGIC Type Register
> > 12.7.5 ICH_HCR, Interrupt Controller Hyp Control Register
> > 12.5.20 ICC_PMR, Interrupt Controller Interrupt Priority Mask Register
> > 12.5.24 ICC_SRE, Interrupt Controller System Register Enable register
> > 12.5.7 ICC_DIR, Interrupt Controller Deactivate Interrupt Register
> > 12.5.9 ICC_EOIR1, Interrupt Controller End Of Interrupt Register 1
> > 12.5.14 ICC_IAR1, Interrupt Controller Interrupt Acknowledge Register 1
> > 12.5.5 ICC_BPR1, Interrupt Controller Binary Point Register 1
> > 12.5.6 ICC_CTLR, Interrupt Controller Control Register
> > 12.5.16 ICC_IGRPEN1, Interrupt Controller Interrupt Group 1 Enable register
> > 12.7.9 ICH_VMCR, Interrupt Controller Virtual Machine Control Register
> As said in the previous patches: this may be my personal opinion but sth like 
> this would be easier to read:
> "
> Define missing assembly aliases for GIC registers on arm32, taking the ones
> defined already for arm64 as a base. Aliases are defined according to the
> GIC Architecture Specification ARM IHI 0069H.
> "
+1 with one remark. I think listing the registers added in the commit 
message (no need for the section) is fine.
> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> > ---
> >
> > Changes from :-
> > v1 - 1. Moved coproc regs definition to asm/cpregs.h
> >
> >   xen/arch/arm/include/asm/cpregs.h | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/cpregs.h 
> > b/xen/arch/arm/include/asm/cpregs.h
> > index bfabee0bc3..62b63f4cef 100644
> > --- a/xen/arch/arm/include/asm/cpregs.h
> > +++ b/xen/arch/arm/include/asm/cpregs.h
> > @@ -415,6 +415,22 @@
> >   #define ICH_AP1R1_EL2             __AP1Rx_EL2(1)
> >   #define ICH_AP1R2_EL2             __AP1Rx_EL2(2)
> >   #define ICH_AP1R3_EL2             __AP1Rx_EL2(3)
> > +
> > +#define ICC_SGI1R_EL1             p15,0,c12
> > +
> > +#define ICC_SRE_EL2               p15,4,c12,c9,5
> > +#define ICH_VTR_EL2               p15,4,c12,c11,1
> > +#define ICH_HCR_EL2               p15,4,c12,c11,0
> > +
> > +#define ICC_PMR_EL1               p15,0,c4,c6,0
> > +#define ICC_SRE_EL1               p15,0,c12,c12,5
> > +#define ICC_DIR_EL1               p15,0,c12,c11,1
> > +#define ICC_EOIR1_EL1             p15,0,c12,c12,1
> > +#define ICC_IAR1_EL1              p15,0,c12,c12,0
> > +#define ICC_BPR1_EL1              p15,0,c12,c12,3
> > +#define ICC_CTLR_EL1              p15,0,c12,c12,4
> > +#define ICC_IGRPEN1_EL1           p15,0,c12,c12,7
> > +#define ICH_VMCR_EL2              p15,4,c12,c11,7
> I did not check this in previous patches but in which order are you defining 
> these registers?
> I took a look at arm64/sysregs.h and these regs are placed in assembly aliases 
> name order.
> So for instance ICC_PMR_EL1 would be defined before ICC_SRE_EL2, etc.
Per the comment in the header, they should be ordered as followed:

Coprocessor-> CRn-> Opcode 1-> CRm-> Opcode 2

Also, we first define the arm32 name and *then* define an alias for 
common code.
These remarks applies for the full series.

Cheers,

--
Julien Grall