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

Re: [Xen-devel] [RFC 10/16] xen/arm: vcpreg: Add wrappers to handle co-proc access trapped by HCR_EL2.TVM

Hi Stefano,

On 11/5/18 7:47 PM, Stefano Stabellini wrote:
On Mon, 8 Oct 2018, Julien Grall wrote:
A follow-up patch will require to emulate some accesses to some
co-processors registers trapped by HCR_EL2.TVM. When set, all NS EL1 writes
to the virtual memory control registers will be trapped to the hypervisor.

This patch adds the infrastructure to passthrough the access to host
registers. For convenience a bunch of helpers have been added to
generate the different helpers.

Note that HCR_EL2.TVM will be set in a follow-up patch dynamically.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
  xen/arch/arm/vcpreg.c        | 144 +++++++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/cpregs.h |   1 +
  2 files changed, 145 insertions(+)

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index b04d996fd3..49529b97cd 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -24,6 +24,122 @@
  #include <asm/traps.h>
  #include <asm/vtimer.h>
+ * Macros to help generating helpers for registers trapped when
+ * HCR_EL2.TVM is set.
+ *
+ * Note that it only traps NS write access from EL1.
+ *
+ *  - TVM_REG() should not be used outside of the macros. It is there to
+ *    help defining TVM_REG32() and TVM_REG64()
+ *  - TVM_REG32(regname, xreg) and TVM_REG64(regname, xreg) are used to
+ *    resp. generate helper accessing 32-bit and 64-bit register. "regname"
+ *    been the Arm32 name and "xreg" the Arm64 name.
          ^ is

Please add that we use the Arm64 reg name to call WRITE_SYSREG in the
Xen source code even on Arm32 in general

I am not sure to understand this. It is common use in Xen to use arm64 name when code is for both architecture. So why would I need a specific comment here?

+ *  - UPDATE_REG32_COMBINED(lowreg, hireg, xreg) are used to generate a


+ *  pair of registers share the same Arm32 registers. "lowreg" and
+ *  "higreg" been resp. the Arm32 name and "xreg" the Arm64 name. "lowreg"
+ *  will use xreg[31:0] and "hireg" will use xreg[63:32].

Please add that xreg is unused in the Arm32 case.

Why do you think that? xreg is actually used. It will get expanded to whatever is the co-processor encoding and caught by reg... in TVM_REG().

+ */
+/* The name is passed from the upper macro to workaround macro expansion. */
+#define TVM_REG(sz, func, reg...)                                           \
+static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read)    \
+{                                                                           \
+    GUEST_BUG_ON(read);                                                     \
+    WRITE_SYSREG##sz(*r, reg);                                              \
+                                                                            \
+    return true;                                                            \
+#define TVM_REG32(regname, xreg) TVM_REG(32, vreg_emulate_##regname, xreg)
+#define TVM_REG64(regname, xreg) TVM_REG(64, vreg_emulate_##regname, xreg)
+#ifdef CONFIG_ARM_32
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                     \
+    /* Use TVM_REG directly to workaround macro expansion. */       \
+    TVM_REG(32, vreg_emulate_##lowreg, lowreg)                      \
+    TVM_REG(32, vreg_emulate_##hireg, hireg)
+#else /* CONFIG_ARM_64 */
+#define TVM_REG32_COMBINED(lowreg, hireg, xreg)                             \
+static bool vreg_emulate_##xreg(struct cpu_user_regs *regs, uint32_t *r,    \
+                                bool read, bool hi)                         \
+{                                                                           \
+    register_t reg = READ_SYSREG(xreg);                                     \
+                                                                            \
+    GUEST_BUG_ON(read);                                                     \
+    if ( hi ) /* reg[63:32] is AArch32 register hireg */                    \
+    {                                                                       \
+        reg &= GENMASK(31, 0);                                              \

Move GENMASK before the if? It's the same regardless

Actually, the second GENMASK is incorrect. It should have been GENMASK(63, 32) as we want to update only the lowreg.

So I will fix the mask instead.

+     * HCR_EL2.TVM
+     *
+     * ARMv8 (DDI 0487B.b): Table D1-37

In 0487D.a is D1-99

I haven't had the chance to download the latest spec (it was released last week). I will update to the new spec.


Julien Grall

Xen-devel mailing list



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