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

[Jan Beulich <jbeulich@xxxxxxxx>]

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