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 <borntrae...@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 <borntrae...@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);
