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

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

Posted by Juergen Gross 1 year, 10 months ago

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

Posted by Michal Orzel 1 year, 10 months ago


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

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

Posted by Jan Beulich 1 year, 10 months ago

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

Posted by Andrew Cooper 1 year, 10 months ago

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

Posted by Jan Beulich 1 year, 10 months ago

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

Posted by Andrew Cooper 1 year, 10 months ago

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

Posted by Andrew Cooper 1 year, 10 months ago

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>