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
Signed-off-by: Ayan Kumar Halder <ayankuma@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
#endif
#endif
--
2.17.1Hi 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 <ayankuma@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
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@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. 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
On 03/11/2022 15:08, Michal Orzel wrote: > Hi Ayan, Hi Michal, > > 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 <ayankuma@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? My bad, I did not follow any particular order. > 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. This makes sense. I will fix this in v3. > > 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? Actually these registers are not being used by arm64 as well. A grep for "ICH_MISR" or "ICH_EISR" did not return any usage of these registers ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri ICH_MISR * xen/arch/arm/include/asm/gic.h:#define GICH_MISR (0x10) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_EOI (1 << 0) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_U (1 << 1) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_LRENP (1 << 2) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_NP (1 << 3) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0E (1 << 4) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0D (1 << 5) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1E (1 << 6) xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1D (1 << 7) xen/arch/arm/include/asm/arm64/sysregs.h:#define ICH_MISR_EL2 S3_4_C12_C11_2 ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri ICH_EISR * xen/arch/arm/include/asm/gic.h:#define GICH_EISR0 (0x20) xen/arch/arm/include/asm/gic.h:#define GICH_EISR1 (0x24) xen/arch/arm/include/asm/arm64/sysregs.h:#define ICH_EISR_EL2 S3_4_C12_C11_3 As I see, they seem deadcode for me. Do you suggest that we should remove them ? If so, I can send a patch for this. - Ayan > > ~Michal
Hi Ayan, On 03/11/2022 21:14, Ayan Kumar Halder wrote: > > On 03/11/2022 15:08, Michal Orzel wrote: >> Hi Ayan, > Hi Michal, >> >> 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 <ayankuma@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? > My bad, I did not follow any particular order. >> 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. > This makes sense. I will fix this in v3. >> >> 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? > > Actually these registers are not being used by arm64 as well. A grep for > "ICH_MISR" or "ICH_EISR" did not return any usage of these registers > > ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri > ICH_MISR * > xen/arch/arm/include/asm/gic.h:#define GICH_MISR (0x10) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_EOI (1 << 0) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_U (1 << 1) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_LRENP (1 << 2) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_NP (1 << 3) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0E (1 << 4) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0D (1 << 5) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1E (1 << 6) > xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1D (1 << 7) > xen/arch/arm/include/asm/arm64/sysregs.h:#define > ICH_MISR_EL2 S3_4_C12_C11_2 > > ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri > ICH_EISR * > xen/arch/arm/include/asm/gic.h:#define GICH_EISR0 (0x20) > xen/arch/arm/include/asm/gic.h:#define GICH_EISR1 (0x24) > xen/arch/arm/include/asm/arm64/sysregs.h:#define > ICH_EISR_EL2 S3_4_C12_C11_3 > > As I see, they seem deadcode for me. Macros are preprocessor constructs whose content is replaced whenever the name is used. I would not call this a deadcode as this is not something that can be executed. If a macro is not used, its content will not appear in the actual code. > > Do you suggest that we should remove them ? If so, I can send a patch > for this. This is a question to maintainers. Bare in mind that we really have a lot of unused macros in Xen codebase. IMO, if we decide to remove them, this should be done in a single series, so no need to add another additional patch in your series, especially if you are not modifying this code directly. > > - Ayan > >> >> ~Michal ~Michal
On 04/11/2022 09:27, Michal Orzel wrote: > Hi Ayan, Hi Michal, > > On 03/11/2022 21:14, Ayan Kumar Halder wrote: >> On 03/11/2022 15:08, Michal Orzel wrote: >>> Hi Ayan, >> Hi Michal, >>> 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 <ayankuma@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? >> My bad, I did not follow any particular order. >>> 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. >> This makes sense. I will fix this in v3. >>> 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? >> Actually these registers are not being used by arm64 as well. A grep for >> "ICH_MISR" or "ICH_EISR" did not return any usage of these registers >> >> ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri >> ICH_MISR * >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR (0x10) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_EOI (1 << 0) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_U (1 << 1) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_LRENP (1 << 2) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_NP (1 << 3) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0E (1 << 4) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP0D (1 << 5) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1E (1 << 6) >> xen/arch/arm/include/asm/gic.h:#define GICH_MISR_VGRP1D (1 << 7) >> xen/arch/arm/include/asm/arm64/sysregs.h:#define >> ICH_MISR_EL2 S3_4_C12_C11_2 >> >> ayankuma@xcbayankuma41x:/scratch/ayankuma/upstream_xen/xen$ grep -ri >> ICH_EISR * >> xen/arch/arm/include/asm/gic.h:#define GICH_EISR0 (0x20) >> xen/arch/arm/include/asm/gic.h:#define GICH_EISR1 (0x24) >> xen/arch/arm/include/asm/arm64/sysregs.h:#define >> ICH_EISR_EL2 S3_4_C12_C11_3 >> >> As I see, they seem deadcode for me. > Macros are preprocessor constructs whose content is replaced whenever the name is used. > I would not call this a deadcode as this is not something that can be executed. > If a macro is not used, its content will not appear in the actual code. This makes sense. > >> Do you suggest that we should remove them ? If so, I can send a patch >> for this. > This is a question to maintainers. > Bare in mind that we really have a lot of unused macros in Xen codebase. > IMO, if we decide to remove them, this should be done in a single series, > so no need to add another additional patch in your series, especially if you > are not modifying this code directly. Yes, I will keep these macros intact (as it exists). WRT your question "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?" These registers are not used by Xen. Should I define these registers for the sake of completeness (to be in parity with AArch64) ? - Ayan > >> - Ayan >> >>> ~Michal > ~Michal
Hi Ayan, On 04/11/2022 10:04, Ayan Kumar Halder wrote: > These registers are not used by Xen. > Should I define these registers for the sake of completeness (to be in > parity with AArch64) ? Yes. I would at least expect the MISR might end up to be used if we were supporting some interrupts controlled (e.g. the Apple Interrupt Controller). Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.