[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v1 03/12] Arm: GICv3: Enable vreg_reg64_* macros for AArch32
On 24/10/2022 13:49, Ayan Kumar Halder wrote:
On 24/10/2022 12:01, Julien Grall wrote:
On 24/10/2022 11:47, Ayan Kumar Halder wrote:
On 22/10/2022 11:13, Julien Grall wrote:
Hi Ayan,
Hi Julien,
I need some clarification.
Title: The code you are modifying below is not GICv3 specific. I
would suggest the following title:
xen/arm: vreg: Support vreg_reg64_* helpers on Aarch32
On 21/10/2022 16:31, Ayan Kumar Halder wrote:
In some situations (eg GICR_TYPER), the hypervior may need to emulate
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, 64 bit is represented by unsigned long long. Thus, we need
to change the prototype accordingly.
Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
---
xen/arch/arm/include/asm/vreg.h | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/include/asm/vreg.h
b/xen/arch/arm/include/asm/vreg.h
index f26a70d024..ac6e702c5c 100644
--- a/xen/arch/arm/include/asm/vreg.h
+++ b/xen/arch/arm/include/asm/vreg.h
@@ -95,7 +95,7 @@ static inline bool vreg_emulate_sysreg(struct
cpu_user_regs *regs, union hsr hsr
* Note that the alignment fault will always be taken in the guest
* (see B3.12.7 DDI0406.b).
*/
-static inline register_t vreg_reg_extract(unsigned long reg,
+static inline register_t vreg_reg_extract(unsigned long long reg,
unsigned int offset,
enum dabt_size size)
{
@@ -105,7 +105,7 @@ static inline register_t
vreg_reg_extract(unsigned long reg,
return reg;
}
-static inline void vreg_reg_update(unsigned long *reg,
register_t val,
+static inline void vreg_reg_update(unsigned long long *reg,
register_t val,
unsigned int offset,
enum dabt_size size)
{
@@ -116,7 +116,7 @@ static inline void vreg_reg_update(unsigned
long *reg, register_t val,
*reg |= ((unsigned long)val & mask) << shift;
}
-static inline void vreg_reg_setbits(unsigned long *reg,
register_t bits,
+static inline void vreg_reg_setbits(unsigned long long *reg,
register_t bits,
unsigned int offset,
enum dabt_size size)
{
@@ -126,7 +126,7 @@ static inline void vreg_reg_setbits(unsigned
long *reg, register_t bits,
*reg |= ((unsigned long)bits & mask) << shift;
}
-static inline void vreg_reg_clearbits(unsigned long *reg,
register_t bits,
+static inline void vreg_reg_clearbits(unsigned long long *reg,
register_t bits,
unsigned int offset,
enum dabt_size size)
{
@@ -149,7 +149,7 @@ static inline void
vreg_reg##sz##_update(uint##sz##_t *reg, \
register_t
val, \
const mmio_info_t
*info) \
{ \
- unsigned long tmp =
*reg; \
+ unsigned long long tmp =
*reg; \
\
vreg_reg_update(&tmp, val, info->gpa &
(offmask), \
info->dabt.size); \
@@ -161,7 +161,7 @@ static inline void
vreg_reg##sz##_setbits(uint##sz##_t *reg, \
register_t
bits, \
const mmio_info_t
*info) \
{ \
- unsigned long tmp =
*reg; \
+ unsigned long long tmp =
*reg; \
\
vreg_reg_setbits(&tmp, bits, info->gpa &
(offmask), \
info->dabt.size); \
@@ -173,7 +173,7 @@ static inline void
vreg_reg##sz##_clearbits(uint##sz##_t *reg, \
register_t
bits, \
const mmio_info_t
*info) \
{ \
- unsigned long tmp =
*reg; \
+ unsigned long long tmp =
*reg; \
\
vreg_reg_clearbits(&tmp, bits, info->gpa &
(offmask), \
info->dabt.size); \
@@ -181,15 +181,8 @@ static inline void
vreg_reg##sz##_clearbits(uint##sz##_t *reg, \
*reg = tmp; \
}
-/*
- * 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
- */
The comment above explain why we never use uint64_t in the helpers
above. IIRC, the compiler would end up to use 2 registers on AArch32
even for the vreg_reg32_* helpers. I wanted to avoid that and would
like like to today. Can you check the code generated?
I am not sure I understood the comment very well.
With this patch, the disassembly is as follows :-
vreg_reg32_update(&v->domain->arch.vgic.ctlr, r, info);
28124c: e597000c ldr r0, [r7, #12]
VREG_REG_HELPERS(32, 0x3);
281250: e5d52002 ldrb r2, [r5, #2]
281254: e1a02322 lsr r2, r2, #6
unsigned long mask = VREG_REG_MASK(size);
Hmmm... Shouldn't this be "unsigned long long"?
The function looks like
Right. My question was why is this still a "unsigned long" with your
patch? If the caller wanted to access the top 32-bit of a 64-bit value...
static inline void vreg_reg_update(unsigned long long *reg, register_t val,
unsigned int offset,
enum dabt_size size)
{
unsigned long mask = VREG_REG_MASK(size);
int shift = offset * 8;
*reg &= ~(mask << shift);
... we would have 'mask << 32' which is AFAIU "undefined" because 'mask'
is 'unsigned long'. Same...
*reg |= ((unsigned long)val & mask) << shift;
... here. The operation would need to be done on 64-bit rather than 32-bit.
For other options, I would consider to either:
1) Fold vreg_reg_* in the macros.
Can you explain this option a bit ?
At the moment, we have generic helpers for vreg_reg_*. They are only
called within the helper generated by VREG_REG_HELPERS().
If we make those helpers size specific, then the only the 64-bit
helpers would use uint64_t local variables.
As they are only called in one place, we could fold them in the
existing helpers.
Just to make sure, I understand this. The code would look like below
#define VREG_REG_HELPERS(type, offmask) \
static inline void vreg_reg_##type##_update(type *reg, register_t val,
\
const mmio_info_t *info) \
{ \
unsigned long mask = VREG_REG_MASK(size); \
unsigned int offset = info->gpa & (offmask); \
int shift = offset * 8; \
*reg &= ~(mask << shift); \
*reg |= ((unsigned long)val & mask) << shift; \
}
This implementation is not correct for 64-bit register. It would need to
look like (untested):
static inline void vreg_reg##sz##_update(uint##sz##_t *reg,
register_t val,
const mmio_info_t *info)
{
uint##sz##_t tmp = *reg;
uint##sz##_t mask = VREG_REG_MASK(info->dabt.size);
unsigned int offset = info->gap & (offsetmask);
*reg &= ~(mask << shift);
*reg |= ((uint##sz##_t)val & mask) << shift;
}
#define vreg_reg_update(reg, val, info) \
do { \
if (sizeof(reg) == 4) \
vreg_reg_uint32_t_update(reg, val, info); \
else if (sizeof(reg) == 8) \
vreg_reg_uint64_t_update(reg, val, info); \
else \
BUG(); \
} while(0); \
After your change above, nobody will call vreg_reg_update(). So no need
to re-implement the function. You can simply drop it.
Similar implementation will be for vreg_reg_clearbits(),
vreg_reg_setbits() and vreg_reg_extract()
VREG_REG_HELPERS(uint32_t, 0x3);
VREG_REG_HELPERS(uint64_t, 0x7);
And the functions would be invoked as follows :-
vreg_update(&priority, r, info);
The code should use vreg_reg<sz>_update() rather than the generic one.
At least it will be clear from the caller which size is expected.
Cheers,
--
Julien Grall
|
