[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN v2 03/12] xen/Arm: vreg: Support vreg_reg64_* helpers on Aarch32
Hi,
On 31/10/2022 15:13, Ayan Kumar Halder wrote:
In some situations (eg GICR_TYPER), the hypervior may need to emulate
Typo: s/eg/e.g./
64bit registers in aarch32 mode. In such situations, the hypervisor may
need to read/modify the lower or upper 32 bits of the 64 bit register.
In aarch32, 'unsigned long' is 32 bits. Thus, we cannot use it for 64 bit
registers.
The correct approach is to typecast 'mask' based on the size of register access
(ie uint32_t or uint64_t) instead of using 'unsigned long' as it will not
generate the correct mask for the upper 32 bits of a 64 bit register.
Also, 'val' needs to be typecasted so that it can correctly update the upper/
'val' doesn't exist everywhere. But rather than explaining variable by
variable what needs to be done, I would suggest something like the
following:
"While we could replace 'unsigned long' by 'uint64_t', it is not
entirely clear whether a 32-bit compiler would not allocate register for
the upper 32-bit. Therefore fold vreg_reg_* helper in the size specific
one and use the appropriate type based on the size requested."
This would also explain why we didn't do a straight replacement of
"unsigned long" to "uint64_t".
[...]
/* N-bit register helpers */
#define VREG_REG_HELPERS(sz, offmask) \
static inline register_t vreg_reg##sz##_extract(uint##sz##_t reg, \
const mmio_info_t *info)\
{ \
- return vreg_reg_extract(reg, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ \
+ reg >>= 8 * offset; \
+ reg &= VREG_REG_MASK(info->dabt.size); \
+ \
+ return reg; \
} \
\
static inline void vreg_reg##sz##_update(uint##sz##_t *reg, \
register_t val, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
- \
- vreg_reg_update(&tmp, val, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ int shift = offset * 8; \
As you fold it, please switch to "unsigned int".
\
- *reg = tmp; \
+ *reg &= ~(mask << shift); \
+ *reg |= ((uint##sz##_t)val & mask) << shift; \
} \
\
static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg, \
register_t bits, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
- \
- vreg_reg_setbits(&tmp, bits, info->gpa & (offmask), \
- info->dabt.size); \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ int shift = offset * 8; \
Same here.
\
- *reg = tmp; \
+ *reg |= ((uint##sz##_t)bits & mask) << shift; \
} \
\
static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg, \
register_t bits, \
const mmio_info_t *info) \
{ \
- unsigned long tmp = *reg; \
+ unsigned int offset = info->gpa & (offmask); \
+ uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); \
+ int shift = offset * 8; \
Same here.
\
- vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask), \
- info->dabt.size); \
- \
- *reg = tmp; \
+ *reg &= ~(((uint##sz##_t)bits & mask) << shift); \
}
-/*
- * 64 bits registers are only supported on platform with 64-bit long.
- * This is also allow us to optimize the 32 bit case by using
- * unsigned long rather than uint64_t
- */
-#if BITS_PER_LONG == 64
-VREG_REG_HELPERS(64, 0x7);
-#endif
VREG_REG_HELPERS(32, 0x3);
+VREG_REG_HELPERS(64, 0x7);
#undef VREG_REG_HELPERS
Cheers,
--
Julien Grall
|
