xen bits broke x32 build

xen bits broke x32 build

[Michael Tokarev]

After bringing in xen guest support, qemu fails to build on x32:

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

Thanks,

/mjt

Re: xen bits broke x32 build

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

Re: xen bits broke x32 build

[Michael Tokarev]

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.


Well. Maybe xen support should be disabled entirely on x32.
Or maybe x32 should be declared as unsupported entirely.
I dunno.

/mjt

Re: xen bits broke x32 build

[Michael Tokarev]

To clarify: I don't think this needs fixing for 8.0.

Yes, it's a regression on x32.  Xen has never worked on x32,
this just looks like forgotten case when it needs to be disabled
in qemu for x86. It's trivial to work around by adding a --disable
argument to configure.  And generally I don't know anyone actually
running qemu on x32. It's some exotic thing which apparently is
going away by its own.

Thanks,

/mjt

Re: xen bits broke x32 build

[David Woodhouse]

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.

Re: xen bits broke x32 build

[Philippe Mathieu-Daudé]

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

Re: xen bits broke x32 build

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

On Mon, 2023-04-03 at 08:46 +0200, Philippe Mathieu-Daudé wrote:
> > > 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"?

No, but this is Xen. The i386 and x86_64 ABIs are different.

My favourite Xen ABI for this is the vcpu_runstate_info:

struct vcpu_runstate_info {
    int state;
    uint64_t state_entry_time;
    uint64_t time[4];
};

That's different between i386 and x86_64 not because of any of the
sizes, but because of the *alignment* differences. Giving us four bytes
of padding between 'state' and 'state_entry_time' in the x86_64 (and
x32) version, and this for i386 compatibility mode:

struct compat_vcpu_runstate_info {
    int state;
    uint64_t state_entry_time;
    uint64_t time[4];
} __attribute__((packed));

I suspect that an x32 build could probably get away with supporting a
32-bit guest, but 64-bit guest support would be entirely hosed. And
it's the *compat* structures I've put the sanity checks on, not the
native ones. 

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: xen bits broke x32 build

[Michael Tokarev]

01.04.2023 11:40, Michael Tokarev wrote:
> After bringing in xen guest support, qemu fails to build on x32:
> 
> 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..

Ok, I took a look at how to disable this new XEN stuff on x32.

It is the commit 820c1aba519bd072ac71c754733f6c86d8b4309 "xen: add
CONFIG_XEN_BUS and CONFIG_XEN_EMU options for Xen emulation" adding
this construct to hw/i386/Kconfig:

config XEN_EMU
     bool
     default y
     depends on KVM && (I386 || X86_64)

Since meson does not know about x32, and while ./conifgure does, it
is not propagated to meson, and sure not propagated to Kconfig too,
there's some more work needed to disable XEN_EMU on x32.

Something like this?

diff --git a/target/i386/Kconfig b/target/i386/Kconfig
index ce6968906e..75a91f497a 100644
--- a/target/i386/Kconfig
+++ b/target/i386/Kconfig
@@ -3,3 +3,6 @@ config I386

  config X86_64
      bool
+
+config X32
+    bool

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d40802d83f..3ad6b44984 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -140,4 +140,4 @@ config VMMOUSE
  config XEN_EMU
      bool
      default y
-    depends on KVM && (I386 || X86_64)
+    depends on KVM && (I386 || X86_64) && !X32


diff --git a/meson.build b/meson.build
index c44d05a13f..9e7c83fc6a 100644
--- a/meson.build
+++ b/meson.build
@@ -70,6 +70,11 @@ if cpu in ['riscv32', 'riscv64']
    cpu = 'riscv'
  endif

+x32 = false
+if cpu == 'x86_64'
+  x32 = cc.sizeof('long') == 4
+endif
+
  target_dirs = config_host['TARGET_DIRS'].split()
  have_linux_user = false
  have_bsd_user = false
@@ -2554,7 +2559,8 @@ host_kconfig = \
    ('CONFIG_LINUX' in config_host ? ['CONFIG_LINUX=y'] : []) + \
    (have_pvrdma ? ['CONFIG_PVRDMA=y'] : []) + \
    (multiprocess_allowed ? ['CONFIG_MULTIPROCESS_ALLOWED=y'] : []) + \
-  (vfio_user_server_allowed ? ['CONFIG_VFIO_USER_SERVER_ALLOWED=y'] : [])
+  (vfio_user_server_allowed ? ['CONFIG_VFIO_USER_SERVER_ALLOWED=y'] : []) + \
+  (x32 ? ['CONFIG_X32=y'] : [])

  ignored = [ 'TARGET_XML_FILES', 'TARGET_ABI_DIR', 'TARGET_ARCH' ]

Re: xen bits broke x32 build

[Peter Maydell]

On Tue, 11 Apr 2023 at 12:57, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 01.04.2023 11:40, Michael Tokarev wrote:
> > After bringing in xen guest support, qemu fails to build on x32:
> >
> > 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..
>
> Ok, I took a look at how to disable this new XEN stuff on x32.
>
> It is the commit 820c1aba519bd072ac71c754733f6c86d8b4309 "xen: add
> CONFIG_XEN_BUS and CONFIG_XEN_EMU options for Xen emulation" adding
> this construct to hw/i386/Kconfig:
>
> config XEN_EMU
>      bool
>      default y
>      depends on KVM && (I386 || X86_64)
>
> Since meson does not know about x32, and while ./conifgure does, it
> is not propagated to meson, and sure not propagated to Kconfig too,
> there's some more work needed to disable XEN_EMU on x32.

Frankly I would prefer to just say "we don't support x32".
It's a weird non-standard configuration that as far as I'm
aware is very little used. Its stats in the debian
popularity-contest graphs peaked at 18 users in 2017, and
have mostly been fluctuating between 1 and 3 for the last
couple of years:
https://popcon.debian.org/stat/sub-x32.png

We're currently planning to deprecate-and-drop 32-bit x86
hosts, which are much more widely used than this. I see
no reason why we should care about this oddball failed
experiment of an ABI...

thanks
-- PMM

Re: xen bits broke x32 build

[Michael Tokarev]

11.04.2023 15:09, Peter Maydell wrote:
..
> Frankly I would prefer to just say "we don't support x32".
> It's a weird non-standard configuration that as far as I'm
> aware is very little used. Its stats in the debian
> popularity-contest graphs peaked at 18 users in 2017, and
> have mostly been fluctuating between 1 and 3 for the last
> couple of years:
> https://popcon.debian.org/stat/sub-x32.png

x32 was a nice idea but it lacked some final steps for it
to fly. In my opinion anyway.  Its compactness and speed
are fantastic, - qemu build is about 10..15% faster with
x32 gcc than it is with x86_64 gcc.

At the time debian picked it up, it was not very usable
b/c too many things didn't work and needed care.  Today,
much more software actually works on x32. It is more,
today with debian multiarch setup, it is possible to install
some *parts* of the system to be x32 while the rest being
x86_64, either for parts which benefits from x32 the most,
or the other way around, main x32 and some parts x86_64.
But it *still* lacks some infrastructure in debian, so it
is possible to do with stable or testing distribution, -
right now it is possible with unstable only.  Maybe we
can change that for bookworm+.

The thing is that now, it is much more complete than it
was in 2017, and it'd be really sad if it goes away.

x32 reveals some interesting problems in the code such
as type misuse, it already helped to find and fix some
bugs in some software, - for example in samba, where
a pointer was misused to store a time_t (which would
break with past-2038 time_t).

qemu never said it supports x32, and no one demanded
such support from it. It's interesting to have it working
there still, I *think*, as long as it does not require
extra efforts.

I'm fine to maintain the change required to keep it at
least buildable on x32 in debian - again as long as it
does not require huge efforts.

> We're currently planning to deprecate-and-drop 32-bit x86
> hosts, which are much more widely used than this. I see
> no reason why we should care about this oddball failed
> experiment of an ABI...

Thanks,

/mjt

Re: xen bits broke x32 build

[Philippe Mathieu-Daudé]

On 11/4/23 19:30, Michael Tokarev wrote:
> 11.04.2023 15:09, Peter Maydell wrote:
> ..
>> Frankly I would prefer to just say "we don't support x32".
>> It's a weird non-standard configuration that as far as I'm
>> aware is very little used. Its stats in the debian
>> popularity-contest graphs peaked at 18 users in 2017, and
>> have mostly been fluctuating between 1 and 3 for the last
>> couple of years:
>> https://popcon.debian.org/stat/sub-x32.png
> 
> x32 was a nice idea but it lacked some final steps for it
> to fly.

I used to use a FreeRTOS POSIX/ucontext port compiled in x32.
This was useful to mimic FreeRTOS baremetal ARM32 memory footprint.

I see having a x32 QEMU binary as a masochist experiment =)

> In my opinion anyway.  Its compactness and speed
> are fantastic, - qemu build is about 10..15% faster with
> x32 gcc than it is with x86_64 gcc.
> 
> At the time debian picked it up, it was not very usable
> b/c too many things didn't work and needed care.  Today,
> much more software actually works on x32. It is more,
> today with debian multiarch setup, it is possible to install
> some *parts* of the system to be x32 while the rest being
> x86_64, either for parts which benefits from x32 the most,
> or the other way around, main x32 and some parts x86_64.
> But it *still* lacks some infrastructure in debian, so it
> is possible to do with stable or testing distribution, -
> right now it is possible with unstable only.  Maybe we
> can change that for bookworm+.
> 
> The thing is that now, it is much more complete than it
> was in 2017, and it'd be really sad if it goes away.
> 
> x32 reveals some interesting problems in the code such
> as type misuse, it already helped to find and fix some
> bugs in some software, - for example in samba, where
> a pointer was misused to store a time_t (which would
> break with past-2038 time_t).

Why wasn't this caught by other 32-bit target?

> qemu never said it supports x32, and no one demanded
> such support from it. It's interesting to have it working
> there still, I *think*, as long as it does not require
> extra efforts.
> 
> I'm fine to maintain the change required to keep it at
> least buildable on x32 in debian - again as long as it
> does not require huge efforts.
> 
>> We're currently planning to deprecate-and-drop 32-bit x86
>> hosts, which are much more widely used than this. I see
>> no reason why we should care about this oddball failed
>> experiment of an ABI...
> 
> Thanks,
> 
> /mjt
>