From: "Laszlo Ersek \(Red Hat\)" <lersek@redhat.com>
To: qemu-devel@nongnu.org
Subject: [Qemu-devel] [Bug 1830872] Re: [RFC PATCH] cputlb: use uint64_t for interim values for unaligned load
Date: Mon, 03 Jun 2019 18:29:11 -0000 [thread overview]
Message-ID: <5f684c68-d2cd-90f7-3892-1c6245728042@redhat.com> (raw)
In-Reply-To: 20190603150120.29255-1-alex.bennee@linaro.org
(+Igor)
On 06/03/19 17:01, Alex Bennée wrote:
> When running on 32 bit TCG backends a wide unaligned load ends up
> truncating data before returning to the guest. We specifically have
> the return type as uint64_t to avoid any premature truncation so we
> should use the same for the interim types.
>
> Hopefully fixes #1830872
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> accel/tcg/cputlb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index cdcc3771020..b796ab1cbea 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
> && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
> >= TARGET_PAGE_SIZE)) {
> target_ulong addr1, addr2;
> - tcg_target_ulong r1, r2;
> + uint64_t r1, r2;
> unsigned shift;
> do_unaligned_access:
> addr1 = addr & ~(size - 1);
>
Applied on top of commit ad88e4252f09c2956b99c90de39e95bab2e8e7af:
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1830872
Title:
AARCH64 to ARMv7 mistranslation in TCG
Status in QEMU:
New
Bug description:
The following guest code:
https://github.com/tianocore/edk2/blob/3604174718e2afc950c3cc64c64ba5165c8692bd/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S
implements, in hand-optimized aarch64 assembly, the CopyMem() edk2 (EFI
Development Kit II) library function. (CopyMem() basically has memmove()
semantics, to provide a standard C analog here.) The relevant functions
are InternalMemCopyMem() and __memcpy().
When TCG translates this aarch64 code to x86_64, everything works
fine.
When TCG translates this aarch64 code to ARMv7, the destination area of
the translated CopyMem() function becomes corrupted -- it differs from
the intended source contents. Namely, in every 4096 byte block, the
8-byte word at offset 4032 (0xFC0) is zeroed out in the destination,
instead of receiving the intended source value.
I'm attaching two hexdumps of the same destination area:
- "good.txt" is a hexdump of the destination area when CopyMem() was
translated to x86_64,
- "bad.txt" is a hexdump of the destination area when CopyMem() was
translated to ARMv7.
In order to assist with the analysis of this issue, I disassembled the
aarch64 binary with "objdump". Please find the listing in
"DxeCore.objdump", attached. The InternalMemCopyMem() function starts at
hex offset 2b2ec. The __memcpy() function starts at hex offset 2b180.
And, I ran the guest on the ARMv7 host with "-d
in_asm,op,op_opt,op_ind,out_asm". Please find the log in
"tcg.in_asm.op.op_opt.op_ind.out_asm.log", attached.
The TBs that correspond to (parts of) the InternalMemCopyMem() and
__memcpy() functions are scattered over the TCG log file, but the offset
between the "nice" disassembly from "DxeCore.objdump", and the in-RAM
TBs in the TCG log, can be determined from the fact that there is a
single prfm instruction in the entire binary. The instruction's offset
is 0x2b180 in "DxeCore.objdump" -- at the beginning of the __memcpy()
function --, and its RAM address is 0x472d2180 in the TCG log. Thus the
difference (= the load address of DxeCore.efi) is 0x472a7000.
QEMU was built at commit a4f667b67149 ("Merge remote-tracking branch
'remotes/cohuck/tags/s390x-20190521-3' into staging", 2019-05-21).
The reproducer command line is (on an ARMv7 host):
qemu-system-aarch64 \
-display none \
-machine virt,accel=tcg \
-nodefaults \
-nographic \
-drive if=pflash,format=raw,file=$prefix/share/qemu/edk2-aarch64-code.fd,readonly \
-drive if=pflash,format=raw,file=$prefix/share/qemu/edk2-arm-vars.fd,snapshot=on \
-cpu cortex-a57 \
-chardev stdio,signal=off,mux=on,id=char0 \
-mon chardev=char0,mode=readline \
-serial chardev:char0
The apparent symptom is an assertion failure *in the guest*, such as
> ASSERT [DxeCore]
> /home/lacos/src/upstream/qemu/roms/edk2/MdePkg/Library/BaseLib/String.c(1090):
> Length < _gPcd_FixedAtBuild_PcdMaximumAsciiStringLength
but that is only a (distant) consequence of the CopyMem()
mistranslation, and resultant destination area corruption.
Originally reported in the following two mailing list messages:
- http://mid.mail-archive.com/9d2e260c-c491-03d2-9b8b-b57b72083f77@redhat.com
- http://mid.mail-archive.com/f1cec8c0-1a9b-f5bb-f951-ea0ba9d276ee@redhat.com
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1830872/+subscriptions
WARNING: multiple messages have this Message-ID (diff)
From: Laszlo Ersek <lersek@redhat.com>
To: "Alex Bennée" <alex.bennee@linaro.org>, qemu-devel@nongnu.org
Cc: 1830872@bugs.launchpad.net, qemu-arm@nongnu.org,
Paolo Bonzini <pbonzini@redhat.com>,
Igor Mammedov <imammedo@redhat.com>,
randrianasulu@gmail.com, Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [RFC PATCH] cputlb: use uint64_t for interim values for unaligned load
Date: Mon, 3 Jun 2019 20:29:11 +0200 [thread overview]
Message-ID: <5f684c68-d2cd-90f7-3892-1c6245728042@redhat.com> (raw)
Message-ID: <20190603182911.WXljjtLe-cUK0xc7e0t1M3QEkIbKGqryppdHNYfRo3I@z> (raw)
In-Reply-To: <20190603150120.29255-1-alex.bennee@linaro.org>
(+Igor)
On 06/03/19 17:01, Alex Bennée wrote:
> When running on 32 bit TCG backends a wide unaligned load ends up
> truncating data before returning to the guest. We specifically have
> the return type as uint64_t to avoid any premature truncation so we
> should use the same for the interim types.
>
> Hopefully fixes #1830872
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> accel/tcg/cputlb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index cdcc3771020..b796ab1cbea 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
> && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
> >= TARGET_PAGE_SIZE)) {
> target_ulong addr1, addr2;
> - tcg_target_ulong r1, r2;
> + uint64_t r1, r2;
> unsigned shift;
> do_unaligned_access:
> addr1 = addr & ~(size - 1);
>
Applied on top of commit ad88e4252f09c2956b99c90de39e95bab2e8e7af:
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
next prev parent reply other threads:[~2019-06-03 18:42 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-29 9:13 [Qemu-devel] [Bug 1830872] [NEW] AARCH64 to ARMv7 mistranslation in TCG Laszlo Ersek (Red Hat)
2019-05-29 12:08 ` [Qemu-devel] [Bug 1830872] " Philippe Mathieu-Daudé
2019-06-02 10:19 ` Laszlo Ersek (Red Hat)
2019-06-02 13:30 ` Alex Bennée
2019-06-02 13:30 ` Alex Bennée
2019-06-03 11:56 ` Alex Bennée
2019-06-03 11:56 ` Alex Bennée
2019-06-02 14:54 ` Alex Bennée
2019-06-03 15:01 ` [Qemu-devel] [RFC PATCH] cputlb: use uint64_t for interim values for unaligned load Alex Bennée
2019-06-03 15:01 ` [Qemu-devel] [Bug 1830872] " Alex Bennée
2019-06-03 15:35 ` [Qemu-devel] " Andrew Randrianasulu
2019-06-03 15:35 ` [Qemu-devel] [Bug 1830872] " Andrew Randrianasulu
2019-06-04 9:43 ` [Qemu-devel] " Alex Bennée
2019-06-04 9:43 ` [Qemu-devel] [Bug 1830872] " Alex Bennée
2019-06-03 18:29 ` Laszlo Ersek (Red Hat) [this message]
2019-06-03 18:29 ` [Qemu-devel] " Laszlo Ersek
2019-06-03 22:01 ` Richard Henderson
2019-06-04 6:52 ` Philippe Mathieu-Daudé
2019-06-04 11:42 ` Igor Mammedov
2019-06-04 11:42 ` [Qemu-devel] [Bug 1830872] " Igor
2019-06-03 15:27 ` [Qemu-devel] [Bug 1830872] Re: AARCH64 to ARMv7 mistranslation in TCG Laszlo Ersek (Red Hat)
2019-06-03 15:45 ` Laszlo Ersek (Red Hat)
2019-06-03 15:51 ` Alex Bennée
2019-06-03 16:53 ` Andrew Randrianasulu
2019-06-03 17:03 ` Andrew Randrianasulu
2019-06-17 18:21 ` Alex Bennée
2019-08-16 5:04 ` Thomas Huth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5f684c68-d2cd-90f7-3892-1c6245728042@redhat.com \
--to=lersek@redhat.com \
--cc=1830872@bugs.launchpad.net \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.