Mailing List Archive: [PATCH] xen/include: move definition of ASM

[PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h

Apr 3, 2024, 5:03 AM

Post #1 of 7 (55 views)

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(-)

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

--
2.35.3

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

andrew.cooper3 at citrix

Apr 3, 2024, 5:20 AM

Post #2 of 7 (55 views)

Permalink

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>

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

jbeulich at suse

Apr 3, 2024, 5:51 AM

Post #3 of 7 (55 views)

Permalink

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.?.

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

andrew.cooper3 at citrix

Apr 3, 2024, 6:59 AM

Post #4 of 7 (55 views)

Permalink

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

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

jbeulich at suse

Apr 3, 2024, 7:22 AM

Post #5 of 7 (55 views)

Permalink

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

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

andrew.cooper3 at citrix

Apr 11, 2024, 5:29 AM

Post #6 of 7 (27 views)

Permalink

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

Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h [ In reply to ]

michal.orzel at amd

Apr 11, 2024, 6:33 AM

Post #7 of 7 (27 views)

Permalink

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