[Qemu-devel] Re: Porting QEMU to new hosts with unusual ABI (sizeof(long) != sizeof(void *))

[Stefan Weil]

Am 05.02.2011 16:35, schrieb Blue Swirl:
> On Sat, Feb 5, 2011 at 2:39 PM, Stefan Weil <address@hidden> wrote:
> > Currently, most QEMU code assumes that pointers and long integers have
> > the same size, typically 32 bit on 32 bit hosts, 64 bit on 64 bit hosts.
> >
> > While this assumption works on QEMU's major hosts, it is not 
> > generally true.
> >
> > There exist 64 bit host OS which use an ABI with 32 bit long integers,
> > maybe to be more compatible with an older 32 bit OS version, so here is
> > sizeof(long) < sizeof(void *).
>
> Oh, that OS-Which-Must-Not-Be-Named.

Right.

> > Other ABIs might use "near" pointers which may reduce code size and 
> > improve
> > code speed. This results in sizeof(long) > sizeof(void *).
> >
> > Both cases will break current QEMU, because lots of code lines use
> > type casts from pointer to long or vice versa like these two examples:
> >
> > start = (long)mmap((void *)host_start, host_len ...
> > code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + ...))
> >
> > Both variants (unsigned long) and (long) can be found (relation 3:2).
> >
> > Changing the existing limitation of QEMU's code simply needs replacing
> > all those type casts, variable declarations and printf format specifiers
> > by portable code.
> >
> > The standard integral type which matches the size of a pointer is defined
> > in stdint.h (which also defines int8_t, ...). It is intptr_t (signed
> > version)
> > or uintptr_t (unsigned version). There is no need to use both.
> >
> > => Replace (unsigned long), (long) type casts of pointers by (uintptr_t).
> >
> > All variables (auto, struct members, parameters) which hold such values
> > must be fixed, too. In the following examples, ptr_var is of that kind.
> >
> > => Replace unsigned long ptr_var, long ptr_var by uintptr_t ptr_var.
> >
> > Finally, the fixed variables are used in printf-like statements,
> > so here the format specifier needs a change. inttypes.h defines
> > PRIxPTR which should be used.
> >
> > => Replace "%lx" by "%" PRIxPTR to print the integer value ptr_var.
> >
> > A single patch which includes all these changes touches 39 files.
> > Splitting it into smaller patches is not trivial because of cross
> > dependencies. Because of its size, it will raise merge conflicts
> > when it is not applied soon.
> >
> > Would these kind of changes be interesting for QEMU?
>
> Does QEMU work in OS-Which-Must-Not-Be-Named when the patch is applied?

Up to now, I never used OS-Which-Must-Not-Be-Named (64 bit) :-)
The patch allows to build executables (which I have put on my
web page).

I noticed two remaining problems with time_t and GetProcessAffinityMask.
Maybe these are not critical, but of course they have to be fixed, too.

> > Are there suggestions how it should be done?
>
> The change, done properly, should not cause any problems for most
> other hosts, where unsigned long size equals pointer type size.

That's correct. The risk of breaking current hosts is minimal.

> > What about ram_addr_t? Should it be replaced by uintptr_t?
>
> I'd use
> typedef uintptr_t ram_addr_t;

So did I. We could go further and eliminate ram_addr_t.

> > Should we use macros like QEMU_PTR2UINT(p), QEMU_UINT2PTR(u)?
>
> Rather, inline functions if open coding is not clear.
>
> > My current version of the patch is available from
> > http://qemu.weilnetz.de/0001-Fix-conversions-from-pointer-to-integral-type-and-vi.patch.
>
> Some comments:
>
> The patch changes also signed longs to uintptr_t. That could introduce
> regressions, so please use signed/unsigned as original.

I changed the code manually, and there was only one location where
signed/unsigned made a difference. That single case was an int
parameter passed in a void pointer, and I used intptr_t there.

I had the impression that in the current code (long) was
sometimes used because it is shorter than (unsigned long) :-)

As long as changes are made manually with the necessary care,
I'd recommend using uintptr_t where possible.

> For example in cpu_physical_memory_reset_dirty() you didn't change
> length, but that should probably become uintptr_t too.

Yes, that would be better (even if dirty memory ranges > 4 GiB
are not common). I'll change that.

> */translate.c: exit_tb() helper uses tcg_target_long, so the cast
> should use that instead.

Obviously tcg_target_long has the same size as uintptr_t,
so I can change this, too.

Regards,
Stefan