* [Qemu-devel] Small patch for getdents syscall
@ 2017-03-07 23:40 Henry Wertz
2017-03-13 10:49 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Henry Wertz @ 2017-03-07 23:40 UTC (permalink / raw)
To: qemu-devel
I have a trivial, 1-line patch for getdents function; due to use of
unsigned long, the struct on 64-bit and 32-bit systems is not the same.
qemu is aware of this, however it currently only checks for a 32-bit target
on 64-bit host case; in my case, I'm running 64-bit target on 32-bit host
(x86-64 binaries on a 32-bit ARM).
My use case for this patch, I have Android Studio on my (32-bit ARM)
chromebook -- thank goodness I got the 4GB model! Since Android Studio is
java, that worked; but it calls aapt from build tools. Up through 23.x.x
(which is build for 32-bit x86) it worked. With 25.0.0 (and 24.x.x
version) aapt is now built for 64-bit x86-64; it would give an odd error
about a directory being invalid. By comparing strace between qemu and real
x86-64, I found getdents was behaving differently. WIth this patch aapt
completes.
I really wasn't sure if this would be better this way ("if target != host")
or if something like "if (target==32 && host==64) || (target==64 &&
host==32)" would be preferrable.
---------------------------------------------------
Signed-off-by: Henry Wertz <hwertz10@gmail.com>
*** linux-user/syscall.c~ 2017-03-04 10:31:14.000000000 -0600
--- linux-user/syscall.c 2017-03-07 17:08:24.615399116 -0600
***************
*** 9913,9921 ****
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
--- 9913,9921 ----
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS != HOST_LONG_BITS
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] Small patch for getdents syscall
2017-03-07 23:40 [Qemu-devel] Small patch for getdents syscall Henry Wertz
@ 2017-03-13 10:49 ` Peter Maydell
2017-03-13 19:00 ` Henry Wertz
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2017-03-13 10:49 UTC (permalink / raw)
To: Henry Wertz; +Cc: QEMU Developers, Riku Voipio, Laurent Vivier
On 8 March 2017 at 00:40, Henry Wertz <hwertz10@gmail.com> wrote:
> I have a trivial, 1-line patch for getdents function; due to use of
> unsigned long, the struct on 64-bit and 32-bit systems is not the same.
> qemu is aware of this, however it currently only checks for a 32-bit target
> on 64-bit host case; in my case, I'm running 64-bit target on 32-bit host
> (x86-64 binaries on a 32-bit ARM).
>
> My use case for this patch, I have Android Studio on my (32-bit ARM)
> chromebook -- thank goodness I got the 4GB model! Since Android Studio is
> java, that worked; but it calls aapt from build tools. Up through 23.x.x
> (which is build for 32-bit x86) it worked. With 25.0.0 (and 24.x.x
> version) aapt is now built for 64-bit x86-64; it would give an odd error
> about a directory being invalid. By comparing strace between qemu and real
> x86-64, I found getdents was behaving differently. WIth this patch aapt
> completes.
>
> I really wasn't sure if this would be better this way ("if target != host")
> or if something like "if (target==32 && host==64) || (target==64 &&
> host==32)" would be preferrable.
> ---------------------------------------------------
>
> Signed-off-by: Henry Wertz <hwertz10@gmail.com>
>
> *** linux-user/syscall.c~ 2017-03-04 10:31:14.000000000 -0600
> --- linux-user/syscall.c 2017-03-07 17:08:24.615399116 -0600
> ***************
> *** 9913,9921 ****
> #endif
> #ifdef TARGET_NR_getdents
> case TARGET_NR_getdents:
> #ifdef __NR_getdents
> ! #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
> {
> struct target_dirent *target_dirp;
> struct linux_dirent *dirp;
> abi_long count = arg3;
> --- 9913,9921 ----
> #endif
> #ifdef TARGET_NR_getdents
> case TARGET_NR_getdents:
> #ifdef __NR_getdents
> ! #if TARGET_ABI_BITS != HOST_LONG_BITS
> {
> struct target_dirent *target_dirp;
> struct linux_dirent *dirp;
> abi_long count = arg3;
There is a theoretically better way to handle the 64-bit
guest on 32-bit host case: since here you know that the
guest dirent struct is bigger than the host dirent
struct you could read them directly into the guest buffer
and then convert in-place rather than having to malloc
a temporary buffer. The conversion code is a little
bit awkward though because you have to make sure you don't
overwrite host data you haven't read yet, so I think
you would need to start at the end of the buffer and
work backwards. This is basically fixing the #else
part of this code to work for guest > host as well
as for guest == host.
However, since 64-bit-on-32 is not exactly a very common
use case these days I think your patch is an OK fix for
things.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
PS: we prefer 'unified' format diffs; git format-patch
should produce those by default.
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] Small patch for getdents syscall
2017-03-13 10:49 ` Peter Maydell
@ 2017-03-13 19:00 ` Henry Wertz
0 siblings, 0 replies; 3+ messages in thread
From: Henry Wertz @ 2017-03-13 19:00 UTC (permalink / raw)
To: Peter Maydell; +Cc: Laurent Vivier, Riku Voipio, QEMU Developers
On Mar 13, 2017 5:49 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 8 March 2017 at 00:40, Henry Wertz <hwertz10@gmail.com> wrote:
> > I have a trivial, 1-line patch for getdents function; due to
> >...
> There is a theoretically better way to handle the 64-bit
> guest on 32-bit host case: since here you know that the
> guest dirent struct is bigger than the host dirent
> struct you could read them directly into the guest buffer
> and then convert in-place rather than having to malloc
> a temporary buffer. The conversion code is a little
> bit awkward though because you have to make sure you don't
> overwrite host data you haven't read yet, so I think
> you would need to start at the end of the buffer and
> work backwards. This is basically fixing the #else
> part of this code to work for guest > host as well
> as for guest == host.
That does sound like it'd be better (avoiding one unnecessary malloc at
least.)
>
> However, since 64-bit-on-32 is not exactly a very common
> use case these days I think your patch is an OK fix for
> things.
I'd guess this use might (slightly) increase if more x86 binary software
begins to be shipped x86-64 instead. That said, this particular code path
may still barely be used; I would have expected most (especially 64-bit)
software would call getdents64 instead of getdents, and was surprised to
find aapt uses getdents instead.
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> PS: we prefer 'unified' format diffs; git format-patch
> should produce those by default.
>
> thanks
> -- PMM
OK I'll make sure to do that next time.
Thanks!
-- Henry Wertz
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-13 19:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-07 23:40 [Qemu-devel] Small patch for getdents syscall Henry Wertz
2017-03-13 10:49 ` Peter Maydell
2017-03-13 19:00 ` Henry Wertz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.