From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43556) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBNUf-0007Nq-Km for qemu-devel@nongnu.org; Mon, 19 Aug 2013 07:19:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VBNUZ-000229-KQ for qemu-devel@nongnu.org; Mon, 19 Aug 2013 07:19:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33637) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBNUZ-000221-CY for qemu-devel@nongnu.org; Mon, 19 Aug 2013 07:19:23 -0400 Date: Mon, 19 Aug 2013 14:21:09 +0300 From: "Michael S. Tsirkin" Message-ID: <20130819112109.GD21552@redhat.com> References: <1376347400-21035-1-git-send-email-mst@redhat.com> <1376347400-21035-3-git-send-email-mst@redhat.com> <5211FC1F.9080000@redhat.com> <5211FE58.7080202@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5211FE58.7080202@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 2/2] loader: put FW CFG ROM files into RAM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: Peter Maydell , Anthony Liguori , pbonzini@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com On Mon, Aug 19, 2013 at 01:15:36PM +0200, Laszlo Ersek wrote: > 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 Exactly.