[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi,

On 29/11/2022 14:33, Michal Orzel wrote:
@@ -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]=%" PRIx64 "\n", i, gicv3_ich_read_lr(i));
1. We do not usually add spaces between " and PRIx.

I don't have a strong preference on this one.


      }
      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]=%" PRIx64 "\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..22871999af 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..4638999514 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -472,6 +472,11 @@
  #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..b85e811f51 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -8,6 +8,8 @@
   * support 32-bit guests.
   */

+#define ___CP32(coproc, opc1, crn, crm, opc2) coproc, opc1, crn, crm, opc2
2. As you are using ___CP32 much later in this file, it could be moved...

__CP32() is already defined in arm32/sysregs.h which includes cpregs.h. We should not define __CP32() twice and the only reason the compiler doesn't complain is because the definition is the same

So one of the two needs to be dropped. Also, I think __CP32(), __CP64(), CP32() and CP64() should be defined together because they are all related.

However...

+
  #define __HSR_CPREG_c0  0
  #define __HSR_CPREG_c1  1
  #define __HSR_CPREG_c2  2
@@ -259,6 +261,48 @@
  #define VBAR            p15,0,c12,c0,0  /* Vector Base Address Register */
  #define HVBAR           p15,4,c12,c0,0  /* Hyp. Vector Base Address Register 
*/

...here, before first use. The remark I gave in v3 was that the definition 
should occur before use,
but it does not mean placing the macro at the top of the file.


+/* CP15 CR12: Interrupt Controller List Registers, n = 0 - 15 */
+#define __LR0(x)        ___CP32(p15, 4, c12, c12, x)
+#define __LR8(x)        ___CP32(p15, 4, c12, c13, x)

... I don't understand why you need to use __CP32 here and everywhere else in this header. In fact I have replaced in my tree all the __CP32(foo) with foo and it still compiles.

So can you explain why they are necessary?

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.