|
[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 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 emulate64bit registers in aarch32 mode. In such situations, the hypervisor mayneed 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.hindex 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
The function looks like
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);
*reg |= ((unsigned long)val & mask) << shift;
}
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. 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; \
}
#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); \
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); Is this the correct understanding ? Cheers,
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |