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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> 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.
> 
> Defined the following registers:-
> 1. Interrupt Controller Interrupt Priority Mask Register
> 2. Interrupt Controller System Register Enable register
> 3. Interrupt Controller Deactivate Interrupt Register
> 4. Interrupt Controller End Of Interrupt Register 1
> 5. Interrupt Controller Interrupt Acknowledge Register 1
> 6. Interrupt Controller Binary Point Register 1
> 7. Interrupt Controller Control Register
> 8. Interrupt Controller Interrupt Group 1 Enable register
> 9. Interrupt Controller Maintenance Interrupt State Register
> 10. Interrupt Controller End of Interrupt Status Register
> 11. Interrupt Controller Empty List Register Status Register
> 12. Interrupt Controller Virtual Machine Control Register
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 
> Changes from :-
> v1 - 1. Moved coproc regs definition to asm/cpregs.h
> 
> v2 - 1. Defined register alias.
> 2. Style issues.
> 3. Defined ELSR, MISR, EISR to make it consistent with AArch64.
> 
>  xen/arch/arm/include/asm/cpregs.h | 32 +++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/cpregs.h 
> b/xen/arch/arm/include/asm/cpregs.h
> index 5331ec3448..0fc606fe99 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -161,6 +161,7 @@
>  #define DACR            p15,0,c3,c0,0   /* Domain Access Control Register */
>  
>  /* CP15 CR4: */
> +#define ICC_PMR         p15,0,c4,c6,0   /* Interrupt Priority Mask Register 
> */
>  
>  /* CP15 CR5: Fault Status Registers */
>  #define DFSR            p15,0,c5,c0,0   /* Data Fault Status Register */
> @@ -254,6 +255,8 @@
>  
>  /* CP15 CR12:  */
>  #define ICC_SGI1R       p15,0,c12       /* Interrupt Controller SGI Group 1 
> */
> +#define ICC_DIR         p15,0,c12,c11,1 /* Interrupt Controller Deactivate 
> Interrupt Register */
> +#define ICC_SRE_L1      p15,0,c12,c12,5 /* Interrupt Controller System 
> Register Enable register */
There is no such register. It should be ICC_SRE.
Also it looks like these are not placed in the correct order. ICC_SRE with its 
assembly alias should be placed...

>  #define ICC_ASGI1R      p15,1,c12       /* Interrupt Controller Alias SGI 
> Group 1 Register */
>  #define ICC_SGI0R       p15,2,c12       /* Interrupt Controller SGI Group 0 
> */
>  #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
> @@ -279,6 +282,19 @@
>  #define ICH_AP1R2       __AP1Rx(2)
>  #define ICH_AP1R3       __AP1Rx(3)
>  
> +#define ICC_IAR1        p15,0,c12,c12,0  /* Interrupt Controller Interrupt 
> Acknowledge Register 1 */
> +#define ICC_EOIR1       p15,0,c12,c12,1  /* Interrupt Controller End Of 
> Interrupt Register 1 */
> +#define ICC_BPR1        p15,0,c12,c12,3  /* Interrupt Controller Binary 
> Point Register 1 */
> +#define ICC_CTLR        p15,0,c12,c12,4  /* Interrupt Controller Control 
> Register */
here...
> +#define ICC_IGRPEN1     p15,0,c12,c12,7  /* Interrupt Controller Interrupt 
> Group 1 Enable register */
> +#define ICC_SRE         p15,4,c12,c9,5   /* Interrupt Controller Hyp System 
> Register Enable register */
This one should be ICC_HSRE.

> +#define ICH_HCR         p15,4,c12,c11,0  /* Interrupt Controller Hyp Control 
> Register */
> +#define ICH_VTR         p15,4,c12,c11,1  /* Interrupt Controller VGIC Type 
> Register */
> +#define ICH_MISR        p15,4,c12,c11,2  /* Interrupt Controller Maintenance 
> Interrupt State Register */
> +#define ICH_EISR        p15,4,c12,c11,3  /* Interrupt Controller End of 
> Interrupt Status Register */
> +#define ICH_ELRSR       p15,4,c12,c11,5  /* Interrupt Controller Empty List 
> Register Status Register */
> +#define ICH_VMCR        p15,4,c12,c11,7  /* Interrupt Controller Virtual 
> Machine Control Register */
> +
>  /* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
>  #define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
>  #define __LR0(x)                  ___CP32(p15, 4, c12, c12, x)
> @@ -380,6 +396,15 @@
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
> +#define ICC_BPR1_EL1            ICC_BPR1
> +#define ICC_CTLR_EL1            ICC_CTLR
> +#define ICC_DIR_EL1             ICC_DIR
> +#define ICC_EOIR1_EL1           ICC_EOIR1
> +#define ICC_IGRPEN1_EL1         ICC_IGRPEN1
> +#define ICC_PMR_EL1             ICC_PMR
> +#define ICC_SGI1R_EL1           ICC_SGI1R
> +#define ICC_SRE_EL1             ICC_SRE_L1
This should be:
#define ICC_SRE_EL1 ICC_SRE

> +#define ICC_SRE_EL2             ICC_SRE
This should be:
#define ICC_SRE_EL2 ICC_HSRE

>  #define ICH_AP0R0_EL2           ICH_AP0R0
>  #define ICH_AP0R1_EL2           ICH_AP0R1
>  #define ICH_AP0R2_EL2           ICH_AP0R2
> @@ -388,6 +413,10 @@
>  #define ICH_AP1R1_EL2           ICH_AP1R1
>  #define ICH_AP1R2_EL2           ICH_AP1R2
>  #define ICH_AP1R3_EL2           ICH_AP1R3
> +#define ICH_EISR_EL2            ICH_EISR
> +#define ICH_ELRSR_EL2           ICH_ELRSR
> +#define ICH_HCR_EL2             ICH_HCR
> +#define ICC_IAR1_EL1            ICC_IAR1
>  #define ICH_LR0_EL2             ICH_LR0
>  #define ICH_LR1_EL2             ICH_LR1
>  #define ICH_LR2_EL2             ICH_LR2
> @@ -420,6 +449,9 @@
>  #define ICH_LRC13_EL2           ICH_LRC13
>  #define ICH_LRC14_EL2           ICH_LRC14
>  #define ICH_LRC15_EL2           ICH_LRC15
> +#define ICH_MISR_EL2            ICH_MISR
> +#define ICH_VMCR_EL2            ICH_VMCR
> +#define ICH_VTR_EL2             ICH_VTR
>  #define ID_AFR0_EL1             ID_AFR0
>  #define ID_DFR0_EL1             ID_DFR0
>  #define ID_DFR1_EL1             ID_DFR1

~Michal