From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WE42H-0005vr-I4 for qemu-devel@nongnu.org; Thu, 13 Feb 2014 16:41:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WE428-0008D6-Jc for qemu-devel@nongnu.org; Thu, 13 Feb 2014 16:41:33 -0500 Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:57065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WE428-0008CD-Ah for qemu-devel@nongnu.org; Thu, 13 Feb 2014 16:41:24 -0500 Received: from /spool/local by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 13 Feb 2014 21:41:22 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 744A017D8059 for ; Thu, 13 Feb 2014 21:41:47 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s1DLf8Ux62849248 for ; Thu, 13 Feb 2014 21:41:08 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s1DLfJB0011457 for ; Thu, 13 Feb 2014 16:41:19 -0500 Message-ID: <52FD3BFE.1070701@de.ibm.com> Date: Thu, 13 Feb 2014 22:41:18 +0100 From: Christian Borntraeger MIME-Version: 1.0 References: <1392283031-40129-1-git-send-email-borntraeger@de.ibm.com> <1392283031-40129-2-git-send-email-borntraeger@de.ibm.com> <52FCE1AA.4080509@twiddle.net> <52FD1F85.8010600@de.ibm.com> In-Reply-To: <52FD1F85.8010600@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] [PATCH/RFC] clear bss memory of ROMS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , Anthony Liguori , Peter Maydell Cc: Cornelia Huck , Jens Freimann , Alexander Graf , =?ISO-8859-1?Q?Andreas_F=E4rber?= , qemu-devel On 13/02/14 20:39, Christian Borntraeger wrote: > On 13/02/14 16:15, Richard Henderson wrote: >> On 02/13/2014 01:17 AM, Christian Borntraeger wrote: >>> The current code does not initialize next_idx as the qemu >>> elf loader does not zero the bss section. >>> Make the initialization explicit. >>> >>> Signed-off-by: Christian Borntraeger >>> --- >>> pc-bios/s390-ccw/virtio.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c >>> index 4d6e48f..a46914d 100644 >>> --- a/pc-bios/s390-ccw/virtio.c >>> +++ b/pc-bios/s390-ccw/virtio.c >>> @@ -124,6 +124,7 @@ static void vring_init(struct vring *vr, unsigned int num, void *p, >>> vr->used->flags = VRING_USED_F_NO_NOTIFY; >>> vr->used->idx = 0; >>> vr->used_idx = 0; >>> + vr->next_idx = 0; >>> >>> debug_print_addr("init vr", vr); >>> } >>> >> >> FWIW, I believe that rom_reset needs to do this re-zeroing of the bss. >> That seems to be the only place we don't take care for datasize != romsize. >> > > Indeed, initializing the data as in my patches isnt wrong (and allows to move > that structures around e.g. from a global variable to stack), so it still makes > sense to apply both patches, but the main problem was that the bss section is > not cleared on reset. > > So we need to memset from rom->data+rom->datasize to rom->data+rom->romsize > to avoid more of these kind of problems in an add-on patch. To correct myself. Actually only Patch 2/3 would be fixed by zeroing the bss. Patch 1/3 is still necessary, since the bios creates the virtqueue not in bss but in real memory. Still, bss clearing seems like a good idea, so what about something like the following: loader: reset bss sections of ROMS The bss section of ELF roms must be zeroed on reset. Signed-off-by: Christian Borntraeger [cborntra@r17lp39 qemu]$ git diff diff --git a/exec.c b/exec.c index b69fd29..f0f6a94 100644 --- a/exec.c +++ b/exec.c @@ -2097,6 +2097,30 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf, address_space_rw(&address_space_memory, addr, buf, len, is_write); } +void cpu_physical_memory_clear_rom(AddressSpace *as, hwaddr addr, size_t len) +{ + hwaddr l; + uint8_t *ptr; + hwaddr addr1; + MemoryRegion *mr; + + while (len > 0) { + l = len; + mr = address_space_translate(as, addr, &addr1, &l, true); + + if (!(memory_region_is_ram(mr) || + memory_region_is_romd(mr))) { + /* do nothing */ + } else { + addr1 += memory_region_get_ram_addr(mr); + ptr = qemu_get_ram_ptr(addr1); + memset(ptr, 0, l); + } + len -= l; + addr += l; + } +} + enum write_rom_type { WRITE_DATA, FLUSH_CACHE, diff --git a/hw/core/loader.c b/hw/core/loader.c index e1a8318..7998a3e 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -786,13 +786,20 @@ static void rom_reset(void *unused) if (rom->fw_file) { continue; } - if (rom->data == NULL) { - continue; - } if (rom->mr) { void *host = memory_region_get_ram_ptr(rom->mr); + memset(host + rom->datasize, 0, rom->romsize - rom->datasize); + if (rom->data == NULL) { + continue; + } memcpy(host, rom->data, rom->datasize); } else { + cpu_physical_memory_clear_rom(&address_space_memory, + rom->addr + rom->datasize, + rom->romsize - rom->datasize); + if (rom->data == NULL) { + continue; + } cpu_physical_memory_write_rom(&address_space_memory, rom->addr, rom->data, rom->datasize); } diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index a21b65a..948de83 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -108,6 +108,7 @@ void stl_phys(AddressSpace *as, hwaddr addr, uint32_t val); void stq_phys(AddressSpace *as, hwaddr addr, uint64_t val); #endif +void cpu_physical_memory_clear_rom(AddressSpace *as, hwaddr addr, size_t len); void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr, const uint8_t *buf, int len); void cpu_flush_icache_range(hwaddr start, int len);