[Qemu-devel] [PATCH 0/2] memory: Dead code removals

[Qemu-devel] [PATCH 0/2] memory: Dead code removals

[Fam Zheng]

Fam Zheng (2):
  memory: Remove code for mr->may_overlap
  memory: Drop FlatRange.romd_mode

 include/exec/memory.h |  1 -
 memory.c              | 39 ---------------------------------------
 2 files changed, 40 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] memory: Remove code for mr->may_overlap

[Fam Zheng]

The collision check does nothing and hasn't been used. Remove the
variable together with related code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/exec/memory.h |  1 -
 memory.c              | 35 -----------------------------------
 2 files changed, 36 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2de7898..f071a7c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -188,7 +188,6 @@ struct MemoryRegion {
     MemoryRegion *alias;
     hwaddr alias_offset;
     int32_t priority;
-    bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;
     QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
diff --git a/memory.c b/memory.c
index 95f7209..d5b75f2 100644
--- a/memory.c
+++ b/memory.c
@@ -1054,13 +1054,6 @@ static void memory_region_get_priority(Object *obj, Visitor *v,
     visit_type_int32(v, name, &value, errp);
 }
 
-static bool memory_region_get_may_overlap(Object *obj, Error **errp)
-{
-    MemoryRegion *mr = MEMORY_REGION(obj);
-
-    return mr->may_overlap;
-}
-
 static void memory_region_get_size(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
@@ -1098,10 +1091,6 @@ static void memory_region_initfn(Object *obj)
                         memory_region_get_priority,
                         NULL, /* memory_region_set_priority */
                         NULL, NULL, &error_abort);
-    object_property_add_bool(OBJECT(mr), "may-overlap",
-                             memory_region_get_may_overlap,
-                             NULL, /* memory_region_set_may_overlap */
-                             &error_abort);
     object_property_add(OBJECT(mr), "size", "uint64",
                         memory_region_get_size,
                         NULL, /* memory_region_set_size, */
@@ -1861,7 +1850,6 @@ void memory_region_del_eventfd(MemoryRegion *mr,
 
 static void memory_region_update_container_subregions(MemoryRegion *subregion)
 {
-    hwaddr offset = subregion->addr;
     MemoryRegion *mr = subregion->container;
     MemoryRegion *other;
 
@@ -1869,27 +1857,6 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
 
     memory_region_ref(subregion);
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
-        if (subregion->may_overlap || other->may_overlap) {
-            continue;
-        }
-        if (int128_ge(int128_make64(offset),
-                      int128_add(int128_make64(other->addr), other->size))
-            || int128_le(int128_add(int128_make64(offset), subregion->size),
-                         int128_make64(other->addr))) {
-            continue;
-        }
-#if 0
-        printf("warning: subregion collision %llx/%llx (%s) "
-               "vs %llx/%llx (%s)\n",
-               (unsigned long long)offset,
-               (unsigned long long)int128_get64(subregion->size),
-               subregion->name,
-               (unsigned long long)other->addr,
-               (unsigned long long)int128_get64(other->size),
-               other->name);
-#endif
-    }
-    QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
             goto done;
@@ -1915,7 +1882,6 @@ void memory_region_add_subregion(MemoryRegion *mr,
                                  hwaddr offset,
                                  MemoryRegion *subregion)
 {
-    subregion->may_overlap = false;
     subregion->priority = 0;
     memory_region_add_subregion_common(mr, offset, subregion);
 }
@@ -1925,7 +1891,6 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          MemoryRegion *subregion,
                                          int priority)
 {
-    subregion->may_overlap = true;
     subregion->priority = priority;
     memory_region_add_subregion_common(mr, offset, subregion);
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/2] memory: Remove code for mr->may_overlap

[Peter Maydell]

On 25 March 2016 at 10:10, Fam Zheng <famz@redhat.com> wrote:
> The collision check does nothing and hasn't been used. Remove the
> variable together with related code.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>

I would prefer it if we enabled the collision check and fixed
the things which weren't specifying overlap priorities.

thanks
-- PMM

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

[Fam Zheng]

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. */
-- 
2.4.3

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 <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

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

[Paolo Bonzini]

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

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

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH 0/2] memory: Dead code removals

[Paolo Bonzini]

> Fam Zheng (2):
>   memory: Remove code for mr->may_overlap
>   memory: Drop FlatRange.romd_mode
> 
>  include/exec/memory.h |  1 -
>  memory.c              | 39 ---------------------------------------
>  2 files changed, 40 deletions(-)


Thanks, both look good.  Not sure they'll make it in 2.6 though.

Paolo