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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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