| Message ID | 20240130222808.106006-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/*/asm-offset: Fix bad copy&paste from x86 | expand |
Hi Andrew, On 1/30/24 4:28 PM, Andrew Cooper wrote: > All architectures have copy&pasted bad logic from x86. > > OFFSET() having a trailing semi-colon within the macro expansion can be a > problematic pattern. It's benign in this case, but fix it anyway. > > Perform style fixes for the other macros, and tame the mess of BLANK() > position to be consistent (one BLANK() after each block) so the intermediate > form is legible too. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Michal Orzel <michal.orzel@amd.com> > CC: Bob Eshleman <bobbyeshleman@gmail.com> > CC: Alistair Francis <alistair.francis@wdc.com> > CC: Connor Davis <connojdavis@gmail.com> > CC: Shawn Anastasio <sanastasio@raptorengineering.com> > > Why does PPC have a local copy of ilog2() here? Especially as it's not used, > and there's a perfectly good one in <xen/bitops.h> I believe this was a carryover from before we had asm/bitops.h implemented on PPC. Now that we have bitops implemented (and we don't define any offsets that use log2 anymore anyways), this is fine to remove. Will submit a patch to do so. As for the actual contents of this patch, Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com> Thanks, Shawn
On 30/01/2024 11:00 pm, Shawn Anastasio wrote: > Hi Andrew, > > On 1/30/24 4:28 PM, Andrew Cooper wrote: >> All architectures have copy&pasted bad logic from x86. >> >> OFFSET() having a trailing semi-colon within the macro expansion can be a >> problematic pattern. It's benign in this case, but fix it anyway. >> >> Perform style fixes for the other macros, and tame the mess of BLANK() >> position to be consistent (one BLANK() after each block) so the intermediate >> form is legible too. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: Bertrand Marquis <bertrand.marquis@arm.com> >> CC: Michal Orzel <michal.orzel@amd.com> >> CC: Bob Eshleman <bobbyeshleman@gmail.com> >> CC: Alistair Francis <alistair.francis@wdc.com> >> CC: Connor Davis <connojdavis@gmail.com> >> CC: Shawn Anastasio <sanastasio@raptorengineering.com> >> >> Why does PPC have a local copy of ilog2() here? Especially as it's not used, >> and there's a perfectly good one in <xen/bitops.h> > I believe this was a carryover from before we had asm/bitops.h > implemented on PPC. Now that we have bitops implemented (and we don't > define any offsets that use log2 anymore anyways), this is fine to > remove. Will submit a patch to do so. Ah - good to know. Thanks. > As for the actual contents of this patch, > > Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com> and thanks also. ~Andrew
On 30.01.2024 23:28, Andrew Cooper wrote: > --- a/xen/arch/x86/x86_64/asm-offsets.c > +++ b/xen/arch/x86/x86_64/asm-offsets.c > @@ -18,11 +18,11 @@ > > #define DEFINE(_sym, _val) \ > asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ > - : : "i" (_val) ) > + :: "i" (_val)) The removal of the last blank is against our style; instead a blank wants insertion after the opening parenthesis. > #define BLANK() \ > - asm volatile ( "\n.ascii\"==><==\"" : : ) > + asm volatile ("\n.ascii\"==><==\"") Similarly here while dropping the colons is fine, the blanks next to the parentheses want keeping. With that adjusted throughout Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Andrew, On 30/01/2024 22:28, Andrew Cooper wrote: > All architectures have copy&pasted bad logic from x86. > > OFFSET() having a trailing semi-colon within the macro expansion can be a > problematic pattern. It's benign in this case, but fix it anyway. > > Perform style fixes for the other macros, and tame the mess of BLANK() > position to be consistent (one BLANK() after each block) so the intermediate > form is legible too. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> With Jan's comments addressed on the Arm side as well: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c index 05c692bb2822..f19523d741af 100644 --- a/xen/arch/arm/arm32/asm-offsets.c +++ b/xen/arch/arm/arm32/asm-offsets.c @@ -15,11 +15,11 @@ #define DEFINE(_sym, _val) \ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ - : : "i" (_val) ) + :: "i" (_val)) #define BLANK() \ - asm volatile ( "\n.ascii\"==><==\"" : : ) + asm volatile ("\n.ascii\"==><==\"") #define OFFSET(_sym, _str, _mem) \ - DEFINE(_sym, offsetof(_str, _mem)); + DEFINE(_sym, offsetof(_str, _mem)) void __dummy__(void) { @@ -62,18 +62,19 @@ void __dummy__(void) BLANK(); DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info)); + BLANK(); OFFSET(VCPU_arch_saved_context, struct vcpu, arch.saved_context); - BLANK(); + DEFINE(PROCINFO_sizeof, sizeof(struct proc_info_list)); OFFSET(PROCINFO_cpu_val, struct proc_info_list, cpu_val); OFFSET(PROCINFO_cpu_mask, struct proc_info_list, cpu_mask); OFFSET(PROCINFO_cpu_init, struct proc_info_list, cpu_init); - BLANK(); - OFFSET(INITINFO_stack, struct init_info, stack); + OFFSET(INITINFO_stack, struct init_info, stack); + BLANK(); } /* diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c index 7adb67a1b81a..8112b9243d39 100644 --- a/xen/arch/arm/arm64/asm-offsets.c +++ b/xen/arch/arm/arm64/asm-offsets.c @@ -15,11 +15,11 @@ #define DEFINE(_sym, _val) \ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ - : : "i" (_val) ) + :: "i" (_val)) #define BLANK() \ - asm volatile ( "\n.ascii\"==><==\"" : : ) + asm volatile ("\n.ascii\"==><==\"") #define OFFSET(_sym, _str, _mem) \ - DEFINE(_sym, offsetof(_str, _mem)); + DEFINE(_sym, offsetof(_str, _mem)) void __dummy__(void) { @@ -48,13 +48,14 @@ void __dummy__(void) DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info)); OFFSET(CPUINFO_flags, struct cpu_info, flags); + BLANK(); OFFSET(VCPU_arch_saved_context, struct vcpu, arch.saved_context); - BLANK(); - OFFSET(INITINFO_stack, struct init_info, stack); + OFFSET(INITINFO_stack, struct init_info, stack); BLANK(); + OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0); OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2); OFFSET(ARM_SMCCC_1_2_REGS_X0_OFFS, struct arm_smccc_1_2_regs, a0); @@ -66,6 +67,7 @@ void __dummy__(void) OFFSET(ARM_SMCCC_1_2_REGS_X12_OFFS, struct arm_smccc_1_2_regs, a12); OFFSET(ARM_SMCCC_1_2_REGS_X14_OFFS, struct arm_smccc_1_2_regs, a14); OFFSET(ARM_SMCCC_1_2_REGS_X16_OFFS, struct arm_smccc_1_2_regs, a16); + BLANK(); } /* diff --git a/xen/arch/ppc/ppc64/asm-offsets.c b/xen/arch/ppc/ppc64/asm-offsets.c index 634d7260e3a4..24065578cbdb 100644 --- a/xen/arch/ppc/ppc64/asm-offsets.c +++ b/xen/arch/ppc/ppc64/asm-offsets.c @@ -9,12 +9,12 @@ #include <asm/boot.h> #define DEFINE(_sym, _val) \ - asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ - : : "i" (_val) ) + asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ + : : "i" (_val)) #define BLANK() \ - asm volatile ( "\n.ascii\"==><==\"" : : ) + asm volatile ("\n.ascii\"==><==\"") #define OFFSET(_sym, _str, _mem) \ - DEFINE(_sym, offsetof(_str, _mem)); + DEFINE(_sym, offsetof(_str, _mem)) /* base-2 logarithm */ #define __L2(_x) (((_x) & 0x00000002) ? 1 : 0) @@ -29,6 +29,7 @@ void __dummy__(void) DEFINE(GPR_WIDTH, sizeof(unsigned long)); DEFINE(FPR_WIDTH, sizeof(double)); + BLANK(); OFFSET(UREGS_gprs, struct cpu_user_regs, gprs); OFFSET(UREGS_r0, struct cpu_user_regs, gprs[0]); @@ -48,9 +49,11 @@ void __dummy__(void) OFFSET(UREGS_fpscr, struct cpu_user_regs, fpscr); OFFSET(UREGS_entry_vector, struct cpu_user_regs, entry_vector); DEFINE(UREGS_sizeof, sizeof(struct cpu_user_regs)); + BLANK(); OFFSET(OPAL_base, struct opal, base); OFFSET(OPAL_entry, struct opal, entry); + BLANK(); } /* diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c index d632b75c2acb..ed74b11bf654 100644 --- a/xen/arch/riscv/riscv64/asm-offsets.c +++ b/xen/arch/riscv/riscv64/asm-offsets.c @@ -5,15 +5,14 @@ #define DEFINE(_sym, _val) \ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ - : : "i" (_val) ) + :: "i" (_val)) #define BLANK() \ - asm volatile ( "\n.ascii\"==><==\"" : : ) + asm volatile ("\n.ascii\"==><==\"") #define OFFSET(_sym, _str, _mem) \ - DEFINE(_sym, offsetof(_str, _mem)); + DEFINE(_sym, offsetof(_str, _mem)) void asm_offsets(void) { - BLANK(); DEFINE(CPU_USER_REGS_SIZE, sizeof(struct cpu_user_regs)); OFFSET(CPU_USER_REGS_ZERO, struct cpu_user_regs, zero); OFFSET(CPU_USER_REGS_RA, struct cpu_user_regs, ra); @@ -50,4 +49,5 @@ void asm_offsets(void) OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc); OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus); OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs); + BLANK(); } diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index f9546ec60b60..a4931c4f7eb2 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -18,11 +18,11 @@ #define DEFINE(_sym, _val) \ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ - : : "i" (_val) ) + :: "i" (_val)) #define BLANK() \ - asm volatile ( "\n.ascii\"==><==\"" : : ) + asm volatile ("\n.ascii\"==><==\"") #define OFFSET(_sym, _str, _mem) \ - DEFINE(_sym, offsetof(_str, _mem)); + DEFINE(_sym, offsetof(_str, _mem)) void __dummy__(void) { @@ -180,4 +180,5 @@ void __dummy__(void) BLANK(); OFFSET(DOMAIN_vm_assist, struct domain, vm_assist); + BLANK(); }
All architectures have copy&pasted bad logic from x86. OFFSET() having a trailing semi-colon within the macro expansion can be a problematic pattern. It's benign in this case, but fix it anyway. Perform style fixes for the other macros, and tame the mess of BLANK() position to be consistent (one BLANK() after each block) so the intermediate form is legible too. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Bob Eshleman <bobbyeshleman@gmail.com> CC: Alistair Francis <alistair.francis@wdc.com> CC: Connor Davis <connojdavis@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> Why does PPC have a local copy of ilog2() here? Especially as it's not used, and there's a perfectly good one in <xen/bitops.h> --- xen/arch/arm/arm32/asm-offsets.c | 13 +++++++------ xen/arch/arm/arm64/asm-offsets.c | 12 +++++++----- xen/arch/ppc/ppc64/asm-offsets.c | 11 +++++++---- xen/arch/riscv/riscv64/asm-offsets.c | 8 ++++---- xen/arch/x86/x86_64/asm-offsets.c | 7 ++++--- 5 files changed, 29 insertions(+), 22 deletions(-)