[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 ...+#else +/* + * 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) +#endif + /* 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. Cheers, -- Julien Grall
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |