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

[Michal Orzel <michal.orzel@xxxxxxx>]

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

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