| Message ID | 1458900629-2334-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@redhat.com> > --- > 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 <famz@redhat.com> > 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 <famz@redhat.com> > Message-Id: <1458900629-2334-3-git-send-email-famz@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > :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 <drjones@redhat.com> Tested-by: Drew Jones <drjones@redhat.com> If my analysis above is correct, then I wouldn't mind taking credit for it, something like: Analyzed-by: Laszlo Ersek <lersek@redhat.com> Final protip: assert() is our friend! :) Cheers! Laszlo
On 24/05/2016 19:47, Laszlo Ersek wrote: > 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 <drjones@redhat.com> > Tested-by: Drew Jones <drjones@redhat.com> > > If my analysis above is correct, then I wouldn't mind taking credit > for it, something like: > > Analyzed-by: Laszlo Ersek <lersek@redhat.com> That makes a lot of sense. Revert on the way. Thanks, Paolo
On Tue, 05/24 19:47, Laszlo Ersek wrote: > 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. Yes, it is a mistake, let's revert it. Thanks a lot for the analysis! Fam
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. */
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 <famz@redhat.com> --- memory.c | 4 ---- 1 file changed, 4 deletions(-)