| Message ID | 20240403120323.18433-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/include: move definition of ASM_INT() to xen/linkage.h | expand |
On 03/04/2024 1:03 pm, Juergen Gross wrote: > ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in > exactly the same way. Instead of replicating this definition for riscv > and ppc, move it to include/xen/linkage.h, where other arch agnostic > definitions for assembler code are living already. > > Adapt the generation of assembler sources via tools/binfile to include > the new home of ASM_INT(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 03.04.2024 14:03, Juergen Gross wrote: > ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in > exactly the same way. Instead of replicating this definition for riscv > and ppc, move it to include/xen/linkage.h, where other arch agnostic > definitions for assembler code are living already. And this is why I didn't make a change right away, back when noticing the duplication: Arch-agnostic really means ... > --- a/xen/include/xen/linkage.h > +++ b/xen/include/xen/linkage.h > @@ -60,6 +60,8 @@ > #define DATA_LOCAL(name, align...) \ > SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) > > +#define ASM_INT(label, val) DATA(label, 4) .long (val); END(label) ... to avoid .long [1]. There's no arch-independent aspect guaranteeing that what .long emits matches "unsigned int" as used e.g. in the declaration of xen_config_data_size. The arch-agnostic directives are .dc.l and friends. Sadly Clang looks to support this only starting with version 4. Nevertheless, seeing that Andrew ack-ed the change already, it's perhaps good enough for the moment. If an obscure port appeared, the further abstraction could be taken care of by them. Jan [1] Note how in gas doc .long refers to .int, .int says "32-bit", just to then have a special case of H8/300 emitting 16-bit values. Things must have been confusing enough for someone to come and add .dc.?.
On 03/04/2024 1:51 pm, Jan Beulich wrote: > On 03.04.2024 14:03, Juergen Gross wrote: >> ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in >> exactly the same way. Instead of replicating this definition for riscv >> and ppc, move it to include/xen/linkage.h, where other arch agnostic >> definitions for assembler code are living already. > And this is why I didn't make a change right away, back when noticing the > duplication: Arch-agnostic really means ... > >> --- a/xen/include/xen/linkage.h >> +++ b/xen/include/xen/linkage.h >> @@ -60,6 +60,8 @@ >> #define DATA_LOCAL(name, align...) \ >> SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) >> >> +#define ASM_INT(label, val) DATA(label, 4) .long (val); END(label) > ... to avoid .long [1]. There's no arch-independent aspect guaranteeing > that what .long emits matches "unsigned int" as used e.g. in the > declaration of xen_config_data_size. I'd forgotten that point, but I don't think it's a good reason force every architecture to implement the same thing. Borrowing a trick from the alternatives, what about this as a sanity check? diff --git a/xen/tools/binfile b/xen/tools/binfile index 0299326ccc3f..21593debc872 100755 --- a/xen/tools/binfile +++ b/xen/tools/binfile @@ -35,4 +35,10 @@ DATA($varname, 1 << $align) END($varname) ASM_INT(${varname}_size, .Lend - $varname) +.Lsize_end: + + .section .discard + # Build assert sizeof(ASM_INT) == 4 + .byte 0xff - ((.Lsize_end - ${varname}_size) == 4) + EOF Ideally we'd want BYTES_PER_INT here but it turns out that doesn't exist in Xen. If we find an architecture where .long isn't the right thing, we can make ASM_INT optionally arch-specific. ~Andrew
On 03.04.2024 15:59, Andrew Cooper wrote: > On 03/04/2024 1:51 pm, Jan Beulich wrote: >> On 03.04.2024 14:03, Juergen Gross wrote: >>> ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in >>> exactly the same way. Instead of replicating this definition for riscv >>> and ppc, move it to include/xen/linkage.h, where other arch agnostic >>> definitions for assembler code are living already. >> And this is why I didn't make a change right away, back when noticing the >> duplication: Arch-agnostic really means ... >> >>> --- a/xen/include/xen/linkage.h >>> +++ b/xen/include/xen/linkage.h >>> @@ -60,6 +60,8 @@ >>> #define DATA_LOCAL(name, align...) \ >>> SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) >>> >>> +#define ASM_INT(label, val) DATA(label, 4) .long (val); END(label) >> ... to avoid .long [1]. There's no arch-independent aspect guaranteeing >> that what .long emits matches "unsigned int" as used e.g. in the >> declaration of xen_config_data_size. > > I'd forgotten that point, but I don't think it's a good reason force > every architecture to implement the same thing. Of course. > Borrowing a trick from the alternatives, what about this as a sanity check? > > diff --git a/xen/tools/binfile b/xen/tools/binfile > index 0299326ccc3f..21593debc872 100755 > --- a/xen/tools/binfile > +++ b/xen/tools/binfile > @@ -35,4 +35,10 @@ DATA($varname, 1 << $align) > END($varname) > > ASM_INT(${varname}_size, .Lend - $varname) > +.Lsize_end: > + > + .section .discard > + # Build assert sizeof(ASM_INT) == 4 > + .byte 0xff - ((.Lsize_end - ${varname}_size) == 4) > + > EOF Hmm, tools/binfile may not be involved in a build, yet ASM_INT() may still be used. Since there may not be any good place, I think we're okay-ish for now without such a check. > Ideally we'd want BYTES_PER_INT here but it turns out that doesn't exist > in Xen. If we find an architecture where .long isn't the right thing, > we can make ASM_INT optionally arch-specific. We don't even need to go this far - merely introducing an abstraction for .long would suffice, and then also allow using that in bug.h. Jan
On 03/04/2024 3:22 pm, Jan Beulich wrote: > On 03.04.2024 15:59, Andrew Cooper wrote: >> On 03/04/2024 1:51 pm, Jan Beulich wrote: >>> On 03.04.2024 14:03, Juergen Gross wrote: >>>> ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in >>>> exactly the same way. Instead of replicating this definition for riscv >>>> and ppc, move it to include/xen/linkage.h, where other arch agnostic >>>> definitions for assembler code are living already. >>> And this is why I didn't make a change right away, back when noticing the >>> duplication: Arch-agnostic really means ... >>> >>>> --- a/xen/include/xen/linkage.h >>>> +++ b/xen/include/xen/linkage.h >>>> @@ -60,6 +60,8 @@ >>>> #define DATA_LOCAL(name, align...) \ >>>> SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) >>>> >>>> +#define ASM_INT(label, val) DATA(label, 4) .long (val); END(label) >>> ... to avoid .long [1]. There's no arch-independent aspect guaranteeing >>> that what .long emits matches "unsigned int" as used e.g. in the >>> declaration of xen_config_data_size. >> I'd forgotten that point, but I don't think it's a good reason force >> every architecture to implement the same thing. > Of course. > >> Borrowing a trick from the alternatives, what about this as a sanity check? >> >> diff --git a/xen/tools/binfile b/xen/tools/binfile >> index 0299326ccc3f..21593debc872 100755 >> --- a/xen/tools/binfile >> +++ b/xen/tools/binfile >> @@ -35,4 +35,10 @@ DATA($varname, 1 << $align) >> END($varname) >> >> ASM_INT(${varname}_size, .Lend - $varname) >> +.Lsize_end: >> + >> + .section .discard >> + # Build assert sizeof(ASM_INT) == 4 >> + .byte 0xff - ((.Lsize_end - ${varname}_size) == 4) >> + >> EOF > Hmm, tools/binfile may not be involved in a build, yet ASM_INT() may > still be used. Since there may not be any good place, I think we're > okay-ish for now without such a check. > >> Ideally we'd want BYTES_PER_INT here but it turns out that doesn't exist >> in Xen. If we find an architecture where .long isn't the right thing, >> we can make ASM_INT optionally arch-specific. > We don't even need to go this far - merely introducing an abstraction > for .long would suffice, and then also allow using that in bug.h. Ok. I'll take this patch as-is for now. We can abstract away .long later. ~Andrew
On 03/04/2024 14:03, Juergen Gross wrote: > > > ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in > exactly the same way. Instead of replicating this definition for riscv > and ppc, move it to include/xen/linkage.h, where other arch agnostic > definitions for assembler code are living already. > > Adapt the generation of assembler sources via tools/binfile to include > the new home of ASM_INT(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Acked-by: Michal Orzel <michal.orzel@amd.com> ~Michal
diff --git a/xen/arch/arm/include/asm/asm_defns.h b/xen/arch/arm/include/asm/asm_defns.h index c489547d29..47efdf5234 100644 --- a/xen/arch/arm/include/asm/asm_defns.h +++ b/xen/arch/arm/include/asm/asm_defns.h @@ -28,9 +28,6 @@ label: .asciz msg; \ .popsection -#define ASM_INT(label, val) \ - DATA(label, 4) .long (val); END(label) - #endif /* __ARM_ASM_DEFNS_H__ */ /* * Local variables: diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h index a69fae78b1..0a3ff70566 100644 --- a/xen/arch/x86/include/asm/asm_defns.h +++ b/xen/arch/x86/include/asm/asm_defns.h @@ -351,9 +351,6 @@ static always_inline void stac(void) 4: .p2align 2 ; \ .popsection -#define ASM_INT(label, val) \ - DATA(label, 4) .long (val); END(label) - #define ASM_CONSTANT(name, value) \ asm ( ".equ " #name ", %P0; .global " #name \ :: "i" ((value)) ); diff --git a/xen/include/xen/linkage.h b/xen/include/xen/linkage.h index 478b1d7287..3d401b88c1 100644 --- a/xen/include/xen/linkage.h +++ b/xen/include/xen/linkage.h @@ -60,6 +60,8 @@ #define DATA_LOCAL(name, align...) \ SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) +#define ASM_INT(label, val) DATA(label, 4) .long (val); END(label) + #endif /* __ASSEMBLY__ */ #endif /* __LINKAGE_H__ */ diff --git a/xen/tools/binfile b/xen/tools/binfile index 099d7eda9a..0299326ccc 100755 --- a/xen/tools/binfile +++ b/xen/tools/binfile @@ -25,7 +25,7 @@ binsource=$2 varname=$3 cat <<EOF >$target -#include <asm/asm_defns.h> +#include <xen/linkage.h> .section $section.rodata, "a", %progbits
ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in exactly the same way. Instead of replicating this definition for riscv and ppc, move it to include/xen/linkage.h, where other arch agnostic definitions for assembler code are living already. Adapt the generation of assembler sources via tools/binfile to include the new home of ASM_INT(). Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/arch/arm/include/asm/asm_defns.h | 3 --- xen/arch/x86/include/asm/asm_defns.h | 3 --- xen/include/xen/linkage.h | 2 ++ xen/tools/binfile | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-)