On 08/19/13 13:06, Laszlo Ersek wrote: > On 08/13/13 00:43, Michael S. Tsirkin wrote:
>> @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir, >> if (rom->fw_file && fw_cfg) { >> const char *basename; >> char fw_file_name[56]; >> + void *data; >> >> basename = strrchr(rom->fw_file, '/'); >> if (basename) { >> @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir, >> } >> snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir, >> basename); >> - fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize); >> snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name); >> + >> + if (rom_file_in_ram) { >> + data = rom_set_mr(rom, OBJECT(fw_cfg), devpath); >> + } else { >> + data = rom->data; >> + } >> + >> + fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize); > > This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the > ROM contents in the qemu process twice -- once in "rom->data" (allocated > just a bit higher up, not shown in context), and in the new RAMBlock. > > This is no bug of course, I'm just wondering if we could drop/repoint > "rom->data" in this case. > >> } else { >> snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr); >> } >> @@ -731,7 +762,12 @@ static void rom_reset(void *unused) >> if (rom->data == NULL) { >> continue; >> } >> - cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize); >> + if (rom->mr) { >> + void *host = memory_region_get_ram_ptr(rom->mr); >> + memcpy(host, rom->data, rom->datasize); >> + } else { >> + cpu_physical_memory_write_rom(rom->addr, rom->data, >> rom->datasize); >> + } > > Hmmm. Why is this (ie. the pre-patch resetting) necessary at all? > > Is this due to the writeability of fw_cfg files via the ioport > (fw_cfg_write())? I think that modifies "rom->data" unconditionally > (which is currently kept separate from the RAMBlock, see above). > > So, regarding the patched version: > - not sure if the RAMBlock can change at all -- it is neither mapped > into guest-phys address space, nor does fw_cfg_write() touch it, > - *if* the guest modifies the contents under "rom->addr", via > fw_cfg_write(), then the hva-space memcpy() is insufficient. Sorry, I'm wrong here. The patched rom_add_file() ensures that fw_cfg_write() modifies the correct backing store. Also, we need to keep "rom->data" around even if "rom_file_in_ram" is set, because that's where we restore the RAMBlock contents from, in case of a reset. Laszlo