| Message ID | 20221021153128.44226-4-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm: Enable GICv3 for AArch32 | expand |
Hi Ayan, 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@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? For other options, I would consider to either: 1) Fold vreg_reg_* in the macros. 2) Write a separate vreg_reg64_* My preference would be 1). If we are planning to keep the code with "unsigned long long", then I think this should be addressed in the commit message. > -#if BITS_PER_LONG == 64 > -VREG_REG_HELPERS(64, 0x7); > -#endif > VREG_REG_HELPERS(32, 0x3); > +VREG_REG_HELPERS(64, 0x7); Regardless what I wrote above, the code movement seems to be unwarranted. > > #undef VREG_REG_HELPERS > Cheers,
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@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); 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. :( I had a look at commit 423e9ecdc26c4b40c8db1fcc63b3655463c29976 which introduced this. But I could not make out the reason from the commit message. > > For other options, I would consider to either: > 1) Fold vreg_reg_* in the macros. Can you explain this option a bit ? - Ayan > 2) Write a separate vreg_reg64_* > > My preference would be 1). > > If we are planning to keep the code with "unsigned long long", then I > think this should be addressed in the commit message. > >> -#if BITS_PER_LONG == 64 >> -VREG_REG_HELPERS(64, 0x7); >> -#endif >> VREG_REG_HELPERS(32, 0x3); >> +VREG_REG_HELPERS(64, 0x7); > > Regardless what I wrote above, the code movement seems to be unwarranted. >> #undef VREG_REG_HELPERS > > Cheers, >
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@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,
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@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"? 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, >
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@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"? > > 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,
Hi Julien, > static inline void vreg_reg##sz##_update(uint##sz##_t *reg, > register_t val, > const mmio_info_t *info) > { > uint##sz##_t tmp = *reg; Drop this as we don't use tmp. > uint##sz##_t mask = VREG_REG_MASK(info->dabt.size); > unsigned int offset = info->gap & (offsetmask); int shift = offset * 8; > > *reg &= ~(mask << shift); > *reg |= ((uint##sz##_t)val & mask) << shift; > } I think this is correct. Except for an improvement (as above). Also, we will always keep this defined for both Arm32 and Arm64 VREG_REG_HELPERS(32, 0x3); VREG_REG_HELPERS(64, 0x7); - Ayan
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 - */ -#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
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@amd.com> --- xen/arch/arm/include/asm/vreg.h | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)