Re: xen bits broke x32 build

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/4/23 14:10, David Woodhouse wrote:
> On 1 April 2023 12:57:33 BST, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 01.04.2023 14:45, Joao Martins пишет:
>>> On 01/04/2023 09:40, Michael Tokarev wrote:
>>>> After bringing in xen guest support, qemu fails to build on x32:
>>>> Adding the folks who added the feature too
>>>
>>>> target/i386/kvm/xen-emu.c:876:5: note: in expansion of macro ‘qemu_build_assert’
>>>>     876 |     qemu_build_assert(sizeof(struct vcpu_info) == 64);
>>>>         |     ^~~~~~~~~~~~~~~~~
>>>>
>>>> This one should be easy to fix, but I wonder if there are other issues
>>>> with x32 exists..
>>>>
>>> Not sure.
>>>
>>> struct vcpu_info is supposed to be 64bytes on both 32-bit and 64-bit builds.
>>>
>>> If anything maybe arch_vcpu_info struct is different on 32-bit and 64-bit...
>>> David, Paul, any ideas?
>>
>> Yes, it is arch_vcpu_info. I assumed it is a trivial thing, but let me explain
>> if it's not the case.
>>
>> include/hw/xen/interface/arch-x86/xen.h :
>>
>> #if defined(__i386__)
>> #include "xen-x86_32.h"
>> #elif defined(__x86_64__)
>> #include "xen-x86_64.h"
>> #endif
>>
>> (I wonder if it ever possible to have none of the two defined).
>>
>> Now, xen-x86_32.h has:
>>
>> struct arch_vcpu_info {
>>     unsigned long cr2;
>>     unsigned long pad[5]; /* sizeof(vcpu_info_t) == 64 */
>> };
>>
>> Assuming sizeof(long)==32bits. But actually it is 64bits on x32.
>>
>> While xen-x86_64.h has:
>>
>> struct arch_vcpu_info {
>>     unsigned long cr2;
>>     unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
>> };
>>
>>
>> It looks like for x32, the test in arch-x86/xen.h should be:
>>
>> #if defined(__x86_64__)
>> #include "xen-x86_64.h"
>> #else
>> #include "xen-x86_32.h"
>> #endif
>>
>> since x32 is almost like x86_64. The only difference from x86_64
>> is sizeof(pointer), which is 32bits.
> 
> Hm, doesn't x32 also align uint64_t to 64 bits (unlike i386 which only aligns it to 32 bits)?
> 
>> Well. Maybe xen support should be disabled entirely on x32.
>> Or maybe x32 should be declared as unsupported entirely.
>> I dunno.
> 
> We rely heavily on the struct layouts being precisely the same, since these are ABI for the Xen guests. Wherever there was some 32/64 compatibility issue — and often where there wasn't — that's why I littered it with those build assertions.
> 
> But while there are enough in there to sanity check the i386 vs. x86_64 ABI differences, I wouldn't swear that I've put in enough checks for x32. So "it builds without hitting an assertion" does not necessarily mean it'll be *correct*. Let's disable it on x32.

Does "(unsigned) long" type mix well with "ABI"?