From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5GQK-000307-U9 for qemu-devel@nongnu.org; Tue, 24 May 2016 13:47:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5GQG-0007GN-FZ for qemu-devel@nongnu.org; Tue, 24 May 2016 13:47:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5GQG-0007G2-6H for qemu-devel@nongnu.org; Tue, 24 May 2016 13:47:16 -0400 References: <1458900629-2334-1-git-send-email-famz@redhat.com> <1458900629-2334-3-git-send-email-famz@redhat.com> From: Laszlo Ersek Message-ID: Date: Tue, 24 May 2016 19:47:12 +0200 MIME-Version: 1.0 In-Reply-To: <1458900629-2334-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] memory: Drop FlatRange.romd_mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Paolo Bonzini , Drew Jones , Ard Biesheuvel , Peter Maydell On 03/25/16 11:10, Fam Zheng wrote: > Its value is alway set to mr->romd_mode, so the removed comparisons are > fully superseded by "a->mr == b->mr". > > Signed-off-by: Fam Zheng > --- > memory.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/memory.c b/memory.c > index d5b75f2..26af83f 100644 > --- a/memory.c > +++ b/memory.c > @@ -224,7 +224,6 @@ struct FlatRange { > hwaddr offset_in_region; > AddrRange addr; > uint8_t dirty_log_mask; > - bool romd_mode; > bool readonly; > }; > > @@ -249,7 +248,6 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b) > return a->mr == b->mr > && addrrange_equal(a->addr, b->addr) > && a->offset_in_region == b->offset_in_region > - && a->romd_mode == b->romd_mode > && a->readonly == b->readonly; > } > > @@ -309,7 +307,6 @@ static bool can_merge(FlatRange *r1, FlatRange *r2) > r1->addr.size), > int128_make64(r2->offset_in_region)) > && r1->dirty_log_mask == r2->dirty_log_mask > - && r1->romd_mode == r2->romd_mode > && r1->readonly == r2->readonly; > } > > @@ -663,7 +660,6 @@ static void render_memory_region(FlatView *view, > > fr.mr = mr; > fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr); > - fr.romd_mode = mr->romd_mode; > fr.readonly = readonly; > > /* Render the region itself into any gaps left by the current view. */ > This patch breaks the UEFI guest firmware (known as ArmVirtPkg or AAVMF) running in the "virt" machine type of "qemu-system-aarch64": > 5b5660adf1fdb61db14ec681b10463b8cba633f1 is the first bad commit > commit 5b5660adf1fdb61db14ec681b10463b8cba633f1 > Author: Fam Zheng > Date: Fri Mar 25 18:10:29 2016 +0800 > > memory: Drop FlatRange.romd_mode > > Its value is alway set to mr->romd_mode, so the removed comparisons are > fully superseded by "a->mr == b->mr". > > Signed-off-by: Fam Zheng > Message-Id: <1458900629-2334-3-git-send-email-famz@redhat.com> > Signed-off-by: Paolo Bonzini > > :100644 100644 ac5236b51587ee397edd177502fc20ce159f2235 9d00dc5e7ea7406a248d32312fbf044e0ff24a3b M memory.c Bisection log follows: > git bisect start > # good: [975eb6a547f809608ccb08c221552f666611af25] Update version for v2.6.0-rc4 release > git bisect good 975eb6a547f809608ccb08c221552f666611af25 > # bad: [287db79df8af8e31f18e262feb5e05103a09e4d4] Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging > git bisect bad 287db79df8af8e31f18e262feb5e05103a09e4d4 > # good: [2cdc848eb5bd7caf467942aee63f813f52db4e40] slirp: Remove some unused code from slirp.h > git bisect good 2cdc848eb5bd7caf467942aee63f813f52db4e40 > # good: [776efef32439a31cb13a6acfe8aab833687745ad] Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging > git bisect good 776efef32439a31cb13a6acfe8aab833687745ad > # good: [2fe760554eb3769d70f608a158474f728ba45ba6] virtio-gpu: check max_outputs only > git bisect good 2fe760554eb3769d70f608a158474f728ba45ba6 > # bad: [c9158547617584bb9d19db7fb139998fbef80133] Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging > git bisect bad c9158547617584bb9d19db7fb139998fbef80133 > # bad: [fd28938b7adb33f8af11849cdd0d0b2fb92990e3] scripts/signrom.py: Check for magic in option ROMs. > git bisect bad fd28938b7adb33f8af11849cdd0d0b2fb92990e3 > # good: [e92a2d9cb3d8f589c9fe5d2eacc83d8dddea0e16] vl: change runstate only if new state is different from current state > git bisect good e92a2d9cb3d8f589c9fe5d2eacc83d8dddea0e16 > # bad: [5b5660adf1fdb61db14ec681b10463b8cba633f1] memory: Drop FlatRange.romd_mode > git bisect bad 5b5660adf1fdb61db14ec681b10463b8cba633f1 > # good: [ab0a99560857302b60053c245d1231acbd976cd4] exec: adjust rcu_read_lock requirement > git bisect good ab0a99560857302b60053c245d1231acbd976cd4 > # good: [b61359781958759317ee6fd1a45b59be0b7dbbe1] memory: Remove code for mr->may_overlap > git bisect good b61359781958759317ee6fd1a45b59be0b7dbbe1 > # first bad commit: [5b5660adf1fdb61db14ec681b10463b8cba633f1] memory: Drop FlatRange.romd_mode The AAVMF breakage occurs during access to the pflash chip. The pflash chip ("hw/block/pflash_cfi01.c") is a ROMD device. It means that the guest-phys address range that is backed by pflash normally behaves as ROM (romd_mode==1): it is readable and executable without traps, but writing to it causes a trap. In this mode, KVM backs the memory range with a read-only memory slot. When writing to the pflash chip in this mode, the device is flipped over to "programming mode" or "command mode". In this mode, (romd_mode==0). KVM removes the read-only memory slot (that is, no memory at all will back the guest-phys address range), and both reads and writes to the range will trap to QEMU, every single access. There is a special command that, when written, flips the device back to ROMD mode (-> reads and executes again without traps, as ROM). The patch breaks this behavior. Namely, contrary to the commit message, (a->mr == b->mr) does *not* imply that (a->romd_mode == b->romd_mode): the pflash device model calls memory_region_rom_device_set_romd() -- for switching between the above modes --, and that function changes mr->romd_mode *only*: void memory_region_rom_device_set_romd(MemoryRegion *mr, bool romd_mode) { if (mr->romd_mode != romd_mode) { memory_region_transaction_begin(); mr->romd_mode = romd_mode; memory_region_update_pending |= mr->enabled; memory_region_transaction_commit(); } } Which I think satisfies (a->mr == b->mr), but falsifies (a->romd_mode == b->romd_mode). In effect, the patch seems to allow merging and equality between FlatRange objects when they only differ in romd_mode, and that's wrong. Given that the cover letter for this series says "memory: Dead code removals", I'm requesting that this patch be simply reverted. Drew and myself bisected this independently, in parallel. The bug was originally reported by Drew, so I think on the revert commit, the Reported-by should belong to him. Beyond the bisection, Drew also tested the exact revert, and it restores functionality. For which reason, I'm proposing, for the revert patch: Reported-by: Drew Jones Tested-by: Drew Jones If my analysis above is correct, then I wouldn't mind taking credit for it, something like: Analyzed-by: Laszlo Ersek Final protip: assert() is our friend! :) Cheers! Laszlo