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 <ayank...@amd.com>
---
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"?
281258: e3a03008 mov r3, #8
28125c: e1a03213 lsl r3, r3, r2
281260: e2633020 rsb r3, r3, #32
281264: e3e02000 mvn r2, #0
281268: e1a02332 lsr r2, r2, r3
VREG_REG_HELPERS(32, 0x3);
28126c: e5953010 ldr r3, [r5, #16]
281270: e2033003 and r3, r3, #3
int shift = offset * 8;
281274: e1a03183 lsl r3, r3, #3
VREG_REG_HELPERS(32, 0x3);
281278: e59013f0 ldr r1, [r0, #1008] ; 0x3f0
28127c: e1c11312 bic r1, r1, r2, lsl r3
*reg |= ((unsigned long)val & mask) << shift;
281280: e0022009 and r2, r2, r9
VREG_REG_HELPERS(32, 0x3);
281284: e1813312 orr r3, r1, r2, lsl r3
281288: e58033f0 str r3, [r0, #1008] ; 0x3f0
v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
28128c: e597200c ldr r2, [r7, #12]
281290: e59233f0 ldr r3, [r2, #1008] ; 0x3f0
281294: e2033001 and r3, r3, #1
281298: e58233f0 str r3, [r2, #1008] ; 0x3f0
Without the patch (ie original code) , the disassembly is :-
vreg_reg32_update(&v->domain->arch.vgic.ctlr, r, info);
27f8f4: e597000c ldr r0, [r7, #12]
VREG_REG_HELPERS(32, 0x3);
27f8f8: e5d52002 ldrb r2, [r5, #2]
27f8fc: e1a02322 lsr r2, r2, #6
unsigned long mask = VREG_REG_MASK(size);
27f900: e3a03008 mov r3, #8
27f904: e1a03213 lsl r3, r3, r2
27f908: e2633020 rsb r3, r3, #32
27f90c: e3e02000 mvn r2, #0
27f910: e1a02332 lsr r2, r2, r3
VREG_REG_HELPERS(32, 0x3);
27f914: e5953010 ldr r3, [r5, #16]
27f918: e2033003 and r3, r3, #3
int shift = offset * 8;
27f91c: e1a03183 lsl r3, r3, #3
*reg &= ~(mask << shift);
27f920: e5901400 ldr r1, [r0, #1024] ; 0x400
27f924: e1c11312 bic r1, r1, r2, lsl r3
*reg |= ((unsigned long)val & mask) << shift;
27f928: e0022009 and r2, r2, r9
27f92c: e1813312 orr r3, r1, r2, lsl r3
VREG_REG_HELPERS(32, 0x3);
27f930: e5803400 str r3, [r0, #1024] ; 0x400
v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
27f934: e597200c ldr r2, [r7, #12]
27f938: e5923400 ldr r3, [r2, #1024] ; 0x400
27f93c: e2033001 and r3, r3, #1
27f940: e5823400 str r3, [r2, #1024] ; 0x400
Sorry, I can't spot the difference. :(
If there is no difference, then it is a good sign. I was worried that
the compiler would end up to use "strd/ldrd" which would result to more
register allocations and therefore inefficient code.
But see above.
I had a look at commit 423e9ecdc26c4b40c8db1fcc63b3655463c29976 which
introduced this. But I could not make out the reason from the commit
message.
The reasoning would be to show that the assembly is either the same or
no worse that then existing one with a few compilers.
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.
Cheers,
--
Julien Grall
