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. " > Signed-off-by: Ayan Kumar Halder <ayank...@amd.com> > --- > > 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. Also, I cannot see some regs like MISR, EISR that are defined for arm64. Did you decide not to define them for arm32 because they are not used by Xen? ~Michal
