On 27/04/2021 08:16, Michal Orzel wrote:
Hi Jilien,
Hi,
On 21.04.2021 21:16, Julien Grall wrote:
Hi Michal,
On 20/04/2021 08:08, Michal Orzel wrote:
AArch64 system registers are 64bit whereas AArch32 ones
are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
we should get rid of helpers READ/WRITE_SYSREG32
in favour of using READ/WRITE_SYSREG.
We should also use register_t type when reading sysregs
which can correspond to uint64_t or uint32_t.
Even though many AArch64 sysregs have upper 32bit reserved
it does not mean that they can't be widen in the future.
As a general comment, all your commit message contains information about the
goal (which is great). But they don't go much in details about the actual
changes. I have tried to point out where I think those details would be helpful.
As there are no other places in the code using READ/WRITE_SYSREG32,
remove the helper macros.
Signed-off-by: Michal Orzel <michal.or...@arm.com>
---
xen/arch/arm/vcpreg.c | 16 ++++++++++++++++
xen/include/asm-arm/arm64/sysregs.h | 5 -----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index c7f516ee0a..6cb7f3108c 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -48,6 +48,7 @@
*/
/* The name is passed from the upper macro to workaround macro expansion.
*/
+#ifdef CONFIG_ARM_32
#define TVM_REG(sz, func, reg...) \
static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
{ \
@@ -61,6 +62,21 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t
*r, bool read) \
\
return true; \
}
+#else /* CONFIG_ARM_64 */
+#define TVM_REG(sz, func, reg...) \
+static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read) \
+{ \
+ struct vcpu *v = current; \
+ bool cache_enabled = vcpu_has_cache_enabled(v); \
+ \
+ GUEST_BUG_ON(read); \
+ WRITE_SYSREG(*r, reg); \
+ \
+ p2m_toggle_cache(v, cache_enabled); \
+ \
+ return true; \
+}
+#endif
It would be nice if this change can be explained in the commit message.
However, I think we can avoid the duplication by aliasing TVM_REG32() to
TVM_REG64() on Arm64.
Unfortunately we cannot. This is the only working solution for now.
If we do the alias we will get many errors due to incompatbile pointer type in
vreg_emulate_*.
Without a proper change in this functions we can't do that.
Right, so the prototype needs to stay the same. How about provide a
wrapper WRITE_SYSREG_SZ(sz, val, sysreg) internal to vcpreg.c?
On 32-bit it would expand to WRITE_SYSREG##sz(val, sysreg) and on
64-bit, it would expand to WRITE_SYSREG().
I will modify it once I start working on vreg_emulate topic but it requires
more investigation.
While it would be great to get rid of {READ, WRITE}_SYSREG32, I don't
want to duplicate the code (even temporarily) just for the sake for
removing the helpers.
If the new proposal I suggested above doesn't work, then I would
strongly prefer to keep the existing code until vreg_emulate_* is reworked.
Cheers,
--
Julien Grall
