Re: [Qemu-devel] [PATCH 2/2] memory: Drop FlatRange.romd_mode

[Laszlo Ersek]

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 <address@hidden>
> ---
>  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 <address@hidden>
> 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 <address@hidden>
>     Message-Id: <address@hidden>
>     Signed-off-by: Paolo Bonzini <address@hidden>
> 
> :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 <address@hidden>
Tested-by: Drew Jones <address@hidden>

If my analysis above is correct, then I wouldn't mind taking credit for it, 
something like:

Analyzed-by: Laszlo Ersek <address@hidden>

Final protip: assert() is our friend! :)

Cheers!
Laszlo