[Qemu-devel] [PATCH/RFC] clear bss memory of ROMS

[Christian Borntraeger <borntraeger@de.ibm.com>]

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 <borntraeger@de.ibm.com>
>>> ---
>>>  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 <borntraeger@de.ibm.com>
[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);