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 <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? > 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
