Patch for ARM memory barriers and getdent

Please find submitted a patch for ARM memory barriers.  This patch is
against qemu-2.12-rc2 but I do believe it should apply for anything from
2.11.x to current.

I found with qemu 2.11.x or newer that I would get an illegal instruction
error running some Intel binaries on my ARM chromebook.  On investigation,
I found it was quitting on memory barriers.
qemu instruction:
mb $0x31
was translating as:
0x604050cc:  5bf07ff5  blpl     #0x600250a8

After patch it gives:
0x604050cc:  f57ff05b  dmb      ish

In short, I found INSN_DMB_ISH (memory barrier for ARMv7) appeared to be
correct based on online docs, but due to some endian-related shenanigans it
had to be byte-swapped to suit qemu; it appears INSN_DMB_MCR (memory
barrier for ARMv6) also should be byte swapped  (and this patch does so).
I have not checked for correctness of aarch64's barrier instruction.
-------------------------------
Please find submitted a patch for getdents (this system call stands for
"get directory entries", it is passed a file descriptor pointing to a
directory and returns a struct with info on the entries in that
directory.)  This patch is against qemu-2.10 series but continues to apply
cleanly on current as of April 15 2018.  If you are running a 32-bit binary
on 64-bit target current qemu will convert he getdents struct, but it will
NOT in the case of 64-bit binary on 32-bit host.  (In my case, I had
android SDK's "aapt" for x86-64 error out on 32-bit ARM without this patch,
and run fine with it.)  This patch simply replaces a "#if TARGET_ABI_BITS
== 32 && HOST_LONG_BITS == 64" with "#if TARGET_ABI_BITS != HOST_LONG_BITS".

Thanks!

On 16 April 2018 at 19:44, Henry Wertz <hwertz10@gmail.com> wrote:
> Please find submitted a patch for ARM memory barriers.  This patch is
> against qemu-2.12-rc2 but I do believe it should apply for anything from
> 2.11.x to current.

Hi Henry; thanks for these patches. Please could you provide
a Signed-off-by: line for them? This says you're happy for us
to apply them to QEMU under our license, and we can't do anything
with them without one. The top part of
https://wiki.qemu.org/Contribute/SubmitAPatch
has more detail.

> I found with qemu 2.11.x or newer that I would get an illegal instruction
> error running some Intel binaries on my ARM chromebook.  On investigation,
> I found it was quitting on memory barriers.
> qemu instruction:
> mb $0x31
> was translating as:
> 0x604050cc:  5bf07ff5  blpl     #0x600250a8
>
> After patch it gives:
> 0x604050cc:  f57ff05b  dmb      ish
>
> In short, I found INSN_DMB_ISH (memory barrier for ARMv7) appeared to be
> correct based on online docs, but due to some endian-related shenanigans it
> had to be byte-swapped to suit qemu; it appears INSN_DMB_MCR (memory
> barrier for ARMv6) also should be byte swapped  (and this patch does so).
> I have not checked for correctness of aarch64's barrier instruction.

Yeah, this definitely looks like a bug -- I can't see how the code
as it stands could ever have worked (it was added in commit 40f191ab8226fdada).

> -------------------------------
> Please find submitted a patch for getdents (this system call stands for
> "get directory entries",

You'll probably have better luck if you submit patches as separate
emails rather than all in one -- the set of people who care about
reviewing TCG backend patches is different from the people who
look after the linux-user syscall emulation code.

> it is passed a file descriptor pointing to a
> directory and returns a struct with info on the entries in that
> directory.)  This patch is against qemu-2.10 series but continues to apply
> cleanly on current as of April 15 2018.  If you are running a 32-bit binary
> on 64-bit target current qemu will convert he getdents struct, but it will
> NOT in the case of 64-bit binary on 32-bit host.

In general, 64-bit binary on a 32-bit host is not supported by linux-user.
Various things are likely to go wrong for anything but the simplest
of guest binaries. You need a 64-bit host to reliably use linux-user
for a 64-bit guest binary.

I think your change isn't sufficient to handle the "target 32 bits
and host 64 bits" case, because the 64-on-32 code path that you're
using for it uses the guest's buffer size as the size of the
host buffer it allocates to pass to the host syscall. That's ok
if the host is 64 bits, because the host struct linux_dirent will
be bigger than the target's struct linux_dirent, and so we can
guarantee to fit all the host's structures into the target's
buffer (with unused space left over at the end). If you try to
use the same code for "target 64 bits and host 32 bits" then we
may have more host dirent structures in the host buffer than we
can fit into the guest buffer, and we'll end up overrunning the
end of the guest's buffer. I don't see an obvious way to support
64-on-32 here, because the record sizes are variable, not fixed,
so you can't tell in advance what sized host buffer you'd need to
be able to fit the results from the kernel into the guest's buffer.
Maybe I'm missing a clever trick?

thanks
-- PMM

Hi Henry; thanks for these patches. Please could you provide
> a Signed-off-by: line for them? This says you're happy for us
> to apply them to QEMU under our license, and we can't do anything
> with them without one. The top part of
> https://wiki.qemu.org/Contribute/SubmitAPatch
> has more detail.
>
     Yup, I'll resubmit both in proper format.

> You'll probably have better luck if you submit patches as separate
> emails rather than all in one -- the set of people who care about
>
     Can do.

In general, 64-bit binary on a 32-bit host is not supported by linux-user.
> Various things are likely to go wrong for anything but the simplest
> of guest binaries. You need a 64-bit host to reliably use linux-user
> for a 64-bit guest binary.
>
     I didn't test it very hard, I was only using it to run the couple
Android SDK binaries (aapt and friends) that Android Studio runs; these in
general open a file or two, process them, write out a result, so they
probably are pretty simple guest binaries.  Nevertheless,with this patch
these few x86-64 binaries run on 32-bit ARM and (last I checked) 32-bit
Intel.


>
> I think your change isn't sufficient to handle the "target 32 bits
> and host 64 bits" case, because the 64-on-32 code path that you're
> using for it uses the guest's buffer size as the size of the
> host buffer it allocates to pass to the host syscall.
>
....

> Maybe I'm missing a clever trick?
>
     As far as I can tell, it makes a "linux_dirent" and "target_dirent"
and is pretty careful to calculate rather than assume offsets, and so on as
it copies data from one struct to the other.  i didn't look before, but
there is in fact a line "assert(count1 + treclen <= count);" that would
trigger if the buffer is going to be overflowed.  I think the code may not
have even been needed, except that d_type (file type) was moved to the
"middle" of the dirent64 struct instead of at the end.

Thanks!
-- Henry