| Message ID | 20170911174933.20789-6-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11.09.2017 19:49, David Hildenbrand wrote: > Let's properly align the sections first and bail out if we would ever > get called with a memory section we don't know yet. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > accel/kvm/kvm-all.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index b677d1b13e..2ae459453d 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, > static int kvm_section_update_flags(KVMMemoryListener *kml, > MemoryRegionSection *section) > { > - hwaddr phys_addr = section->offset_within_address_space; > - ram_addr_t size = int128_get64(section->size); > - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); > + hwaddr start_addr, size; > + KVMSlot *mem; > > - if (mem == NULL) { > + size = kvm_align_section(section, &start_addr); > + if (!size) { > return 0; > - } else { > - return kvm_slot_update_flags(kml, mem, section->mr); > } > + > + mem = kvm_lookup_matching_slot(kml, start_addr, size); > + if (!mem) { > + fprintf(stderr, "%s: error finding slot\n", __func__); > + abort(); > + } FYI, this abort now triggers with the "isa-vga" device: $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga kvm_section_update_flags: error finding slot Aborted (core dumped) Any ideas how to fix this? Thomas
On 10/10/2017 11:06, Thomas Huth wrote: > On 11.09.2017 19:49, David Hildenbrand wrote: >> Let's properly align the sections first and bail out if we would ever >> get called with a memory section we don't know yet. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> accel/kvm/kvm-all.c | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index b677d1b13e..2ae459453d 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, >> static int kvm_section_update_flags(KVMMemoryListener *kml, >> MemoryRegionSection *section) >> { >> - hwaddr phys_addr = section->offset_within_address_space; >> - ram_addr_t size = int128_get64(section->size); >> - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); >> + hwaddr start_addr, size; >> + KVMSlot *mem; >> >> - if (mem == NULL) { >> + size = kvm_align_section(section, &start_addr); >> + if (!size) { >> return 0; >> - } else { >> - return kvm_slot_update_flags(kml, mem, section->mr); >> } >> + >> + mem = kvm_lookup_matching_slot(kml, start_addr, size); >> + if (!mem) { >> + fprintf(stderr, "%s: error finding slot\n", __func__); >> + abort(); >> + } > > FYI, this abort now triggers with the "isa-vga" device: > > $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga > kvm_section_update_flags: error finding slot > Aborted (core dumped) > > Any ideas how to fix this? Reverting, unless David has some time to look into it. Paolo
On 10.10.2017 15:29, Paolo Bonzini wrote: > On 10/10/2017 11:06, Thomas Huth wrote: >> On 11.09.2017 19:49, David Hildenbrand wrote: >>> Let's properly align the sections first and bail out if we would ever >>> get called with a memory section we don't know yet. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> accel/kvm/kvm-all.c | 18 ++++++++++++------ >>> 1 file changed, 12 insertions(+), 6 deletions(-) >>> >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>> index b677d1b13e..2ae459453d 100644 >>> --- a/accel/kvm/kvm-all.c >>> +++ b/accel/kvm/kvm-all.c >>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, >>> static int kvm_section_update_flags(KVMMemoryListener *kml, >>> MemoryRegionSection *section) >>> { >>> - hwaddr phys_addr = section->offset_within_address_space; >>> - ram_addr_t size = int128_get64(section->size); >>> - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); >>> + hwaddr start_addr, size; >>> + KVMSlot *mem; >>> >>> - if (mem == NULL) { >>> + size = kvm_align_section(section, &start_addr); >>> + if (!size) { >>> return 0; >>> - } else { >>> - return kvm_slot_update_flags(kml, mem, section->mr); >>> } >>> + >>> + mem = kvm_lookup_matching_slot(kml, start_addr, size); >>> + if (!mem) { >>> + fprintf(stderr, "%s: error finding slot\n", __func__); >>> + abort(); >>> + } >> >> FYI, this abort now triggers with the "isa-vga" device: >> >> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga >> kvm_section_update_flags: error finding slot >> Aborted (core dumped) >> >> Any ideas how to fix this? > > Reverting, unless David has some time to look into it. > > Paolo > Just returned from vacation, I'll have a look this week. Thanks for that nice reproducer Thomas, that should help a lot!
On 16.10.2017 09:16, David Hildenbrand wrote: > On 10.10.2017 15:29, Paolo Bonzini wrote: >> On 10/10/2017 11:06, Thomas Huth wrote: >>> On 11.09.2017 19:49, David Hildenbrand wrote: >>>> Let's properly align the sections first and bail out if we would ever >>>> get called with a memory section we don't know yet. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> accel/kvm/kvm-all.c | 18 ++++++++++++------ >>>> 1 file changed, 12 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>> index b677d1b13e..2ae459453d 100644 >>>> --- a/accel/kvm/kvm-all.c >>>> +++ b/accel/kvm/kvm-all.c >>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, >>>> static int kvm_section_update_flags(KVMMemoryListener *kml, >>>> MemoryRegionSection *section) >>>> { >>>> - hwaddr phys_addr = section->offset_within_address_space; >>>> - ram_addr_t size = int128_get64(section->size); >>>> - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); >>>> + hwaddr start_addr, size; >>>> + KVMSlot *mem; >>>> >>>> - if (mem == NULL) { >>>> + size = kvm_align_section(section, &start_addr); >>>> + if (!size) { >>>> return 0; >>>> - } else { >>>> - return kvm_slot_update_flags(kml, mem, section->mr); >>>> } >>>> + >>>> + mem = kvm_lookup_matching_slot(kml, start_addr, size); >>>> + if (!mem) { >>>> + fprintf(stderr, "%s: error finding slot\n", __func__); >>>> + abort(); >>>> + } >>> >>> FYI, this abort now triggers with the "isa-vga" device: >>> >>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga >>> kvm_section_update_flags: error finding slot >>> Aborted (core dumped) >>> >>> Any ideas how to fix this? >> >> Reverting, unless David has some time to look into it. >> >> Paolo >> > > Just returned from vacation, I'll have a look this week. > > Thanks for that nice reproducer Thomas, that should help a lot! FWIW, I've found the problem with the scripts/device-crash-test script, so you might want to run that, too, before submitting a new version. Thomas
On 16.10.2017 10:52, Thomas Huth wrote: > On 16.10.2017 09:16, David Hildenbrand wrote: >> On 10.10.2017 15:29, Paolo Bonzini wrote: >>> On 10/10/2017 11:06, Thomas Huth wrote: >>>> On 11.09.2017 19:49, David Hildenbrand wrote: >>>>> Let's properly align the sections first and bail out if we would ever >>>>> get called with a memory section we don't know yet. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> accel/kvm/kvm-all.c | 18 ++++++++++++------ >>>>> 1 file changed, 12 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>>> index b677d1b13e..2ae459453d 100644 >>>>> --- a/accel/kvm/kvm-all.c >>>>> +++ b/accel/kvm/kvm-all.c >>>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, >>>>> static int kvm_section_update_flags(KVMMemoryListener *kml, >>>>> MemoryRegionSection *section) >>>>> { >>>>> - hwaddr phys_addr = section->offset_within_address_space; >>>>> - ram_addr_t size = int128_get64(section->size); >>>>> - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); >>>>> + hwaddr start_addr, size; >>>>> + KVMSlot *mem; >>>>> >>>>> - if (mem == NULL) { >>>>> + size = kvm_align_section(section, &start_addr); >>>>> + if (!size) { >>>>> return 0; >>>>> - } else { >>>>> - return kvm_slot_update_flags(kml, mem, section->mr); >>>>> } >>>>> + >>>>> + mem = kvm_lookup_matching_slot(kml, start_addr, size); >>>>> + if (!mem) { >>>>> + fprintf(stderr, "%s: error finding slot\n", __func__); >>>>> + abort(); >>>>> + } >>>> >>>> FYI, this abort now triggers with the "isa-vga" device: >>>> >>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga >>>> kvm_section_update_flags: error finding slot >>>> Aborted (core dumped) >>>> >>>> Any ideas how to fix this? >>> >>> Reverting, unless David has some time to look into it. >>> >>> Paolo >>> >> >> Just returned from vacation, I'll have a look this week. >> >> Thanks for that nice reproducer Thomas, that should help a lot! > > FWIW, I've found the problem with the scripts/device-crash-test script, > so you might want to run that, too, before submitting a new version. > > Thomas > This one was easy to fix, there was just one place where the asssumption "only called with known sections" was wrong. See "memory: call log_start after region_add"
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index b677d1b13e..2ae459453d 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem, static int kvm_section_update_flags(KVMMemoryListener *kml, MemoryRegionSection *section) { - hwaddr phys_addr = section->offset_within_address_space; - ram_addr_t size = int128_get64(section->size); - KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size); + hwaddr start_addr, size; + KVMSlot *mem; - if (mem == NULL) { + size = kvm_align_section(section, &start_addr); + if (!size) { return 0; - } else { - return kvm_slot_update_flags(kml, mem, section->mr); } + + mem = kvm_lookup_matching_slot(kml, start_addr, size); + if (!mem) { + fprintf(stderr, "%s: error finding slot\n", __func__); + abort(); + } + + return kvm_slot_update_flags(kml, mem, section->mr); } static void kvm_log_start(MemoryListener *listener,
Let's properly align the sections first and bail out if we would ever get called with a memory section we don't know yet. Signed-off-by: David Hildenbrand <david@redhat.com> --- accel/kvm/kvm-all.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)