log_start might be called by memory.c just before registering the
section. So we can actually get a log_start without a region_add, which
we can silently ignore.

This makes current KVM code trigger an assertion
("kvm_section_update_flags: error finding slot").

Also, if we want to trap every access to a section, we might not have a
slot. So let's just tolerate if we don't have a slot.

Fixes: 343562e8fa22 ("kvm: kvm_log_start/stop are only called with known sections")
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 90c88b517d..64de8461e0 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -394,8 +394,11 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
 
     mem = kvm_lookup_matching_slot(kml, start_addr, size);
     if (!mem) {
-        fprintf(stderr, "%s: error finding slot\n", __func__);
-        abort();
+        /*
+         * log_start() might be called before region_add(), and sometimes
+         * we don't have a slot as we want to trap every access.
+         */
+        return 0;
     }
 
     return kvm_slot_update_flags(kml, mem, section->mr);
-- 
2.13.5

On 16.10.2017 11:41, David Hildenbrand wrote:
> log_start might be called by memory.c just before registering the
> section. So we can actually get a log_start without a region_add, which
> we can silently ignore.
> 
> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot").
> 
> Also, if we want to trap every access to a section, we might not have a
> slot. So let's just tolerate if we don't have a slot.
> 
> Fixes: 343562e8fa22 ("kvm: kvm_log_start/stop are only called with known sections")
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 90c88b517d..64de8461e0 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -394,8 +394,11 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
>  
>      mem = kvm_lookup_matching_slot(kml, start_addr, size);
>      if (!mem) {
> -        fprintf(stderr, "%s: error finding slot\n", __func__);
> -        abort();
> +        /*
> +         * log_start() might be called before region_add(), and sometimes
> +         * we don't have a slot as we want to trap every access.
> +         */
> +        return 0;
>      }
>  
>      return kvm_slot_update_flags(kml, mem, section->mr);
> 

I'll also send a patch for log_sync(), dropping the same assert.

Looks like adding these assertions was counter productive :)

-- 

Thanks,

David

On 16/10/2017 11:53, David Hildenbrand wrote:
> 
> Looks like adding these assertions was counter productive :)

Or productive.  Bugs happen, and if they are a good occasion to add new
tests to the testsuite or at least a comment to the code, all the better.

Paolo

On 16/10/2017 11:41, David Hildenbrand wrote:
> log_start might be called by memory.c just before registering the
> section. So we can actually get a log_start without a region_add, which
> we can silently ignore.

This is really a bug in memory.c, I think.  When you put together
everything as a single patch series, can you include the memory.c change
instead, and drop this reference in the commit message and the kvm-all.c
comment?

Thanks,

Paolo

> This makes current KVM code trigger an assertion
> ("kvm_section_update_flags: error finding slot").
> 
> Also, if we want to trap every access to a section, we might not have a
> slot. So let's just tolerate if we don't have a slot.