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

Re: [PATCH v2] xen/arm64: Remove READ/WRITE_SYSREG32 helper macros

On 12/07/2021 07:26, Michal Orzel wrote:
Hi Julien,

Hi Michal,

On 09.07.2021 17:21, Julien Grall wrote:
Hi Michal,

On 09/07/2021 13:40, 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.

The last place in code making use of READ/WRITE_SYSREG32
on arm64 is in TVM_REG macro defining functions vreg_emulate_<register>.
Implement a macro WRITE_SYSREG_SZ which expands as follows:
-on arm64: WRITE_SYSREG
-on arm32: WRITE_SYSREG{32/64}

As there are no other places in the code using these helpers
on arm64 - remove them.

Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
Changes since v1:
-implement WRITE_SYSREG_SZ instead of duplicating the TVM_REG
   xen/arch/arm/vcpreg.c               | 12 +++++++++++-
   xen/include/asm-arm/arm64/sysregs.h |  4 ----
   2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index f0cdcc8a54..10c4846954 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -47,6 +47,16 @@
   +#ifdef CONFIG_ARM_64
+#define WRITE_SYSREG_SZ(sz, val, sysreg)     WRITE_SYSREG(val, sysreg)

I think you want to cast to (uint##sz##_t) to avoid any surprise. I think...

But the val will always be of type uint##sz##_t because it is passed from ...
+ * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes
+ * on the below macro to be defined like that as well.
+ */
+#define WRITE_SYSREG_SZ(sz, val, sysreg...)  WRITE_SYSREG##sz(val, sysreg)
   /* 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)    \
@@ -55,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool 
read)    \
... here(*r argument).
So I do not think that such casting makes sense e.g
uint##sz##_t foo;
WRITE_SYSREG((uint##sz##_t)foo, bar);

I agree that this doesn't make sense for the *current* use. However, when writing code, we need to think how one could use it in the future...

When I read the name, I would expect that if I call it with sz == 32, then bottom 32-bit value to be written and the top 32-bit zeroed. But this is not the case...

You are relying on the developper and reviewer to notice that only 32-bit value should be passed when sz == 32.

I am not particularly keen on relying on this when a simple cast could prevent any future misuse. Alternatively, I would be happy to consider checking the type of the value at build time.


Julien Grall



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