From: Igor Mammedov <imammedo@redhat.com> To: "Alex Bennée" <alex.bennee@linaro.org> Cc: qemu-devel@nongnu.org, 1830872@bugs.launchpad.net, qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@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: Tue, 4 Jun 2019 13:42:05 +0200 [thread overview] Message-ID: <20190604134205.757b217a@redhat.com> (raw) In-Reply-To: <20190603150120.29255-1-alex.bennee@linaro.org> On Mon, 3 Jun 2019 16:01:20 +0100 Alex Bennée <alex.bennee@linaro.org> 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> Fixes arm/virt bios-tables-test for me, so Tested-by: Igor Mammedov <imammedo@redhat.com> > --- > 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);
WARNING: multiple messages have this Message-ID (diff)
From: Igor <imammedo@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: Tue, 04 Jun 2019 11:42:05 -0000 [thread overview] Message-ID: <20190604134205.757b217a@redhat.com> (raw) Message-ID: <20190604114205.v_izOkoUBRpFXRkluHQS-x6oNAbBgIn2rQTU7WUhlNY@z> (raw) In-Reply-To: 20190603150120.29255-1-alex.bennee@linaro.org On Mon, 3 Jun 2019 16:01:20 +0100 Alex Bennée <alex.bennee@linaro.org> 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> Fixes arm/virt bios-tables-test for me, so Tested-by: Igor Mammedov <imammedo@redhat.com> > --- > 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); -- 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
next prev parent reply other threads:[~2019-06-04 11:43 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) 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 [this message] 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=20190604134205.757b217a@redhat.com \ --to=imammedo@redhat.com \ --cc=1830872@bugs.launchpad.net \ --cc=alex.bennee@linaro.org \ --cc=pbonzini@redhat.com \ --cc=qemu-arm@nongnu.org \ --cc=qemu-devel@nongnu.org \ --cc=randrianasulu@gmail.com \ --cc=rth@twiddle.net \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).