On Mon, Jul 1, 2024 at 4:30 PM Edgar E. Iglesias <edgar.igles...@gmail.com> wrote:
> > > On Mon, Jul 1, 2024 at 3:58 PM Edgar E. Iglesias <edgar.igles...@gmail.com> > wrote: > >> On Mon, Jul 1, 2024 at 2:55 PM Anthony PERARD <anthony.per...@vates.tech> >> wrote: >> >>> Hi all, >>> >>> Following this commit, a test which install Debian in a guest with OVMF >>> as firmware started to fail. QEMU exit with an error when GRUB is >>> running on the freshly installed Debian (I don't know if GRUB is >>> starting Linux or not). >>> The error is: >>> Bad ram offset ffffffffffffffff >>> >>> Some logs: >>> >>> http://logs.test-lab.xenproject.org/osstest/logs/186611/test-amd64-amd64-xl-qemuu-ovmf-amd64/info.html >>> >>> Any idea? Something is trying to do something with the address "-1" when >>> it shouldn't? >>> >>> >> Hi Anothny, >> >> Yes, it looks like something is calling qemu_get_ram_block() on something >> that isn't mapped. >> One possible path is in qemu_ram_block_from_host() but there may be >> others. >> >> The following patch may help. >> Any chance you could try to get a backtrace from QEMU when it failed? >> >> diff --git a/system/physmem.c b/system/physmem.c >> index 33d09f7571..2669c4dbbb 100644 >> --- a/system/physmem.c >> +++ b/system/physmem.c >> @@ -2277,6 +2277,9 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool >> round_offset, >> ram_addr_t ram_addr; >> RCU_READ_LOCK_GUARD(); >> ram_addr = xen_ram_addr_from_mapcache(ptr); >> + if (ram_addr == RAM_ADDR_INVALID) { >> + return NULL; >> + } >> block = qemu_get_ram_block(ram_addr); >> if (block) { >> *offset = ram_addr - block->offset; >> >> >> > One more thing, regarding this specific patch. I don't think we should > clear the > entire entry, the next field should be kept, otherwise we'll disconnect > following > mappings that will never be found again. IIUC, this could very well be > causing the problem you see. > > Does the following make sense? > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c > index 5f23b0adbe..e9df53c19d 100644 > --- a/hw/xen/xen-mapcache.c > +++ b/hw/xen/xen-mapcache.c > @@ -597,7 +597,14 @@ static void > xen_invalidate_map_cache_entry_unlocked(MapCache *mc, > pentry->next = entry->next; > g_free(entry); > } else { > - memset(entry, 0, sizeof *entry); > + /* Invalidate mapping. */ > + entry->paddr_index = 0; > + entry->vaddr_base = NULL; > + entry->size = 0; > + g_free(entry->valid_mapping); > + entry->valid_mapping = NULL; > + entry->flags = 0; > + /* Keep entry->next pointing to the rest of the list. */ > } > } > > > And here without double-freeing entry->valid_mapping: diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c index 5f23b0adbe..667807b3b6 100644 --- a/hw/xen/xen-mapcache.c +++ b/hw/xen/xen-mapcache.c @@ -597,7 +597,13 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc, pentry->next = entry->next; g_free(entry); } else { - memset(entry, 0, sizeof *entry); + /* Invalidate mapping. */ + entry->paddr_index = 0; + entry->vaddr_base = NULL; + entry->size = 0; + entry->valid_mapping = NULL; + entry->flags = 0; + /* Keep entry->next pointing to the rest of the list. */ } }
