[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM |
Date: |
Mon, 19 Aug 2013 13:15:36 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130806 Thunderbird/17.0.8 |
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), "/address@hidden", 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
[Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM, Michael S. Tsirkin, 2013/08/12
Re: [Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration, Michael S. Tsirkin, 2013/08/18
Re: [Qemu-devel] [PATCH v3 0/2] future proof rom loading for cross versiom migration, Laszlo Ersek, 2013/08/19