[XEN v3 00/12] Arm: Enable GICv3 for AArch32

[XEN v3 00/12] Arm: Enable GICv3 for AArch32

[Ayan Kumar Halder]

Hi All,

Please find the following patches to enable GICv3 for AArch32.
This is a pre-requisite to support Xen on Cortex-R52 (AArch32-v8R system)

Let me know your thoughts.

The following patches have been reviewed and acked and contain no changes
from v2 :-
1. [XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host
2. [XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32

Changes from -

v1 :-
1. Updated in the changelog for each of the patches.

v2 :-
1. Dropped "xen/Arm: GICv3: Move the macros to compute the affnity level to
arm64/arm32". The reason being aff3 does not exist on arm32. And aff0..2 is
the same between arm32, AArch32 and AArch64.

2. Introduce a new patch "xen/Arm: GICv3: Adapt access to VMPIDR register for
AArch32".

3. For the new registers introduced, we have defined the arm32 name and then
an alias.

4. Use 'AArch32' across all the patches.

5. Dropped the 'R-b' and 'Ack' in "[XEN v3 08/12] xen/Arm: GICv3: Define
ICH_AP0R<n> and ICH_AP1R<n> for AArch32".


Ayan Kumar Halder (12):
  xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only
  xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32
  xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32
  xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32
  xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit
    host
  xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32
  xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32
  xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32
  xen/Arm: GICv3: Define remaining GIC registers for AArch32
  xen/Arm: GICv3: Use ULL instead of UL for 64bits
  xen/Arm: GICv3: Define macros to read/write 64 bit
  xen/Arm: GICv3: Enable GICv3 for AArch32

 SUPPORT.md                               |   7 +
 xen/arch/arm/Kconfig                     |   5 +-
 xen/arch/arm/gic-v3.c                    | 159 +++++++++++++----------
 xen/arch/arm/include/asm/arm32/io.h      |   9 ++
 xen/arch/arm/include/asm/arm32/sysregs.h |  19 +++
 xen/arch/arm/include/asm/arm64/sysregs.h |   4 +
 xen/arch/arm/include/asm/cpregs.h        | 135 +++++++++++++++++++
 xen/arch/arm/include/asm/cpufeature.h    |   1 +
 xen/arch/arm/include/asm/gic_v3_defs.h   |  24 ++--
 xen/arch/arm/include/asm/vreg.h          |  86 +++---------
 xen/arch/arm/vgic-v3.c                   |  18 ++-
 11 files changed, 312 insertions(+), 155 deletions(-)

-- 
2.17.1

[XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only

[Ayan Kumar Halder]

Sysreg emulation is 64-bit specific, so guard the calls to
vgic_v3_emulate_sysreg() as well as the function itself with
"#ifdef CONFIG_ARM_64".

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -
v1 - 1. Updated the commit message.

v2 - 1. Updated the commit message (removed the reference to Arm ARM as it is
not required).

 xen/arch/arm/vgic-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 015446be17..3f4509dcd3 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1519,6 +1519,7 @@ static bool vgic_v3_emulate_sgi1r(struct cpu_user_regs *regs, uint64_t *r,
     }
 }
 
+#ifdef CONFIG_ARM_64
 static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
@@ -1539,6 +1540,7 @@ static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
         return false;
     }
 }
+#endif
 
 static bool vgic_v3_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 {
@@ -1562,8 +1564,10 @@ static bool vgic_v3_emulate_reg(struct cpu_user_regs *regs, union hsr hsr)
 {
     switch (hsr.ec)
     {
+#ifdef CONFIG_ARM_64
     case HSR_EC_SYSREG:
         return vgic_v3_emulate_sysreg(regs, hsr);
+#endif
     case HSR_EC_CP15_64:
         return vgic_v3_emulate_cp64(regs, hsr);
     default:
-- 
2.17.1

Re: [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Sysreg emulation is 64-bit specific, so guard the calls to
> vgic_v3_emulate_sysreg() as well as the function itself with
> "#ifdef CONFIG_ARM_64".
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v3 01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for AArch64 only

[Julien Grall]

On 17/11/2022 13:05, Michal Orzel wrote:
> Hi Ayan,
> 
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Sysreg emulation is 64-bit specific, so guard the calls to
>> vgic_v3_emulate_sysreg() as well as the function itself with
>> "#ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <julien@xen.org>

Cheers,

> 
> ~Michal

-- 
Julien Grall

[XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Ayan Kumar Halder]

Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
Affinity level 3 is not present in AArch32.
Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
Affinity level 3 is not present in Armv7 (ie arm32).
Thus, any access to affinity level 3 needs to be guarded within
"ifdef CONFIG_ARM_64".

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -

v1 - NA (as it is a new patch)

v2 - NA (as it is a new patch)

 xen/arch/arm/gic-v3.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 018fa0dfa0..64a76307dd 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -527,7 +527,10 @@ static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending)
 static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
 {
      uint64_t mpidr = cpu_logical_map(cpu);
-     return (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
+     return (
+#ifdef CONFIG_ARM_64
+             MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
+#endif
              MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
              MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8  |
              MPIDR_AFFINITY_LEVEL(mpidr, 0));
@@ -720,7 +723,10 @@ static int __init gicv3_populate_rdist(void)
      * Convert affinity to a 32bit value that can be matched to GICR_TYPER
      * bits [63:32]
      */
-    aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+    aff = (
+#ifdef CONFIG_ARM_64
+           MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+#endif
            MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
            MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
            MPIDR_AFFINITY_LEVEL(mpidr, 0));
@@ -972,7 +978,10 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const cpumask_t *cpumask)
          * Prepare affinity path of the cluster for which SGI is generated
          * along with SGI number
          */
-        val = (MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48  |
+        val = (
+#ifdef CONFIG_ARM_64
+               MPIDR_AFFINITY_LEVEL(cluster_id, 3) << 48  |
+#endif
                MPIDR_AFFINITY_LEVEL(cluster_id, 2) << 32  |
                sgi << 24                                  |
                MPIDR_AFFINITY_LEVEL(cluster_id, 1) << 16  |
-- 
2.17.1

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
> Affinity level 3 is not present in AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
> Affinity level 3 is not present in Armv7 (ie arm32).
> Thus, any access to affinity level 3 needs to be guarded within
> "ifdef CONFIG_ARM_64".
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

although, IMO the commit msg does not reflect the change (i.e. you do nothing
related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
Also, I'm not sure why you used VMPIDR and not MPIDR.

~Michal

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Julien Grall]

On 17/11/2022 13:39, Michal Orzel wrote:
> Hi Ayan,
> 
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>> Affinity level 3 is not present in AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>> Affinity level 3 is not present in Armv7 (ie arm32).
>> Thus, any access to affinity level 3 needs to be guarded within
>> "ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> although, IMO the commit msg does not reflect the change (i.e. you do nothing
> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
> Also, I'm not sure why you used VMPIDR and not MPIDR.

+1. Can one of you propose an improved commit message/title?

I would be happy to update the patch on commit if there are nothing else 
to update in this series.

Cheers,

-- 
Julien Grall

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Michal Orzel]

Hi Julien,

On 22/11/2022 21:31, Julien Grall wrote:
> 
> 
> On 17/11/2022 13:39, Michal Orzel wrote:
>> Hi Ayan,
>>
>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>> Affinity level 3 is not present in AArch32.
>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>> Thus, any access to affinity level 3 needs to be guarded within
>>> "ifdef CONFIG_ARM_64".
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> although, IMO the commit msg does not reflect the change (i.e. you do nothing
>> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
>> Also, I'm not sure why you used VMPIDR and not MPIDR.
> 
> +1. Can one of you propose an improved commit message/title?
Title: "Do not calculate affinity level 3 for AArch32"

I think the commit message can stay as it is.
> 
> I would be happy to update the patch on commit if there are nothing else
> to update in this series.
The series requires re-spinning anyway so no need to update the patch on commit.

> 
> Cheers,
> 
> --
> Julien Grall

~Michal

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Julien Grall]

On 23/11/2022 10:35, Michal Orzel wrote:
> Hi Julien,
> 
> On 22/11/2022 21:31, Julien Grall wrote:
>>
>>
>> On 17/11/2022 13:39, Michal Orzel wrote:
>>> Hi Ayan,
>>>
>>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>>> Affinity level 3 is not present in AArch32.
>>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>>> Thus, any access to affinity level 3 needs to be guarded within
>>>> "ifdef CONFIG_ARM_64".
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> although, IMO the commit msg does not reflect the change (i.e. you do nothing
>>> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
>>> Also, I'm not sure why you used VMPIDR and not MPIDR.
>>
>> +1. Can one of you propose an improved commit message/title?
> Title: "Do not calculate affinity level 3 for AArch32"

Sounds good to me.

> 
> I think the commit message can stay as it is.
>>
>> I would be happy to update the patch on commit if there are nothing else
>> to update in this series.
> The series requires re-spinning anyway so no need to update the patch on commit.

Fine with that.

Cheers,

-- 
Julien Grall

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Ayan Kumar Halder]

On 17/11/2022 13:39, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
>
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>> Affinity level 3 is not present in AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>> Affinity level 3 is not present in Armv7 (ie arm32).
>> Thus, any access to affinity level 3 needs to be guarded within
>> "ifdef CONFIG_ARM_64".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> although, IMO the commit msg does not reflect the change (i.e. you do nothing
> related to accessing MPIDR, but instead you are just not taking the Aff3 into account for AArch32).
> Also, I'm not sure why you used VMPIDR and not MPIDR.

Actually MPIDR in EL2 is known as VMPIDR. So I used this name.

ReferARM DDI 0487I.aID081822, D17-6354,D17.2.152, VMPIDR_EL2, 
Virtualization Multiprocessor ID Register

- Ayan

>
> ~Michal

Re: [XEN v3 02/12] xen/Arm: GICv3: Adapt access to VMPIDR register for AArch32

[Julien Grall]

On 27/11/2022 14:32, Ayan Kumar Halder wrote:
> 
> On 17/11/2022 13:39, Michal Orzel wrote:
>> Hi Ayan,
> Hi Michal,
>>
>> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>>> Refer ARM DDI 0487I.a ID081822, G8-9817, G8.2.169
>>> Affinity level 3 is not present in AArch32.
>>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106,
>>> Affinity level 3 is not present in Armv7 (ie arm32).
>>> Thus, any access to affinity level 3 needs to be guarded within
>>> "ifdef CONFIG_ARM_64".
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> although, IMO the commit msg does not reflect the change (i.e. you do 
>> nothing
>> related to accessing MPIDR, but instead you are just not taking the 
>> Aff3 into account for AArch32).
>> Also, I'm not sure why you used VMPIDR and not MPIDR.
> 
> Actually MPIDR in EL2 is known as VMPIDR. So I used this name.

Quoting the Arm Arm: The VMPIDR, holds the value of the Virtualization 
Multiprocessor ID. This is the value returned by Non-secure EL1 reads of 
MPIDR.

The code you are touching is looking a the pCPU information. Therefore, 
the correct name of the register is MPIDR.

Cheers,

-- 
Julien Grall

[XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32

[Ayan Kumar Halder]

In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
64bit registers in AArch32 mode. In such situations, the hypervisor may
need to read/modify the lower or upper 32 bits of the 64 bit register.

In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
registers.

While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
whether a 32-bit compiler would not allocate register for the upper 32-bit.
Therefore fold vreg_reg_* helper in the size specific one and use the
appropriate type based on the size requested.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from -

v1 - 1. Remove vreg_reg_extract(), vreg_reg_update(), vreg_reg_setbits() and
vreg_reg_clearbits(). Moved the implementation to  vreg_reg##sz##_*.
'mask' and 'val' is now using uint##sz##_t.

v2 - 1. Use 'unsigned int' for 'shift' variable.
2. Updated the commit message.

 xen/arch/arm/include/asm/vreg.h | 86 ++++++++-------------------------
 1 file changed, 19 insertions(+), 67 deletions(-)

diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h
index f26a70d024..d92450017b 100644
--- a/xen/arch/arm/include/asm/vreg.h
+++ b/xen/arch/arm/include/asm/vreg.h
@@ -89,106 +89,58 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr
  * The check on the size supported by the register has to be done by
  * the caller of vreg_regN_*.
  *
- * vreg_reg_* should never be called directly. Instead use the vreg_regN_*
- * according to size of the emulated register
- *
  * Note that the alignment fault will always be taken in the guest
  * (see B3.12.7 DDI0406.b).
  */
-static inline register_t vreg_reg_extract(unsigned long reg,
-                                          unsigned int offset,
-                                          enum dabt_size size)
-{
-    reg >>= 8 * offset;
-    reg &= VREG_REG_MASK(size);
-
-    return reg;
-}
-
-static inline void vreg_reg_update(unsigned long *reg, register_t val,
-                                   unsigned int offset,
-                                   enum dabt_size size)
-{
-    unsigned long mask = VREG_REG_MASK(size);
-    int shift = offset * 8;
-
-    *reg &= ~(mask << shift);
-    *reg |= ((unsigned long)val & mask) << shift;
-}
-
-static inline void vreg_reg_setbits(unsigned long *reg, register_t bits,
-                                    unsigned int offset,
-                                    enum dabt_size size)
-{
-    unsigned long mask = VREG_REG_MASK(size);
-    int shift = offset * 8;
-
-    *reg |= ((unsigned long)bits & mask) << shift;
-}
-
-static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits,
-                                      unsigned int offset,
-                                      enum dabt_size size)
-{
-    unsigned long mask = VREG_REG_MASK(size);
-    int shift = offset * 8;
-
-    *reg &= ~(((unsigned long)bits & mask) << shift);
-}
 
 /* N-bit register helpers */
 #define VREG_REG_HELPERS(sz, offmask)                                   \
 static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg,       \
                                                 const mmio_info_t *info)\
 {                                                                       \
-    return vreg_reg_extract(reg, info->gpa & (offmask),                 \
-                            info->dabt.size);                           \
+    unsigned int offset = info->gpa & (offmask);                        \
+                                                                        \
+    reg >>= 8 * offset;                                                 \
+    reg &= VREG_REG_MASK(info->dabt.size);                              \
+                                                                        \
+    return reg;                                                         \
 }                                                                       \
                                                                         \
 static inline void vreg_reg##sz##_update(uint##sz##_t *reg,             \
                                          register_t val,                \
                                          const mmio_info_t *info)       \
 {                                                                       \
-    unsigned long tmp = *reg;                                           \
+    unsigned int offset = info->gpa & (offmask);                        \
+    uint##sz##_t mask = VREG_REG_MASK(info->dabt.size);                 \
+    unsigned int shift = offset * 8;                                    \
                                                                         \
-    vreg_reg_update(&tmp, val, info->gpa & (offmask),                   \
-                    info->dabt.size);                                   \
-                                                                        \
-    *reg = tmp;                                                         \
+    *reg &= ~(mask << shift);                                           \
+    *reg |= ((uint##sz##_t)val & mask) << shift;                        \
 }                                                                       \
                                                                         \
 static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg,            \
                                           register_t bits,              \
                                           const mmio_info_t *info)      \
 {                                                                       \
-    unsigned long tmp = *reg;                                           \
-                                                                        \
-    vreg_reg_setbits(&tmp, bits, info->gpa & (offmask),                 \
-                     info->dabt.size);                                  \
+    unsigned int offset = info->gpa & (offmask);                        \
+    uint##sz##_t mask = VREG_REG_MASK(info->dabt.size);                 \
+    unsigned int shift = offset * 8;                                    \
                                                                         \
-    *reg = tmp;                                                         \
+    *reg |= ((uint##sz##_t)bits & mask) << shift;                       \
 }                                                                       \
                                                                         \
 static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg,          \
                                             register_t bits,            \
                                             const mmio_info_t *info)    \
 {                                                                       \
-    unsigned long tmp = *reg;                                           \
-                                                                        \
-    vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask),               \
-                       info->dabt.size);                                \
+    unsigned int offset = info->gpa & (offmask);                        \
+    uint##sz##_t mask = VREG_REG_MASK(info->dabt.size);                 \
+    unsigned int shift = offset * 8;                                    \
                                                                         \
-    *reg = tmp;                                                         \
+    *reg &= ~(((uint##sz##_t)bits & mask) << shift);                    \
 }
 
-/*
- * 64 bits registers are only supported on platform with 64-bit long.
- * This is also allow us to optimize the 32 bit case by using
- * unsigned long rather than uint64_t
- */
-#if BITS_PER_LONG == 64
 VREG_REG_HELPERS(64, 0x7);
-#endif
 VREG_REG_HELPERS(32, 0x3);
 
 #undef VREG_REG_HELPERS
-- 
2.17.1

Re: [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
> 64bit registers in AArch32 mode. In such situations, the hypervisor may
> need to read/modify the lower or upper 32 bits of the 64 bit register.
> 
> In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
> registers.
> 
> While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
> whether a 32-bit compiler would not allocate register for the upper 32-bit.
> Therefore fold vreg_reg_* helper in the size specific one and use the
> appropriate type based on the size requested.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v3 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on AArch32

[Julien Grall]

Hi Michal,

On 17/11/2022 13:11, Michal Orzel wrote:
> On 11/11/2022 15:17, Ayan Kumar Halder wrote:
>> In some situations (e.g. GICR_TYPER), the hypervior may need to emulate
>> 64bit registers in AArch32 mode. In such situations, the hypervisor may
>> need to read/modify the lower or upper 32 bits of the 64 bit register.
>>
>> In AArch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
>> registers.
>>
>> While we could replace 'unsigned long' by 'uint64_t', it is not entirely clear
>> whether a 32-bit compiler would not allocate register for the upper 32-bit.
>> Therefore fold vreg_reg_* helper in the size specific one and use the
>> appropriate type based on the size requested.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

[XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32

[Ayan Kumar Halder]

Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
Aff3 does not exist on AArch32.
Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
Aff3 does not exist on Armv7 (ie arm32).

Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
'uint64_t vmpidr' to perform the shifts.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from :-
v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use 
MPIDR_AFFINITY_LEVEL macros to extract the affinity value.

v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
"#ifdef CONFIG_ARM_64".
2. Updated commit message.

 xen/arch/arm/vgic-v3.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 3f4509dcd3..a7a935ff57 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
     case VREG64(GICR_TYPER):
     {
         uint64_t typer, aff;
+        uint64_t vmpidr = v->arch.vmpidr;
 
         if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
-        aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
-               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
-               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
-               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
+        aff = (
+#ifdef CONFIG_ARM_64
+               MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
+#endif
+               MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
+               MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
+               MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
         typer = aff;
         /* We use the VCPU ID as the redistributor ID in bits[23:8] */
         typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
-- 
2.17.1

Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> 
> 
> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
> Aff3 does not exist on AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
> Aff3 does not exist on Armv7 (ie arm32).
> 
> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
s/have been contained within/has been protected with/

> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
s/copied/assigned/

> 'uint64_t vmpidr' to perform the shifts.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32

[Julien Grall]

Hi Ayan,

On 11/11/2022 14:17, Ayan Kumar Halder wrote:
> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
> Aff3 does not exist on AArch32.
> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
> Aff3 does not exist on Armv7 (ie arm32).
> 
> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have copied it to
> 'uint64_t vmpidr' to perform the shifts.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from :-
> v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
> MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
> 
> v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
> "#ifdef CONFIG_ARM_64".
> 2. Updated commit message.
> 
>   xen/arch/arm/vgic-v3.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 3f4509dcd3..a7a935ff57 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>       case VREG64(GICR_TYPER):
>       {
>           uint64_t typer, aff;
> +        uint64_t vmpidr = v->arch.vmpidr;

The type-widening here deserve an in-code comment. Otherwise, it would 
be easier for someone to decide to open-code v->arch.vmpidr again.

>   
>           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> -        aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
> +        aff = (
> +#ifdef CONFIG_ARM_64
> +               MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
> +#endif
> +               MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
> +               MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
> +               MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
>           typer = aff;
>           /* We use the VCPU ID as the redistributor ID in bits[23:8] */
>           typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;

Cheers,

-- 
Julien Grall

Re: [XEN v3 04/12] xen/Arm: vGICv3: Adapt emulation of GICR_TYPER for AArch32

[Ayan Kumar Halder]

On 22/11/2022 20:37, Julien Grall wrote:
> Hi Ayan,

Hi Julien,

I need a clarification.

>
> On 11/11/2022 14:17, Ayan Kumar Halder wrote:
>> Refer ARM DDI 0487I.a ID081822, G8-9650, G8.2.113
>> Aff3 does not exist on AArch32.
>> Also, refer ARM DDI 0406C.d ID040418, B4-1644, B4.1.106
>> Aff3 does not exist on Armv7 (ie arm32).
>>
>> Thus, access to aff3 have been contained within "#ifdef CONFIG_ARM_64".
>> Also, v->arch.vmpidr is a 32 bit register on AArch32. So, we have 
>> copied it to
>> 'uint64_t vmpidr' to perform the shifts.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> Changes from :-
>> v1 - Assigned v->arch.vmpidr to "uint64_t vmpdir". Then, we can use
>> MPIDR_AFFINITY_LEVEL macros to extract the affinity value.
>>
>> v2 - 1. "MPIDR_AFFINITY_LEVEL(vmpidr, 3)" is contained within
>> "#ifdef CONFIG_ARM_64".
>> 2. Updated commit message.
>>
>>   xen/arch/arm/vgic-v3.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 3f4509dcd3..a7a935ff57 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -191,12 +191,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct 
>> vcpu *v, mmio_info_t *info,
>>       case VREG64(GICR_TYPER):
>>       {
>>           uint64_t typer, aff;
>> +        uint64_t vmpidr = v->arch.vmpidr;
>
> The type-widening here deserve an in-code comment. Otherwise, it would 
> be easier for someone to decide to open-code v->arch.vmpidr again.

Does this comment look fine ?

         /*
          * This is to enable shifts greater than 32 bits which would have
          * otherwise caused overflow (as v->arch.vmpidr is 32 bit on 
AArch32).
          */
         uint64_t vmpidr = v->arch.vmpidr;

- Ayan

>
>>             if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> -        aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
>> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
>> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
>> -               MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
>> +        aff = (
>> +#ifdef CONFIG_ARM_64
>> +               MPIDR_AFFINITY_LEVEL(vmpidr, 3) << 56 |
>> +#endif
>> +               MPIDR_AFFINITY_LEVEL(vmpidr, 2) << 48 |
>> +               MPIDR_AFFINITY_LEVEL(vmpidr, 1) << 40 |
>> +               MPIDR_AFFINITY_LEVEL(vmpidr, 0) << 32);
>>           typer = aff;
>>           /* We use the VCPU ID as the redistributor ID in bits[23:8] */
>>           typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
>
> Cheers,
>

[XEN v3 05/12] xen/Arm: GICv3: Fix GICR_{PENDBASER, PROPBASER} emulation on 32-bit host

[Ayan Kumar Halder]

'unsigned long long' is defined as 64 bit across both AArch32 and AArch64.
So, use 'ULL' for 64 bit word instead of UL which is 32 bits for AArch32.
GICR_PENDBASER and GICR_PROPBASER both are 64 bit registers.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---

Changes from -
v1 - 1. Extract the bug fix for incorrect bit clearing (GICR_PENDBASER_PTZ)
into a separate patch fix.
https://patchwork.kernel.org/project/xen-devel/patch/20221027185555.46125-1-ayankuma@amd.com/

v2 - No changes.

 xen/arch/arm/include/asm/gic_v3_defs.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 728e28d5e5..48a1bc401e 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -134,15 +134,15 @@
 
 #define GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT         56
 #define GICR_PROPBASER_OUTER_CACHEABILITY_MASK               \
-        (7UL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
+        (7ULL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
 #define GICR_PROPBASER_SHAREABILITY_SHIFT               10
 #define GICR_PROPBASER_SHAREABILITY_MASK                     \
-        (3UL << GICR_PROPBASER_SHAREABILITY_SHIFT)
+        (3ULL << GICR_PROPBASER_SHAREABILITY_SHIFT)
 #define GICR_PROPBASER_INNER_CACHEABILITY_SHIFT         7
 #define GICR_PROPBASER_INNER_CACHEABILITY_MASK               \
-        (7UL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
+        (7ULL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
 #define GICR_PROPBASER_RES0_MASK                             \
-        (GENMASK(63, 59) | GENMASK(55, 52) | GENMASK(6, 5))
+        (GENMASK_ULL(63, 59) | GENMASK_ULL(55, 52) | GENMASK_ULL(6, 5))
 
 #define GICR_PENDBASER_SHAREABILITY_SHIFT               10
 #define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT         7
@@ -152,11 +152,11 @@
 #define GICR_PENDBASER_INNER_CACHEABILITY_MASK               \
 	(7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT)
 #define GICR_PENDBASER_OUTER_CACHEABILITY_MASK               \
-        (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
-#define GICR_PENDBASER_PTZ                              BIT(62, UL)
+        (7ULL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
+#define GICR_PENDBASER_PTZ                              BIT(62, ULL)
 #define GICR_PENDBASER_RES0_MASK                             \
-        (BIT(63, UL) | GENMASK(61, 59) | GENMASK(55, 52) |  \
-         GENMASK(15, 12) | GENMASK(6, 0))
+        (BIT(63, ULL) | GENMASK_ULL(61, 59) | GENMASK_ULL(55, 52) |  \
+         GENMASK_ULL(15, 12) | GENMASK_ULL(6, 0))
 
 #define DEFAULT_PMR_VALUE            0xff
 
-- 
2.17.1

[XEN v3 06/12] xen/Arm: vGICv3: Fix emulation of ICC_SGI1R on AArch32

[Ayan Kumar Halder]

Refer Arm IHI 0069H ID020922, 12.5.23, ICC_SGI1R is a 64 bit register on
AArch32 systems. Thus, the function needs to change to reflect this.
The reason being 'register_t' is defined as 'u32' on AArch32.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---

Changes from :-
v1 - 1. Updated the commit message.

v2 - 1. No changes.

 xen/arch/arm/vgic-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index a7a935ff57..93c8a0ae79 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1479,7 +1479,7 @@ write_reserved:
     return 1;
 }
 
-static bool vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
+static bool vgic_v3_to_sgi(struct vcpu *v, uint64_t sgir)
 {
     int virq;
     int irqmode;
-- 
2.17.1

[XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32

[Ayan Kumar Halder]

Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers

AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally
mapped to AArch32 System register ICH_LR<n>[31:0].
AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally
mapped to AArch32 System register ICH_LRC<n>[31:0].

Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for AArch32.
For AArch32, the link register is stored as :-
(((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2

Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
AArch64.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from :-
v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
2. Use GENMASK(31, 0) to represent 0xFFFFFFFF
3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
4. Multi-line macro definitions should be enclosed within ({ }).

v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
2. Defined the register alias.
3. Style issues.

 xen/arch/arm/gic-v3.c                    | 132 +++++++++++------------
 xen/arch/arm/include/asm/arm32/sysregs.h |  19 ++++
 xen/arch/arm/include/asm/arm64/sysregs.h |   4 +
 xen/arch/arm/include/asm/cpregs.h        |  75 +++++++++++++
 xen/arch/arm/include/asm/gic_v3_defs.h   |   6 +-
 5 files changed, 167 insertions(+), 69 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 64a76307dd..4722bb4daf 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
     switch ( gicv3_info.nr_lrs )
     {
     case 16:
-        v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
+        v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
     case 15:
-        v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
+        v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
     case 14:
-        v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
+        v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
     case 13:
-        v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
+        v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
     case 12:
-        v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
+        v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
     case 11:
-        v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
+        v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
     case 10:
-        v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
+        v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
     case 9:
-        v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
+        v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
     case 8:
-        v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
+        v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
     case 7:
-        v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
+        v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
     case 6:
-        v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
+        v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
     case 5:
-        v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
+        v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
     case 4:
-        v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
+        v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
     case 3:
-        v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
+        v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
     case 2:
-        v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
+        v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
     case 1:
-         v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
+         v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
          break;
     default:
          BUG();
@@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
     switch ( gicv3_info.nr_lrs )
     {
     case 16:
-        WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
     case 15:
-        WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
     case 14:
-        WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
     case 13:
-        WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
     case 12:
-        WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
     case 11:
-        WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
     case 10:
-        WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
     case 9:
-        WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
     case 8:
-        WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
     case 7:
-        WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
     case 6:
-        WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
     case 5:
-        WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
     case 4:
-        WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
     case 3:
-        WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
     case 2:
-        WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
     case 1:
-        WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
+        WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
         break;
     default:
          BUG();
@@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
 {
     switch ( lr )
     {
-    case 0: return READ_SYSREG(ICH_LR0_EL2);
-    case 1: return READ_SYSREG(ICH_LR1_EL2);
-    case 2: return READ_SYSREG(ICH_LR2_EL2);
-    case 3: return READ_SYSREG(ICH_LR3_EL2);
-    case 4: return READ_SYSREG(ICH_LR4_EL2);
-    case 5: return READ_SYSREG(ICH_LR5_EL2);
-    case 6: return READ_SYSREG(ICH_LR6_EL2);
-    case 7: return READ_SYSREG(ICH_LR7_EL2);
-    case 8: return READ_SYSREG(ICH_LR8_EL2);
-    case 9: return READ_SYSREG(ICH_LR9_EL2);
-    case 10: return READ_SYSREG(ICH_LR10_EL2);
-    case 11: return READ_SYSREG(ICH_LR11_EL2);
-    case 12: return READ_SYSREG(ICH_LR12_EL2);
-    case 13: return READ_SYSREG(ICH_LR13_EL2);
-    case 14: return READ_SYSREG(ICH_LR14_EL2);
-    case 15: return READ_SYSREG(ICH_LR15_EL2);
+    case 0: return READ_SYSREG_LR(0);
+    case 1: return READ_SYSREG_LR(1);
+    case 2: return READ_SYSREG_LR(2);
+    case 3: return READ_SYSREG_LR(3);
+    case 4: return READ_SYSREG_LR(4);
+    case 5: return READ_SYSREG_LR(5);
+    case 6: return READ_SYSREG_LR(6);
+    case 7: return READ_SYSREG_LR(7);
+    case 8: return READ_SYSREG_LR(8);
+    case 9: return READ_SYSREG_LR(9);
+    case 10: return READ_SYSREG_LR(10);
+    case 11: return READ_SYSREG_LR(11);
+    case 12: return READ_SYSREG_LR(12);
+    case 13: return READ_SYSREG_LR(13);
+    case 14: return READ_SYSREG_LR(14);
+    case 15: return READ_SYSREG_LR(15);
     default:
         BUG();
     }
@@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
     switch ( lr )
     {
     case 0:
-        WRITE_SYSREG(val, ICH_LR0_EL2);
+        WRITE_SYSREG_LR(val, 0);
         break;
     case 1:
-        WRITE_SYSREG(val, ICH_LR1_EL2);
+        WRITE_SYSREG_LR(val, 1);
         break;
     case 2:
-        WRITE_SYSREG(val, ICH_LR2_EL2);
+        WRITE_SYSREG_LR(val, 2);
         break;
     case 3:
-        WRITE_SYSREG(val, ICH_LR3_EL2);
+        WRITE_SYSREG_LR(val, 3);
         break;
     case 4:
-        WRITE_SYSREG(val, ICH_LR4_EL2);
+        WRITE_SYSREG_LR(val, 4);
         break;
     case 5:
-        WRITE_SYSREG(val, ICH_LR5_EL2);
+        WRITE_SYSREG_LR(val, 5);
         break;
     case 6:
-        WRITE_SYSREG(val, ICH_LR6_EL2);
+        WRITE_SYSREG_LR(val, 6);
         break;
     case 7:
-        WRITE_SYSREG(val, ICH_LR7_EL2);
+        WRITE_SYSREG_LR(val, 7);
         break;
     case 8:
-        WRITE_SYSREG(val, ICH_LR8_EL2);
+        WRITE_SYSREG_LR(val, 8);
         break;
     case 9:
-        WRITE_SYSREG(val, ICH_LR9_EL2);
+        WRITE_SYSREG_LR(val, 9);
         break;
     case 10:
-        WRITE_SYSREG(val, ICH_LR10_EL2);
+        WRITE_SYSREG_LR(val, 10);
         break;
     case 11:
-        WRITE_SYSREG(val, ICH_LR11_EL2);
+        WRITE_SYSREG_LR(val, 11);
         break;
     case 12:
-        WRITE_SYSREG(val, ICH_LR12_EL2);
+        WRITE_SYSREG_LR(val, 12);
         break;
     case 13:
-        WRITE_SYSREG(val, ICH_LR13_EL2);
+        WRITE_SYSREG_LR(val, 13);
         break;
     case 14:
-        WRITE_SYSREG(val, ICH_LR14_EL2);
+        WRITE_SYSREG_LR(val, 14);
         break;
     case 15:
-        WRITE_SYSREG(val, ICH_LR15_EL2);
+        WRITE_SYSREG_LR(val, 15);
         break;
     default:
         return;
@@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
     if ( v == current )
     {
         for ( i = 0; i < gicv3_info.nr_lrs; i++ )
-            printk("   HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
+            printk("   HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
     }
     else
     {
         for ( i = 0; i < gicv3_info.nr_lrs; i++ )
-            printk("   VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
+            printk("   VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
     }
 }
 
diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
index 6841d5de43..31ad7eaefb 100644
--- a/xen/arch/arm/include/asm/arm32/sysregs.h
+++ b/xen/arch/arm/include/asm/arm32/sysregs.h
@@ -62,6 +62,25 @@
 #define READ_SYSREG(R...)       READ_SYSREG32(R)
 #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
 
+/* Wrappers for accessing interrupt controller list registers. */
+#define ICH_LR_REG(index)       ICH_LR ## index ## _EL2
+#define ICH_LRC_REG(index)      ICH_LRC ## index ## _EL2
+
+#define READ_SYSREG_LR(index) ({                            \
+    uint64_t _val;                                          \
+    uint32_t _lrc = READ_CP32(ICH_LRC_REG(index));          \
+    uint32_t _lr = READ_CP32(ICH_LR_REG(index));            \
+                                                            \
+    _val = ((uint64_t) _lrc << 32) | _lr;                   \
+    _val;                                                   \
+})
+
+#define WRITE_SYSREG_LR(V, index) ({                        \
+    uint64_t _val = (V);                                    \
+    WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(index));   \
+    WRITE_CP32(_val >> 32, ICH_LRC_REG(index));             \
+})
+
 /* MVFR2 is not defined on ARMv7 */
 #define MVFR2_MAYBE_UNDEFINED
 
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 54670084c3..b72a6ea3b0 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -472,6 +472,10 @@
 #define READ_SYSREG(name)     READ_SYSREG64(name)
 #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
 
+/* Wrappers for accessing interrupt controller list registers. */
+#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
+#define WRITE_SYSREG_LR(V, index)  WRITE_SYSREG(V, ICH_LR_REG(index))
+#define READ_SYSREG_LR(index)      READ_SYSREG(ICH_LR_REG(index))
 #endif /* _ASM_ARM_ARM64_SYSREGS_H */
 
 /*
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 6daf2b1a30..242dabaea8 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -259,6 +259,49 @@
 #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
 #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base Address 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)
+#define __LR8(x)                  ___CP32(p15, 4, c12, c13, x)
+
+#define ICH_LR0                   __LR0(0)
+#define ICH_LR1                   __LR0(1)
+#define ICH_LR2                   __LR0(2)
+#define ICH_LR3                   __LR0(3)
+#define ICH_LR4                   __LR0(4)
+#define ICH_LR5                   __LR0(5)
+#define ICH_LR6                   __LR0(6)
+#define ICH_LR7                   __LR0(7)
+#define ICH_LR8                   __LR8(0)
+#define ICH_LR9                   __LR8(1)
+#define ICH_LR10                  __LR8(2)
+#define ICH_LR11                  __LR8(3)
+#define ICH_LR12                  __LR8(4)
+#define ICH_LR13                  __LR8(5)
+#define ICH_LR14                  __LR8(6)
+#define ICH_LR15                  __LR8(7)
+
+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LRC0(x)                 ___CP32(p15, 4, c12, c14, x)
+#define __LRC8(x)                 ___CP32(p15, 4, c12, c15, x)
+
+#define ICH_LRC0                  __LRC0(0)
+#define ICH_LRC1                  __LRC0(1)
+#define ICH_LRC2                  __LRC0(2)
+#define ICH_LRC3                  __LRC0(3)
+#define ICH_LRC4                  __LRC0(4)
+#define ICH_LRC5                  __LRC0(5)
+#define ICH_LRC6                  __LRC0(6)
+#define ICH_LRC7                  __LRC0(7)
+#define ICH_LRC8                  __LRC8(0)
+#define ICH_LRC9                  __LRC8(1)
+#define ICH_LRC10                 __LRC8(2)
+#define ICH_LRC11                 __LRC8(3)
+#define ICH_LRC12                 __LRC8(4)
+#define ICH_LRC13                 __LRC8(5)
+#define ICH_LRC14                 __LRC8(6)
+#define ICH_LRC15                 __LRC8(7)
+
 /* CP15 CR13:  */
 #define FCSEIDR         p15,0,c13,c0,0  /* FCSE Process ID Register */
 #define CONTEXTIDR      p15,0,c13,c0,1  /* Context ID Register */
@@ -317,6 +360,38 @@
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define HSTR_EL2                HSTR
+#define ICH_LR0_EL2             ICH_LR0
+#define ICH_LR1_EL2             ICH_LR1
+#define ICH_LR2_EL2             ICH_LR2
+#define ICH_LR3_EL2             ICH_LR3
+#define ICH_LR4_EL2             ICH_LR4
+#define ICH_LR5_EL2             ICH_LR5
+#define ICH_LR6_EL2             ICH_LR6
+#define ICH_LR7_EL2             ICH_LR7
+#define ICH_LR8_EL2             ICH_LR8
+#define ICH_LR9_EL2             ICH_LR9
+#define ICH_LR10_EL2            ICH_LR10
+#define ICH_LR11_EL2            ICH_LR11
+#define ICH_LR12_EL2            ICH_LR12
+#define ICH_LR13_EL2            ICH_LR13
+#define ICH_LR14_EL2            ICH_LR14
+#define ICH_LR15_EL2            ICH_LR15
+#define ICH_LRC0_EL2            ICH_LRC0
+#define ICH_LRC1_EL2            ICH_LRC1
+#define ICH_LRC2_EL2            ICH_LRC2
+#define ICH_LRC3_EL2            ICH_LRC3
+#define ICH_LRC4_EL2            ICH_LRC4
+#define ICH_LRC5_EL2            ICH_LRC5
+#define ICH_LRC6_EL2            ICH_LRC6
+#define ICH_LRC7_EL2            ICH_LRC7
+#define ICH_LRC8_EL2            ICH_LRC8
+#define ICH_LRC9_EL2            ICH_LRC9
+#define ICH_LRC10_EL2           ICH_LRC10
+#define ICH_LRC11_EL2           ICH_LRC11
+#define ICH_LRC12_EL2           ICH_LRC12
+#define ICH_LRC13_EL2           ICH_LRC13
+#define ICH_LRC14_EL2           ICH_LRC14
+#define ICH_LRC15_EL2           ICH_LRC15
 #define ID_AFR0_EL1             ID_AFR0
 #define ID_DFR0_EL1             ID_DFR0
 #define ID_DFR1_EL1             ID_DFR1
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 48a1bc401e..743ebb20fd 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -185,9 +185,9 @@
 #define ICH_LR_HW_SHIFT              61
 #define ICH_LR_GRP_MASK              0x1
 #define ICH_LR_GRP_SHIFT             60
-#define ICH_LR_MAINTENANCE_IRQ       (1UL<<41)
-#define ICH_LR_GRP1                  (1UL<<60)
-#define ICH_LR_HW                    (1UL<<61)
+#define ICH_LR_MAINTENANCE_IRQ       (1ULL << 41)
+#define ICH_LR_GRP1                  (1ULL << 60)
+#define ICH_LR_HW                    (1ULL << 61)
 
 #define ICH_VTR_NRLRGS               0x3f
 #define ICH_VTR_PRIBITS_MASK         0x7
-- 
2.17.1

Re: [XEN v3 07/12] xen/Arm: GICv3: Define ICH_LR<n>_EL2 on AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Refer "Arm IHI 0069H ID020922", 12.4.6, Interrupt Controller List Registers
> 
> AArch64 System register ICH_LR<n>_EL2 bits [31:0] are architecturally
> mapped to AArch32 System register ICH_LR<n>[31:0].
> AArch64 System register ICH_LR<n>_EL2 bits [63:32] are architecturally
> mapped to AArch32 System register ICH_LRC<n>[31:0].
> 
> Defined ICH_LR<0...15>_EL2 and ICH_LRC<0...15>_EL2 for AArch32.
> For AArch32, the link register is stored as :-
> (((uint64_t) ICH_LRC<0...15>_EL2) << 32) | ICH_LR<0...15>_EL2
> 
> Also, ICR_LR macros need to be modified as ULL is 64 bits for AArch32 and
> AArch64.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from :-
> v1 - 1. Moved the coproc register definitions to asm/cpregs.h.
> 2. Use GENMASK(31, 0) to represent 0xFFFFFFFF
> 3. Use READ_CP32()/WRITE_CP32() instead of READ_SYSREG()/WRITE_SYSREG().
> 4. Multi-line macro definitions should be enclosed within ({ }).
> 
> v2 - 1. Use WRITE_SYSREG_LR(V, R) to make it consistent with before.
> 2. Defined the register alias.
> 3. Style issues.
> 
>  xen/arch/arm/gic-v3.c                    | 132 +++++++++++------------
>  xen/arch/arm/include/asm/arm32/sysregs.h |  19 ++++
>  xen/arch/arm/include/asm/arm64/sysregs.h |   4 +
>  xen/arch/arm/include/asm/cpregs.h        |  75 +++++++++++++
>  xen/arch/arm/include/asm/gic_v3_defs.h   |   6 +-
>  5 files changed, 167 insertions(+), 69 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 64a76307dd..4722bb4daf 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -73,37 +73,37 @@ static inline void gicv3_save_lrs(struct vcpu *v)
>      switch ( gicv3_info.nr_lrs )
>      {
>      case 16:
> -        v->arch.gic.v3.lr[15] = READ_SYSREG(ICH_LR15_EL2);
> +        v->arch.gic.v3.lr[15] = READ_SYSREG_LR(15);
>      case 15:
> -        v->arch.gic.v3.lr[14] = READ_SYSREG(ICH_LR14_EL2);
> +        v->arch.gic.v3.lr[14] = READ_SYSREG_LR(14);
>      case 14:
> -        v->arch.gic.v3.lr[13] = READ_SYSREG(ICH_LR13_EL2);
> +        v->arch.gic.v3.lr[13] = READ_SYSREG_LR(13);
>      case 13:
> -        v->arch.gic.v3.lr[12] = READ_SYSREG(ICH_LR12_EL2);
> +        v->arch.gic.v3.lr[12] = READ_SYSREG_LR(12);
>      case 12:
> -        v->arch.gic.v3.lr[11] = READ_SYSREG(ICH_LR11_EL2);
> +        v->arch.gic.v3.lr[11] = READ_SYSREG_LR(11);
>      case 11:
> -        v->arch.gic.v3.lr[10] = READ_SYSREG(ICH_LR10_EL2);
> +        v->arch.gic.v3.lr[10] = READ_SYSREG_LR(10);
>      case 10:
> -        v->arch.gic.v3.lr[9] = READ_SYSREG(ICH_LR9_EL2);
> +        v->arch.gic.v3.lr[9] = READ_SYSREG_LR(9);
>      case 9:
> -        v->arch.gic.v3.lr[8] = READ_SYSREG(ICH_LR8_EL2);
> +        v->arch.gic.v3.lr[8] = READ_SYSREG_LR(8);
>      case 8:
> -        v->arch.gic.v3.lr[7] = READ_SYSREG(ICH_LR7_EL2);
> +        v->arch.gic.v3.lr[7] = READ_SYSREG_LR(7);
>      case 7:
> -        v->arch.gic.v3.lr[6] = READ_SYSREG(ICH_LR6_EL2);
> +        v->arch.gic.v3.lr[6] = READ_SYSREG_LR(6);
>      case 6:
> -        v->arch.gic.v3.lr[5] = READ_SYSREG(ICH_LR5_EL2);
> +        v->arch.gic.v3.lr[5] = READ_SYSREG_LR(5);
>      case 5:
> -        v->arch.gic.v3.lr[4] = READ_SYSREG(ICH_LR4_EL2);
> +        v->arch.gic.v3.lr[4] = READ_SYSREG_LR(4);
>      case 4:
> -        v->arch.gic.v3.lr[3] = READ_SYSREG(ICH_LR3_EL2);
> +        v->arch.gic.v3.lr[3] = READ_SYSREG_LR(3);
>      case 3:
> -        v->arch.gic.v3.lr[2] = READ_SYSREG(ICH_LR2_EL2);
> +        v->arch.gic.v3.lr[2] = READ_SYSREG_LR(2);
>      case 2:
> -        v->arch.gic.v3.lr[1] = READ_SYSREG(ICH_LR1_EL2);
> +        v->arch.gic.v3.lr[1] = READ_SYSREG_LR(1);
>      case 1:
> -         v->arch.gic.v3.lr[0] = READ_SYSREG(ICH_LR0_EL2);
> +         v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
>           break;
>      default:
>           BUG();
> @@ -120,37 +120,37 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
>      switch ( gicv3_info.nr_lrs )
>      {
>      case 16:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[15], ICH_LR15_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
>      case 15:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[14], ICH_LR14_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[14], 14);
>      case 14:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[13], ICH_LR13_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[13], 13);
>      case 13:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[12], ICH_LR12_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[12], 12);
>      case 12:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[11], ICH_LR11_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[11], 11);
>      case 11:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[10], ICH_LR10_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[10], 10);
>      case 10:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[9], ICH_LR9_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[9], 9);
>      case 9:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[8], ICH_LR8_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[8], 8);
>      case 8:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[7], ICH_LR7_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[7], 7);
>      case 7:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[6], ICH_LR6_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[6], 6);
>      case 6:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[5], ICH_LR5_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[5], 5);
>      case 5:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[4], ICH_LR4_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[4], 4);
>      case 4:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[3], ICH_LR3_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[3], 3);
>      case 3:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[2], ICH_LR2_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[2], 2);
>      case 2:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[1], ICH_LR1_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[1], 1);
>      case 1:
> -        WRITE_SYSREG(v->arch.gic.v3.lr[0], ICH_LR0_EL2);
> +        WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
>          break;
>      default:
>           BUG();
> @@ -161,22 +161,22 @@ static uint64_t gicv3_ich_read_lr(int lr)
>  {
>      switch ( lr )
>      {
> -    case 0: return READ_SYSREG(ICH_LR0_EL2);
> -    case 1: return READ_SYSREG(ICH_LR1_EL2);
> -    case 2: return READ_SYSREG(ICH_LR2_EL2);
> -    case 3: return READ_SYSREG(ICH_LR3_EL2);
> -    case 4: return READ_SYSREG(ICH_LR4_EL2);
> -    case 5: return READ_SYSREG(ICH_LR5_EL2);
> -    case 6: return READ_SYSREG(ICH_LR6_EL2);
> -    case 7: return READ_SYSREG(ICH_LR7_EL2);
> -    case 8: return READ_SYSREG(ICH_LR8_EL2);
> -    case 9: return READ_SYSREG(ICH_LR9_EL2);
> -    case 10: return READ_SYSREG(ICH_LR10_EL2);
> -    case 11: return READ_SYSREG(ICH_LR11_EL2);
> -    case 12: return READ_SYSREG(ICH_LR12_EL2);
> -    case 13: return READ_SYSREG(ICH_LR13_EL2);
> -    case 14: return READ_SYSREG(ICH_LR14_EL2);
> -    case 15: return READ_SYSREG(ICH_LR15_EL2);
> +    case 0: return READ_SYSREG_LR(0);
> +    case 1: return READ_SYSREG_LR(1);
> +    case 2: return READ_SYSREG_LR(2);
> +    case 3: return READ_SYSREG_LR(3);
> +    case 4: return READ_SYSREG_LR(4);
> +    case 5: return READ_SYSREG_LR(5);
> +    case 6: return READ_SYSREG_LR(6);
> +    case 7: return READ_SYSREG_LR(7);
> +    case 8: return READ_SYSREG_LR(8);
> +    case 9: return READ_SYSREG_LR(9);
> +    case 10: return READ_SYSREG_LR(10);
> +    case 11: return READ_SYSREG_LR(11);
> +    case 12: return READ_SYSREG_LR(12);
> +    case 13: return READ_SYSREG_LR(13);
> +    case 14: return READ_SYSREG_LR(14);
> +    case 15: return READ_SYSREG_LR(15);
>      default:
>          BUG();
>      }
> @@ -187,52 +187,52 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>      switch ( lr )
>      {
>      case 0:
> -        WRITE_SYSREG(val, ICH_LR0_EL2);
> +        WRITE_SYSREG_LR(val, 0);
>          break;
>      case 1:
> -        WRITE_SYSREG(val, ICH_LR1_EL2);
> +        WRITE_SYSREG_LR(val, 1);
>          break;
>      case 2:
> -        WRITE_SYSREG(val, ICH_LR2_EL2);
> +        WRITE_SYSREG_LR(val, 2);
>          break;
>      case 3:
> -        WRITE_SYSREG(val, ICH_LR3_EL2);
> +        WRITE_SYSREG_LR(val, 3);
>          break;
>      case 4:
> -        WRITE_SYSREG(val, ICH_LR4_EL2);
> +        WRITE_SYSREG_LR(val, 4);
>          break;
>      case 5:
> -        WRITE_SYSREG(val, ICH_LR5_EL2);
> +        WRITE_SYSREG_LR(val, 5);
>          break;
>      case 6:
> -        WRITE_SYSREG(val, ICH_LR6_EL2);
> +        WRITE_SYSREG_LR(val, 6);
>          break;
>      case 7:
> -        WRITE_SYSREG(val, ICH_LR7_EL2);
> +        WRITE_SYSREG_LR(val, 7);
>          break;
>      case 8:
> -        WRITE_SYSREG(val, ICH_LR8_EL2);
> +        WRITE_SYSREG_LR(val, 8);
>          break;
>      case 9:
> -        WRITE_SYSREG(val, ICH_LR9_EL2);
> +        WRITE_SYSREG_LR(val, 9);
>          break;
>      case 10:
> -        WRITE_SYSREG(val, ICH_LR10_EL2);
> +        WRITE_SYSREG_LR(val, 10);
>          break;
>      case 11:
> -        WRITE_SYSREG(val, ICH_LR11_EL2);
> +        WRITE_SYSREG_LR(val, 11);
>          break;
>      case 12:
> -        WRITE_SYSREG(val, ICH_LR12_EL2);
> +        WRITE_SYSREG_LR(val, 12);
>          break;
>      case 13:
> -        WRITE_SYSREG(val, ICH_LR13_EL2);
> +        WRITE_SYSREG_LR(val, 13);
>          break;
>      case 14:
> -        WRITE_SYSREG(val, ICH_LR14_EL2);
> +        WRITE_SYSREG_LR(val, 14);
>          break;
>      case 15:
> -        WRITE_SYSREG(val, ICH_LR15_EL2);
> +        WRITE_SYSREG_LR(val, 15);
>          break;
>      default:
>          return;
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
>      if ( v == current )
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   HW_LR[%d]=%lx\n", i, gicv3_ich_read_lr(i));
> +            printk("   HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
>      }
>      else
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   VCPU_LR[%d]=%lx\n", i, v->arch.gic.v3.lr[i]);
> +            printk("   VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
>      }
>  }
>  
> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h b/xen/arch/arm/include/asm/arm32/sysregs.h
> index 6841d5de43..31ad7eaefb 100644
> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
> @@ -62,6 +62,25 @@
>  #define READ_SYSREG(R...)       READ_SYSREG32(R)
>  #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
>  
> +/* Wrappers for accessing interrupt controller list registers. */
> +#define ICH_LR_REG(index)       ICH_LR ## index ## _EL2
> +#define ICH_LRC_REG(index)      ICH_LRC ## index ## _EL2
> +
> +#define READ_SYSREG_LR(index) ({                            \
> +    uint64_t _val;                                          \
> +    uint32_t _lrc = READ_CP32(ICH_LRC_REG(index));          \
> +    uint32_t _lr = READ_CP32(ICH_LR_REG(index));            \
> +                                                            \
> +    _val = ((uint64_t) _lrc << 32) | _lr;                   \
> +    _val;                                                   \
> +})
> +
> +#define WRITE_SYSREG_LR(V, index) ({                        \
Didn't we agree on using lower case version?
This upper case V looks a bit odd here...

> +    uint64_t _val = (V);                                    \
> +    WRITE_CP32(_val & GENMASK(31, 0), ICH_LR_REG(index));   \
> +    WRITE_CP32(_val >> 32, ICH_LRC_REG(index));             \
> +})
> +
>  /* MVFR2 is not defined on ARMv7 */
>  #define MVFR2_MAYBE_UNDEFINED
>  
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index 54670084c3..b72a6ea3b0 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -472,6 +472,10 @@
>  #define READ_SYSREG(name)     READ_SYSREG64(name)
>  #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
>  
> +/* Wrappers for accessing interrupt controller list registers. */
> +#define ICH_LR_REG(index)          ICH_LR ## index ## _EL2
> +#define WRITE_SYSREG_LR(V, index)  WRITE_SYSREG(V, ICH_LR_REG(index))
and here.

> +#define READ_SYSREG_LR(index)      READ_SYSREG(ICH_LR_REG(index))
Before you added a new code block there was an empty line here.
Could you please add it?
>  #endif /* _ASM_ARM_ARM64_SYSREGS_H */
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 6daf2b1a30..242dabaea8 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -259,6 +259,49 @@
>  #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
>  #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base Address Register */
Here the value of HVBAR...

>  
> +/* 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)
> +#define __LR8(x)                  ___CP32(p15, 4, c12, c13, x)
> +
> +#define ICH_LR0                   __LR0(0)
> +#define ICH_LR1                   __LR0(1)
> +#define ICH_LR2                   __LR0(2)
> +#define ICH_LR3                   __LR0(3)
> +#define ICH_LR4                   __LR0(4)
> +#define ICH_LR5                   __LR0(5)
> +#define ICH_LR6                   __LR0(6)
> +#define ICH_LR7                   __LR0(7)
> +#define ICH_LR8                   __LR8(0)
> +#define ICH_LR9                   __LR8(1)
> +#define ICH_LR10                  __LR8(2)
> +#define ICH_LR11                  __LR8(3)
> +#define ICH_LR12                  __LR8(4)
> +#define ICH_LR13                  __LR8(5)
> +#define ICH_LR14                  __LR8(6)
> +#define ICH_LR15                  __LR8(7)
> +
> +/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
> +#define __LRC0(x)                 ___CP32(p15, 4, c12, c14, x)
> +#define __LRC8(x)                 ___CP32(p15, 4, c12, c15, x)
> +
> +#define ICH_LRC0                  __LRC0(0)
> +#define ICH_LRC1                  __LRC0(1)
> +#define ICH_LRC2                  __LRC0(2)
> +#define ICH_LRC3                  __LRC0(3)
> +#define ICH_LRC4                  __LRC0(4)
> +#define ICH_LRC5                  __LRC0(5)
> +#define ICH_LRC6                  __LRC0(6)
> +#define ICH_LRC7                  __LRC0(7)
> +#define ICH_LRC8                  __LRC8(0)
> +#define ICH_LRC9                  __LRC8(1)
> +#define ICH_LRC10                 __LRC8(2)
> +#define ICH_LRC11                 __LRC8(3)
> +#define ICH_LRC12                 __LRC8(4)
> +#define ICH_LRC13                 __LRC8(5)
> +#define ICH_LRC14                 __LRC8(6)
> +#define ICH_LRC15                 __LRC8(7)
> +
>  /* CP15 CR13:  */
>  #define FCSEIDR         p15,0,c13,c0,0  /* FCSE Process ID Register */
is aligned to the value of FCSEIDR.
So I'm not sure why you cannot align your code block to it.
Generally a new code block can have its own alignement, but here you did insert
it into something aligned which no longer is after your change.

>  #define CONTEXTIDR      p15,0,c13,c0,1  /* Context ID Register */
> @@ -317,6 +360,38 @@
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
> +#define ICH_LR0_EL2             ICH_LR0
> +#define ICH_LR1_EL2             ICH_LR1
> +#define ICH_LR2_EL2             ICH_LR2
> +#define ICH_LR3_EL2             ICH_LR3
> +#define ICH_LR4_EL2             ICH_LR4
> +#define ICH_LR5_EL2             ICH_LR5
> +#define ICH_LR6_EL2             ICH_LR6
> +#define ICH_LR7_EL2             ICH_LR7
> +#define ICH_LR8_EL2             ICH_LR8
> +#define ICH_LR9_EL2             ICH_LR9
> +#define ICH_LR10_EL2            ICH_LR10
> +#define ICH_LR11_EL2            ICH_LR11
> +#define ICH_LR12_EL2            ICH_LR12
> +#define ICH_LR13_EL2            ICH_LR13
> +#define ICH_LR14_EL2            ICH_LR14
> +#define ICH_LR15_EL2            ICH_LR15
> +#define ICH_LRC0_EL2            ICH_LRC0
> +#define ICH_LRC1_EL2            ICH_LRC1
> +#define ICH_LRC2_EL2            ICH_LRC2
> +#define ICH_LRC3_EL2            ICH_LRC3
> +#define ICH_LRC4_EL2            ICH_LRC4
> +#define ICH_LRC5_EL2            ICH_LRC5
> +#define ICH_LRC6_EL2            ICH_LRC6
> +#define ICH_LRC7_EL2            ICH_LRC7
> +#define ICH_LRC8_EL2            ICH_LRC8
> +#define ICH_LRC9_EL2            ICH_LRC9
> +#define ICH_LRC10_EL2           ICH_LRC10
> +#define ICH_LRC11_EL2           ICH_LRC11
> +#define ICH_LRC12_EL2           ICH_LRC12
> +#define ICH_LRC13_EL2           ICH_LRC13
> +#define ICH_LRC14_EL2           ICH_LRC14
> +#define ICH_LRC15_EL2           ICH_LRC15
>  #define ID_AFR0_EL1             ID_AFR0
>  #define ID_DFR0_EL1             ID_DFR0
>  #define ID_DFR1_EL1             ID_DFR1
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 48a1bc401e..743ebb20fd 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -185,9 +185,9 @@
>  #define ICH_LR_HW_SHIFT              61
>  #define ICH_LR_GRP_MASK              0x1
>  #define ICH_LR_GRP_SHIFT             60
> -#define ICH_LR_MAINTENANCE_IRQ       (1UL<<41)
> -#define ICH_LR_GRP1                  (1UL<<60)
> -#define ICH_LR_HW                    (1UL<<61)
> +#define ICH_LR_MAINTENANCE_IRQ       (1ULL << 41)
> +#define ICH_LR_GRP1                  (1ULL << 60)
> +#define ICH_LR_HW                    (1ULL << 61)
>  
>  #define ICH_VTR_NRLRGS               0x3f
>  #define ICH_VTR_PRIBITS_MASK         0x7

~Michal

[XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32

[Ayan Kumar Halder]

Adapt save_aprn_regs()/restore_aprn_regs() for AArch32.

For which we have defined the following registers:-
1. Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
2. Interrupt Controller Hyp Active Priorities Group1 Registers 0-3

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from :-
v1 - 1. Moved coproc register definition to asm/cpregs.h.

v2 - 1. Defined register alias.
2. Style issues.
3. Dropped R-b and Ack.

 xen/arch/arm/include/asm/cpregs.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index 242dabaea8..5331ec3448 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -259,6 +259,26 @@
 #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
 #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base Address Register */
 
+/*
+ * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 0 Registers,
+ * n = 0 - 3
+ */
+#define __AP0Rx(x)      ___CP32(p15, 4, c12, c8, x)
+#define ICH_AP0R0       __AP0Rx(0)
+#define ICH_AP0R1       __AP0Rx(1)
+#define ICH_AP0R2       __AP0Rx(2)
+#define ICH_AP0R3       __AP0Rx(3)
+
+/*
+ * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 1 Registers,
+ * n = 0 - 3
+ */
+#define __AP1Rx(x)      ___CP32(p15, 4, c12, c9, x)
+#define ICH_AP1R0       __AP1Rx(0)
+#define ICH_AP1R1       __AP1Rx(1)
+#define ICH_AP1R2       __AP1Rx(2)
+#define ICH_AP1R3       __AP1Rx(3)
+
 /* 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)
@@ -360,6 +380,14 @@
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define HSTR_EL2                HSTR
+#define ICH_AP0R0_EL2           ICH_AP0R0
+#define ICH_AP0R1_EL2           ICH_AP0R1
+#define ICH_AP0R2_EL2           ICH_AP0R2
+#define ICH_AP0R3_EL2           ICH_AP0R3
+#define ICH_AP1R0_EL2           ICH_AP1R0
+#define ICH_AP1R1_EL2           ICH_AP1R1
+#define ICH_AP1R2_EL2           ICH_AP1R2
+#define ICH_AP1R3_EL2           ICH_AP1R3
 #define ICH_LR0_EL2             ICH_LR0
 #define ICH_LR1_EL2             ICH_LR1
 #define ICH_LR2_EL2             ICH_LR2
-- 
2.17.1

Re: [XEN v3 08/12] xen/Arm: GICv3: Define ICH_AP0R<n> and ICH_AP1R<n> for AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> Adapt save_aprn_regs()/restore_aprn_regs() for AArch32.
> 
> For which we have defined the following registers:-
> 1. Interrupt Controller Hyp Active Priorities Group0 Registers 0-3
> 2. Interrupt Controller Hyp Active Priorities Group1 Registers 0-3
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from :-
> v1 - 1. Moved coproc register definition to asm/cpregs.h.
> 
> v2 - 1. Defined register alias.
> 2. Style issues.
> 3. Dropped R-b and Ack.
> 
>  xen/arch/arm/include/asm/cpregs.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index 242dabaea8..5331ec3448 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -259,6 +259,26 @@
>  #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
>  #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base Address Register */
>  
> +/*
> + * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 0 Registers,
> + * n = 0 - 3
> + */
> +#define __AP0Rx(x)      ___CP32(p15, 4, c12, c8, x)
You are using a macro ___CP32 here but it is defined somewhere down the file.
So I think you need to make a change so that the definition appears before use.

> +#define ICH_AP0R0       __AP0Rx(0)
> +#define ICH_AP0R1       __AP0Rx(1)
> +#define ICH_AP0R2       __AP0Rx(2)
> +#define ICH_AP0R3       __AP0Rx(3)
> +
> +/*
> + * CP15 CR12: Interrupt Controller Hyp Active Priorities Group 1 Registers,
> + * n = 0 - 3
> + */
> +#define __AP1Rx(x)      ___CP32(p15, 4, c12, c9, x)
> +#define ICH_AP1R0       __AP1Rx(0)
> +#define ICH_AP1R1       __AP1Rx(1)
> +#define ICH_AP1R2       __AP1Rx(2)
> +#define ICH_AP1R3       __AP1Rx(3)
> +
>  /* 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)
> @@ -360,6 +380,14 @@
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
> +#define ICH_AP0R0_EL2           ICH_AP0R0
> +#define ICH_AP0R1_EL2           ICH_AP0R1
> +#define ICH_AP0R2_EL2           ICH_AP0R2
> +#define ICH_AP0R3_EL2           ICH_AP0R3
> +#define ICH_AP1R0_EL2           ICH_AP1R0
> +#define ICH_AP1R1_EL2           ICH_AP1R1
> +#define ICH_AP1R2_EL2           ICH_AP1R2
> +#define ICH_AP1R3_EL2           ICH_AP1R3
>  #define ICH_LR0_EL2             ICH_LR0
>  #define ICH_LR1_EL2             ICH_LR1
>  #define ICH_LR2_EL2             ICH_LR2

~Michal

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

[Ayan Kumar Halder]

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

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 */
 #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 */
+#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 */
+#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
+#define ICC_SRE_EL2             ICC_SRE
 #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
-- 
2.17.1

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

[Michal Orzel]

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

[XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits

[Ayan Kumar Halder]

"unsigned long long" is defined as 64 bits on AArch64 and AArch32
Thus, one should this instead of "unsigned long" which is 32 bits
on AArch32.

Also use 'PRIx64' instead of 'lx' or 'llx' to print uint64_t.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changed from :-
v1 - 1. Replace PRIu64 with PRIx64 so that the values are printed in hex as
desired.
2. Use ULL in GITS_BASER_RO_MASK as MMIO registers are always unsigned.

v2 - 1. Removed changes to ITS and LPI as they are not supported for AArch32.

 xen/arch/arm/gic-v3.c                  | 4 ++--
 xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4722bb4daf..6457e7033c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
     if ( v == current )
     {
         for ( i = 0; i < gicv3_info.nr_lrs; i++ )
-            printk("   HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
+            printk("   HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
     }
     else
     {
         for ( i = 0; i < gicv3_info.nr_lrs; i++ )
-            printk("   VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
+            printk("   VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
     }
 }
 
diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
index 743ebb20fd..227533868f 100644
--- a/xen/arch/arm/include/asm/gic_v3_defs.h
+++ b/xen/arch/arm/include/asm/gic_v3_defs.h
@@ -195,7 +195,7 @@
 
 #define ICH_SGI_IRQMODE_SHIFT        40
 #define ICH_SGI_IRQMODE_MASK         0x1
-#define ICH_SGI_TARGET_OTHERS        1UL
+#define ICH_SGI_TARGET_OTHERS        1ULL
 #define ICH_SGI_TARGET_LIST          0
 #define ICH_SGI_IRQ_SHIFT            24
 #define ICH_SGI_IRQ_MASK             0xf
-- 
2.17.1

Re: [XEN v3 10/12] xen/Arm: GICv3: Use ULL instead of UL for 64bits

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> "unsigned long long" is defined as 64 bits on AArch64 and AArch32
> Thus, one should this instead of "unsigned long" which is 32 bits
> on AArch32.
> 
> Also use 'PRIx64' instead of 'lx' or 'llx' to print uint64_t.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changed from :-
> v1 - 1. Replace PRIu64 with PRIx64 so that the values are printed in hex as
> desired.
> 2. Use ULL in GITS_BASER_RO_MASK as MMIO registers are always unsigned.
> 
> v2 - 1. Removed changes to ITS and LPI as they are not supported for AArch32.
> 
>  xen/arch/arm/gic-v3.c                  | 4 ++--
>  xen/arch/arm/include/asm/gic_v3_defs.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4722bb4daf..6457e7033c 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -417,12 +417,12 @@ static void gicv3_dump_state(const struct vcpu *v)
>      if ( v == current )
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   HW_LR[%d]=%llx\n", i, gicv3_ich_read_lr(i));
> +            printk("   HW_LR[%d]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
>      }
>      else
>      {
>          for ( i = 0; i < gicv3_info.nr_lrs; i++ )
> -            printk("   VCPU_LR[%d]=%llx\n", i, v->arch.gic.v3.lr[i]);
> +            printk("   VCPU_LR[%d]=%" PRIx64 "\n", i, v->arch.gic.v3.lr[i]);
You changed these specifiers to be llx in patch no.7 so such a change (using PRIx64) should
be done in that patch. Generally there is no need for a patch to fix something that you
introduced earlier in the series. It should be done in one step. Having said that...

>      }
>  }
>  
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 743ebb20fd..227533868f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -195,7 +195,7 @@
>  
>  #define ICH_SGI_IRQMODE_SHIFT        40
>  #define ICH_SGI_IRQMODE_MASK         0x1
> -#define ICH_SGI_TARGET_OTHERS        1UL
> +#define ICH_SGI_TARGET_OTHERS        1ULL
>  #define ICH_SGI_TARGET_LIST          0
>  #define ICH_SGI_IRQ_SHIFT            24
>  #define ICH_SGI_IRQ_MASK             0xf

adding a patch for just this macro is not very useful and you could take the opportunity
to modify it in any of your patches.

~Michal

[XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Ayan Kumar Halder]

On AArch32, ldrd/strd instructions are not atomic when used to access MMIO.
Furthermore, ldrd/strd instructions are not decoded by Arm when running as
a guest to access emulated MMIO region.
Thus, we have defined readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
which in turn calls readl_relaxed()/writel_relaxed() for the lower and upper
32 bits.
As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changes from :-
v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
2. No need to use le64_to_cpu() as the returned byte order is already in cpu
endianess.

v2 - 1. Replace {read/write}q_relaxed with {read/write}q_relaxed_non_atomic().

 xen/arch/arm/gic-v3.c               | 12 ++++++++++++
 xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
 2 files changed, 21 insertions(+)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6457e7033c..a5bc549765 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
     affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
 
     for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
+#ifdef CONFIG_ARM_32
+        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
+#else
         writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
+#endif
 }
 
 static int gicv3_enable_redist(void)
@@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
         }
 
         do {
+#ifdef CONFIG_ARM_32
+            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
+#else
             typer = readq_relaxed(ptr + GICR_TYPER);
+#endif
 
             if ( (typer >> 32) == aff )
             {
@@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
     affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
 
     if ( desc->irq >= NR_GIC_LOCAL_IRQS )
+#ifdef CONFIG_ARM_32
+        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
+#else
         writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
+#endif
 
     spin_unlock(&gicv3.lock);
 }
diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h
index 73a879e9fb..4ddfbea5c2 100644
--- a/xen/arch/arm/include/asm/arm32/io.h
+++ b/xen/arch/arm/include/asm/arm32/io.h
@@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
                                         __raw_readw(c)); __r; })
 #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
                                         __raw_readl(c)); __r; })
+#define readq_relaxed_non_atomic(c) \
+                         ({ u64 __r = (((u64)readl_relaxed((c) + 4)) << 32) | \
+                                             readl_relaxed(c); __r; })
 
 #define writeb_relaxed(v,c)     __raw_writeb(v,c)
 #define writew_relaxed(v,c)     __raw_writew((__force u16) cpu_to_le16(v),c)
 #define writel_relaxed(v,c)     __raw_writel((__force u32) cpu_to_le32(v),c)
+#define writeq_relaxed_non_atomic(v,c) \
+                                ({ writel_relaxed((u32)v, c); \
+                                   writel_relaxed((u32)((v) >> 32), (c) + 4); })
 
 #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
 #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
 #define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
+#define readq(c)                ({ u64 __v = readq_relaxed_non_atomic(c); \
+                                             __iormb(); __v; })
 
 #define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
 #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
 #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
+#define writeq(v,c)             ({ __iowmb(); writeq_relaxed_non_atomic(v,c); })
 
 #endif /* _ARM_ARM32_IO_H */
-- 
2.17.1

Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Xenia Ragiadakou]

Hi Ayan,

On 11/11/22 16:17, Ayan Kumar Halder wrote:
> On AArch32, ldrd/strd instructions are not atomic when used to access MMIO.
> Furthermore, ldrd/strd instructions are not decoded by Arm when running as
> a guest to access emulated MMIO region.
> Thus, we have defined readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
> which in turn calls readl_relaxed()/writel_relaxed() for the lower and upper
> 32 bits.
> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a non atomic
> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changes from :-
> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
> 2. No need to use le64_to_cpu() as the returned byte order is already in cpu
> endianess.
> 
> v2 - 1. Replace {read/write}q_relaxed with {read/write}q_relaxed_non_atomic().
> 
>   xen/arch/arm/gic-v3.c               | 12 ++++++++++++
>   xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6457e7033c..a5bc549765 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>   
>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
> +#ifdef CONFIG_ARM_32
> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i * 8);
> +#else
>           writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
> +#endif
>   }
>   
>   static int gicv3_enable_redist(void)
> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>           }
>   
>           do {
> +#ifdef CONFIG_ARM_32
> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
> +#else
>               typer = readq_relaxed(ptr + GICR_TYPER);
> +#endif
>   
>               if ( (typer >> 32) == aff )
>               {
> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>   
>       if ( desc->irq >= NR_GIC_LOCAL_IRQS )
> +#ifdef CONFIG_ARM_32
> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +#else
>           writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 8));
> +#endif
>   
>       spin_unlock(&gicv3.lock);
>   }
> diff --git a/xen/arch/arm/include/asm/arm32/io.h b/xen/arch/arm/include/asm/arm32/io.h
> index 73a879e9fb..4ddfbea5c2 100644
> --- a/xen/arch/arm/include/asm/arm32/io.h
> +++ b/xen/arch/arm/include/asm/arm32/io.h
> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
>                                           __raw_readw(c)); __r; })
>   #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>                                           __raw_readl(c)); __r; })
> +#define readq_relaxed_non_atomic(c) \
> +                         ({ u64 __r = (((u64)readl_relaxed((c) + 4)) << 32) | \
> +                                             readl_relaxed(c); __r; })

As Julien pointed out, the expression c will be evaluated twice and if 
it produces side effects they will be performed twice.
To prevent this, you can either assign the expression to a local 
variable and pass this one to readl_relaxed() or use a static inline 
function instead of a macro, for implementing readq_relaxed_non_atomic().
The latter is the MISRA C recommended (not strictly required) approach 
according to Dir 4.9 "A function should be used in preference to a 
function-like macro where
  they are interchangeable".
...

>   
>   #define writeb_relaxed(v,c)     __raw_writeb(v,c)
>   #define writew_relaxed(v,c)     __raw_writew((__force u16) cpu_to_le16(v),c)
>   #define writel_relaxed(v,c)     __raw_writel((__force u32) cpu_to_le32(v),c)
> +#define writeq_relaxed_non_atomic(v,c) \
> +                                ({ writel_relaxed((u32)v, c); \
> +                                   writel_relaxed((u32)((v) >> 32), (c) + 4); })

... same here.

>   
>   #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
>   #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
>   #define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
> +#define readq(c)                ({ u64 __v = readq_relaxed_non_atomic(c); \
> +                                             __iormb(); __v; })

I think that, here also, the macro identifier needs to inform that the 
access is non-atomic.
...

>   
>   #define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
>   #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
>   #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
> +#define writeq(v,c)             ({ __iowmb(); writeq_relaxed_non_atomic(v,c); })

... same here.

>   
>   #endif /* _ARM_ARM32_IO_H */

-- 
Xenia

Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Ayan Kumar Halder]

On 11/11/2022 16:17, Xenia Ragiadakou wrote:
> Hi Ayan,
Hi Xenia,
>
> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>> On AArch32, ldrd/strd instructions are not atomic when used to access 
>> MMIO.
>> Furthermore, ldrd/strd instructions are not decoded by Arm when 
>> running as
>> a guest to access emulated MMIO region.
>> Thus, we have defined 
>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>> which in turn calls readl_relaxed()/writel_relaxed() for the lower 
>> and upper
>> 32 bits.
>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a 
>> non atomic
>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> Changes from :-
>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>> 2. No need to use le64_to_cpu() as the returned byte order is already 
>> in cpu
>> endianess.
>>
>> v2 - 1. Replace {read/write}q_relaxed with 
>> {read/write}q_relaxed_non_atomic().
>>
>>   xen/arch/arm/gic-v3.c               | 12 ++++++++++++
>>   xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 6457e7033c..a5bc549765 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>         for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>> +#ifdef CONFIG_ARM_32
>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i 
>> * 8);
>> +#else
>>           writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>> +#endif
>>   }
>>     static int gicv3_enable_redist(void)
>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>>           }
>>             do {
>> +#ifdef CONFIG_ARM_32
>> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>> +#else
>>               typer = readq_relaxed(ptr + GICR_TYPER);
>> +#endif
>>                 if ( (typer >> 32) == aff )
>>               {
>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct 
>> irq_desc *desc, const cpumask_t *mask)
>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>         if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>> +#ifdef CONFIG_ARM_32
>> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
>> desc->irq * 8));
>> +#else
>>           writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 
>> 8));
>> +#endif
>>         spin_unlock(&gicv3.lock);
>>   }
>> diff --git a/xen/arch/arm/include/asm/arm32/io.h 
>> b/xen/arch/arm/include/asm/arm32/io.h
>> index 73a879e9fb..4ddfbea5c2 100644
>> --- a/xen/arch/arm/include/asm/arm32/io.h
>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void 
>> __iomem *addr)
>>                                           __raw_readw(c)); __r; })
>>   #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>>                                           __raw_readl(c)); __r; })
>> +#define readq_relaxed_non_atomic(c) \
>> +                         ({ u64 __r = (((u64)readl_relaxed((c) + 4)) 
>> << 32) | \
>> +                                             readl_relaxed(c); __r; })
>
> As Julien pointed out, the expression c will be evaluated twice and if 
> it produces side effects they will be performed twice.
> To prevent this, you can either assign the expression to a local 
> variable and pass this one to readl_relaxed() 

Just to make sure I understand you correctly, you are suggesting this :-

#define readq_relaxed_non_atomic(c) \

                         ({ void _iomem *__addr = (c); \

                             u64 __r = (((u64)readl_relaxed(__addr + 4)) 
<< 32) | \

readl_relaxed(__addr); __r; })

#define writeq_relaxed_non_atomic(v,c) \

                        (( u64 __v = (v); \

                           void _iomem *__addr = (c); \

                           writel_relaxed((u32)__v, __addr); \

                           writel_relaxed((u32)((__v) >> 32), (__addr + 
4); })

Is this correct understanding ?

> or use a static inline function instead of a macro, for implementing 
> readq_relaxed_non_atomic().
> The latter is the MISRA C recommended (not strictly required) approach 
> according to Dir 4.9 "A function should be used in preference to a 
> function-like macro where
>  they are interchangeable".

I have mixed opinion about this.

On one hand, there will be a performance penalty when invoking a 
function (compared to macro).

On the other hand {readq/writeq}_relaxed_non_atomic() are called during 
init (gicv3 initialization, setting up the interrupt handlers), so the 
impact will not be bad.

I am fine with whatever you and any maintainer suggest.

Also now I realize that I had missed another point raised by Julien (a 
code comment explaining why ldrd/strd cannot be used) :(.

I will address this in my next version

> ...
>
>>     #define writeb_relaxed(v,c) __raw_writeb(v,c)
>>   #define writew_relaxed(v,c)     __raw_writew((__force u16) 
>> cpu_to_le16(v),c)
>>   #define writel_relaxed(v,c)     __raw_writel((__force u32) 
>> cpu_to_le32(v),c)
>> +#define writeq_relaxed_non_atomic(v,c) \
>> +                                ({ writel_relaxed((u32)v, c); \
>> +                                   writel_relaxed((u32)((v) >> 32), 
>> (c) + 4); })
>
> ... same here.
Ack
>
>>     #define readb(c)                ({ u8 __v = readb_relaxed(c); 
>> __iormb(); __v; })
>>   #define readw(c)                ({ u16 __v = readw_relaxed(c); 
>> __iormb(); __v; })
>>   #define readl(c)                ({ u32 __v = readl_relaxed(c); 
>> __iormb(); __v; })
>> +#define readq(c)                ({ u64 __v = 
>> readq_relaxed_non_atomic(c); \
>> +                                             __iormb(); __v; })
>
> I think that, here also, the macro identifier needs to inform that the 
> access is non-atomic.
I will remove this as we will be using readq_relaxed_non_atomic() in the 
code.
> ...
>
>>     #define writeb(v,c)             ({ __iowmb(); 
>> writeb_relaxed(v,c); })
>>   #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
>>   #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
>> +#define writeq(v,c)             ({ __iowmb(); 
>> writeq_relaxed_non_atomic(v,c); })
>
> ... same here.

I will remove this as we will be using writeq_relaxed_non_atomic()  in 
the code.

- Ayan

>
>>     #endif /* _ARM_ARM32_IO_H */
>

Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Julien Grall]

Hi,

On 11/11/2022 17:37, Ayan Kumar Halder wrote:
> 
> On 11/11/2022 16:17, Xenia Ragiadakou wrote:
>> Hi Ayan,
> Hi Xenia,
>>
>> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>>> On AArch32, ldrd/strd instructions are not atomic when used to access 
>>> MMIO.
>>> Furthermore, ldrd/strd instructions are not decoded by Arm when 
>>> running as
>>> a guest to access emulated MMIO region.
>>> Thus, we have defined 
>>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>>> which in turn calls readl_relaxed()/writel_relaxed() for the lower 
>>> and upper
>>> 32 bits.
>>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a 
>>> non atomic
>>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>>
>>> Changes from :-
>>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>>> 2. No need to use le64_to_cpu() as the returned byte order is already 
>>> in cpu
>>> endianess.
>>>
>>> v2 - 1. Replace {read/write}q_relaxed with 
>>> {read/write}q_relaxed_non_atomic().
>>>
>>>   xen/arch/arm/gic-v3.c               | 12 ++++++++++++
>>>   xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index 6457e7033c..a5bc549765 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>         for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>>> +#ifdef CONFIG_ARM_32
>>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + i 
>>> * 8);
>>> +#else
>>>           writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>>> +#endif

I would have been OK if there was one place needed a #ifdef. But 2 is a 
bit too much.

Please provide a wrapper writeq_relaxed_non_atomic() for arm64. The 
implementation would call writeq(). The same stands for...

>>>   }
>>>     static int gicv3_enable_redist(void)
>>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>>>           }
>>>             do {
>>> +#ifdef CONFIG_ARM_32
>>> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>>> +#else
>>>               typer = readq_relaxed(ptr + GICR_TYPER);
>>> +#endif

... here.

>>>                 if ( (typer >> 32) == aff )
>>>               {
>>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct 
>>> irq_desc *desc, const cpumask_t *mask)
>>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>         if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>>> +#ifdef CONFIG_ARM_32
>>> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
>>> desc->irq * 8));
>>> +#else
>>>           writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq * 
>>> 8));
>>> +#endif
>>>         spin_unlock(&gicv3.lock);
>>>   }
>>> diff --git a/xen/arch/arm/include/asm/arm32/io.h 
>>> b/xen/arch/arm/include/asm/arm32/io.h
>>> index 73a879e9fb..4ddfbea5c2 100644
>>> --- a/xen/arch/arm/include/asm/arm32/io.h
>>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile void 
>>> __iomem *addr)
>>>                                           __raw_readw(c)); __r; })
>>>   #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>>>                                           __raw_readl(c)); __r; })
>>> +#define readq_relaxed_non_atomic(c) \
>>> +                         ({ u64 __r = (((u64)readl_relaxed((c) + 4)) 
>>> << 32) | \
>>> +                                             readl_relaxed(c); __r; })
>>
>> As Julien pointed out, the expression c will be evaluated twice and if 
>> it produces side effects they will be performed twice.
>> To prevent this, you can either assign the expression to a local 
>> variable and pass this one to readl_relaxed() 
> 
> Just to make sure I understand you correctly, you are suggesting this :-
> 
> #define readq_relaxed_non_atomic(c) \
> 
>                          ({ void _iomem *__addr = (c); \
> 
>                              u64 __r = (((u64)readl_relaxed(__addr + 4)) 
> << 32) | \
> 
> readl_relaxed(__addr); __r; })
> 
> #define writeq_relaxed_non_atomic(v,c) \
> 
>                         (( u64 __v = (v); \
> 
>                            void _iomem *__addr = (c); \
> 
>                            writel_relaxed((u32)__v, __addr); \
> 
>                            writel_relaxed((u32)((__v) >> 32), (__addr + 
> 4); })


> 
> Is this correct understanding ?
> 
>> or use a static inline function instead of a macro, for implementing 
>> readq_relaxed_non_atomic().
>> The latter is the MISRA C recommended (not strictly required) approach 
>> according to Dir 4.9 "A function should be used in preference to a 
>> function-like macro where
>>  they are interchangeable".
> 
> I have mixed opinion about this.
> 
> On one hand, there will be a performance penalty when invoking a 
> function (compared to macro).

Most of the compilers are nowadays clever enough to inline small 
functions. But we can force the compiler with the attribute always_inline.

> 
> On the other hand {readq/writeq}_relaxed_non_atomic() are called during 
> init (gicv3 initialization, setting up the interrupt handlers), so the 
> impact will not be bad.
> 
> I am fine with whatever you and any maintainer suggest.

Project wide, we are trying to use "static inline" whenever it is 
possible because it adds a bit more type-safety (the error you made 
wouldn't have happened) and the code is clearer (no slash).

So my preference is to use static line.

Cheers,

-- 
Julien Grall

Re: [XEN v3 11/12] xen/Arm: GICv3: Define macros to read/write 64 bit

[Ayan Kumar Halder]

On 11/11/2022 17:53, Julien Grall wrote:
> Hi,

Hi Julien,

I need a clarification.

>
> On 11/11/2022 17:37, Ayan Kumar Halder wrote:
>>
>> On 11/11/2022 16:17, Xenia Ragiadakou wrote:
>>> Hi Ayan,
>> Hi Xenia,
>>>
>>> On 11/11/22 16:17, Ayan Kumar Halder wrote:
>>>> On AArch32, ldrd/strd instructions are not atomic when used to 
>>>> access MMIO.
>>>> Furthermore, ldrd/strd instructions are not decoded by Arm when 
>>>> running as
>>>> a guest to access emulated MMIO region.
>>>> Thus, we have defined 
>>>> readq_relaxed_non_atomic()/writeq_relaxed_non_atomic()
>>>> which in turn calls readl_relaxed()/writel_relaxed() for the lower 
>>>> and upper
>>>> 32 bits.
>>>> As GICv3 registers (GICD_IROUTER, GICR_TYPER) can be accessed in a 
>>>> non atomic
>>>> fashion, so we have used {read/write}q_relaxed_non_atomic() on Arm32.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>>
>>>> Changes from :-
>>>> v1 - 1. Use ldrd/strd for readq_relaxed()/writeq_relaxed().
>>>> 2. No need to use le64_to_cpu() as the returned byte order is 
>>>> already in cpu
>>>> endianess.
>>>>
>>>> v2 - 1. Replace {read/write}q_relaxed with 
>>>> {read/write}q_relaxed_non_atomic().
>>>>
>>>>   xen/arch/arm/gic-v3.c               | 12 ++++++++++++
>>>>   xen/arch/arm/include/asm/arm32/io.h |  9 +++++++++
>>>>   2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>>> index 6457e7033c..a5bc549765 100644
>>>> --- a/xen/arch/arm/gic-v3.c
>>>> +++ b/xen/arch/arm/gic-v3.c
>>>> @@ -651,7 +651,11 @@ static void __init gicv3_dist_init(void)
>>>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>>         for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
>>>> +#ifdef CONFIG_ARM_32
>>>> +        writeq_relaxed_non_atomic(affinity, GICD + GICD_IROUTER + 
>>>> i * 8);
>>>> +#else
>>>>           writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
>>>> +#endif
>
> I would have been OK if there was one place needed a #ifdef. But 2 is 
> a bit too much.
>
> Please provide a wrapper writeq_relaxed_non_atomic() for arm64. The 
> implementation would call writeq(). The same stands for...

For arm64, why shouldn't we invoke {read/write}q_relaxed() for 
{read/write}q_relaxed_non_atomic() ?

As I understand, this will be less expensive than invoking 
readq()/writeq() (as it will introduce memory barriers).

Also, the original code was invoking {read/write}q_relaxed() for arm64.

Please let me know if I am missing something ?

- Ayan

>
>>>>   }
>>>>     static int gicv3_enable_redist(void)
>>>> @@ -745,7 +749,11 @@ static int __init gicv3_populate_rdist(void)
>>>>           }
>>>>             do {
>>>> +#ifdef CONFIG_ARM_32
>>>> +            typer = readq_relaxed_non_atomic(ptr + GICR_TYPER);
>>>> +#else
>>>>               typer = readq_relaxed(ptr + GICR_TYPER);
>>>> +#endif
>
> ... here.
>
>>>>                 if ( (typer >> 32) == aff )
>>>>               {
>>>> @@ -1265,7 +1273,11 @@ static void gicv3_irq_set_affinity(struct 
>>>> irq_desc *desc, const cpumask_t *mask)
>>>>       affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
>>>>         if ( desc->irq >= NR_GIC_LOCAL_IRQS )
>>>> +#ifdef CONFIG_ARM_32
>>>> +        writeq_relaxed_non_atomic(affinity, (GICD + GICD_IROUTER + 
>>>> desc->irq * 8));
>>>> +#else
>>>>           writeq_relaxed(affinity, (GICD + GICD_IROUTER + desc->irq 
>>>> * 8));
>>>> +#endif
>>>>         spin_unlock(&gicv3.lock);
>>>>   }
>>>> diff --git a/xen/arch/arm/include/asm/arm32/io.h 
>>>> b/xen/arch/arm/include/asm/arm32/io.h
>>>> index 73a879e9fb..4ddfbea5c2 100644
>>>> --- a/xen/arch/arm/include/asm/arm32/io.h
>>>> +++ b/xen/arch/arm/include/asm/arm32/io.h
>>>> @@ -80,17 +80,26 @@ static inline u32 __raw_readl(const volatile 
>>>> void __iomem *addr)
>>>>                                           __raw_readw(c)); __r; })
>>>>   #define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
>>>>                                           __raw_readl(c)); __r; })
>>>> +#define readq_relaxed_non_atomic(c) \
>>>> +                         ({ u64 __r = (((u64)readl_relaxed((c) + 
>>>> 4)) << 32) | \
>>>> + readl_relaxed(c); __r; })
>>>
>>> As Julien pointed out, the expression c will be evaluated twice and 
>>> if it produces side effects they will be performed twice.
>>> To prevent this, you can either assign the expression to a local 
>>> variable and pass this one to readl_relaxed() 
>>
>> Just to make sure I understand you correctly, you are suggesting this :-
>>
>> #define readq_relaxed_non_atomic(c) \
>>
>>                          ({ void _iomem *__addr = (c); \
>>
>>                              u64 __r = (((u64)readl_relaxed(__addr + 
>> 4)) << 32) | \
>>
>> readl_relaxed(__addr); __r; })
>>
>> #define writeq_relaxed_non_atomic(v,c) \
>>
>>                         (( u64 __v = (v); \
>>
>>                            void _iomem *__addr = (c); \
>>
>>                            writel_relaxed((u32)__v, __addr); \
>>
>>                            writel_relaxed((u32)((__v) >> 32), (__addr 
>> + 4); })
>
>
>>
>> Is this correct understanding ?
>>
>>> or use a static inline function instead of a macro, for implementing 
>>> readq_relaxed_non_atomic().
>>> The latter is the MISRA C recommended (not strictly required) 
>>> approach according to Dir 4.9 "A function should be used in 
>>> preference to a function-like macro where
>>>  they are interchangeable".
>>
>> I have mixed opinion about this.
>>
>> On one hand, there will be a performance penalty when invoking a 
>> function (compared to macro).
>
> Most of the compilers are nowadays clever enough to inline small 
> functions. But we can force the compiler with the attribute 
> always_inline.
>
>>
>> On the other hand {readq/writeq}_relaxed_non_atomic() are called 
>> during init (gicv3 initialization, setting up the interrupt 
>> handlers), so the impact will not be bad.
>>
>> I am fine with whatever you and any maintainer suggest.
>
> Project wide, we are trying to use "static inline" whenever it is 
> possible because it adds a bit more type-safety (the error you made 
> wouldn't have happened) and the code is clearer (no slash).
>
> So my preference is to use static line.
>
> Cheers,
>

[XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32

[Ayan Kumar Halder]

One can now use GICv3 on AArch32 systems. However, ITS is not supported.
The reason being currently we are trying to validate GICv3 on an AArch32_v8R
system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
"A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
implement LPI support."

By default GICv3 is disabled on AArch32 and enabled on AArch64.

Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

Changed from :-
v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
2. Updated SUPPORT.md.

v2 - 1. GICv3 is enabled by default only on ARM_64.
2. Updated SUPPORT.md.

 SUPPORT.md                            | 7 +++++++
 xen/arch/arm/Kconfig                  | 9 +++++----
 xen/arch/arm/include/asm/cpufeature.h | 1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/SUPPORT.md b/SUPPORT.md
index ab71464cf6..3f16d83191 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
 
     Status: Experimental
 
+### ARM/GICv3
+
+GICv3 is an interrupt controller specification designed by Arm.
+
+    Status, Arm64: Security supported
+    Status, Arm32: Supported, not security supported
+
 ## Guest Type
 
 ### x86/PV
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 1fe5faf847..b90930955b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
 	select 64BIT
 	select ARM_EFI
 	select HAS_FAST_MULTIPLY
+	select GICV3
 
 config ARM
 	def_bool y
@@ -41,16 +42,16 @@ config ARM_EFI
 
 config GICV3
 	bool "GICv3 driver"
-	depends on ARM_64 && !NEW_VGIC
-	default y
+	depends on !NEW_VGIC
+	default n
 	---help---
 
 	  Driver for the ARM Generic Interrupt Controller v3.
-	  If unsure, say Y
+	  If unsure, say N
 
 config HAS_ITS
         bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
-        depends on GICV3 && !NEW_VGIC
+        depends on GICV3 && !NEW_VGIC && !ARM_32
 
 config HVM
         def_bool y
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index c86a2e7f29..c62cf6293f 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -33,6 +33,7 @@
 #define cpu_has_aarch32   (cpu_has_arm || cpu_has_thumb)
 
 #ifdef CONFIG_ARM_32
+#define cpu_has_gicv3     (boot_cpu_feature32(gic) >= 1)
 #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
 /*
  * On Armv7, the value 0 is used to indicate that PMUv2 is not
-- 
2.17.1

Re: [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32

[Michal Orzel]

Hi Ayan,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> One can now use GICv3 on AArch32 systems. However, ITS is not supported.
> The reason being currently we are trying to validate GICv3 on an AArch32_v8R
> system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
> "A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
> implement LPI support."
> 
> By default GICv3 is disabled on AArch32 and enabled on AArch64.
> 
> Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

with one remark...

> ---
> 
> Changed from :-
> v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
> 2. Updated SUPPORT.md.
> 
> v2 - 1. GICv3 is enabled by default only on ARM_64.
> 2. Updated SUPPORT.md.
> 
>  SUPPORT.md                            | 7 +++++++
>  xen/arch/arm/Kconfig                  | 9 +++++----
>  xen/arch/arm/include/asm/cpufeature.h | 1 +
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index ab71464cf6..3f16d83191 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
>  
>      Status: Experimental
>  
> +### ARM/GICv3
... For me this section should go before the section ARM/GICv3 ITS.
The reason being ITS is an extension and your section being generic
should be listed before listing extension.

~Michal

Re: [XEN v3 12/12] xen/Arm: GICv3: Enable GICv3 for AArch32

[Julien Grall]

Hi,

On 11/11/2022 15:17, Ayan Kumar Halder wrote:
> One can now use GICv3 on AArch32 systems. However, ITS is not supported.
> The reason being currently we are trying to validate GICv3 on an AArch32_v8R
> system. Refer ARM DDI 0568A.c ID110520, B1.3.1,
> "A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE must not
> implement LPI support."
> 
> By default GICv3 is disabled on AArch32 and enabled on AArch64.
> 
> Updated SUPPORT.md to state that GICv3 on Arm32 is not security supported.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> Changed from :-
> v1 - 1. Remove "ARM_64 || ARM_32" as it is always true.
> 2. Updated SUPPORT.md.
> 
> v2 - 1. GICv3 is enabled by default only on ARM_64.
> 2. Updated SUPPORT.md.
> 
>   SUPPORT.md                            | 7 +++++++
>   xen/arch/arm/Kconfig                  | 9 +++++----
>   xen/arch/arm/include/asm/cpufeature.h | 1 +
>   3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index ab71464cf6..3f16d83191 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -82,6 +82,13 @@ Extension to the GICv3 interrupt controller to support MSI.
>   
>       Status: Experimental
>   
> +### ARM/GICv3
> +
> +GICv3 is an interrupt controller specification designed by Arm.
> +
> +    Status, Arm64: Security supported
> +    Status, Arm32: Supported, not security supported
> +
>   ## Guest Type
>   
>   ### x86/PV
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 1fe5faf847..b90930955b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -9,6 +9,7 @@ config ARM_64
>   	select 64BIT
>   	select ARM_EFI
>   	select HAS_FAST_MULTIPLY
> +	select GICV3

AFAIU "select" will force the GICV3 to be enabled. IOW, it would not be 
possible to disable it for Arm64 anymore. You want to remove the 
"select" and ...

>   
>   config ARM
>   	def_bool y
> @@ -41,16 +42,16 @@ config ARM_EFI
>   
>   config GICV3
>   	bool "GICv3 driver"
> -	depends on ARM_64 && !NEW_VGIC
> -	default y
> +	depends on !NEW_VGIC
> +	default n

... replace the "default n" with:

         default n if ARM_32
         default y if ARM_64

>   	---help---
>   
>   	  Driver for the ARM Generic Interrupt Controller v3.
> -	  If unsure, say Y
> +	  If unsure, say N

That's technically incorrect. For Arm64, we still want the user to 
select it by default. So I think we need to differentiate 32-bit vs 
64-bit for the "If unsure" part.

Cheers,

-- 
Julien Grall