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
