From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8xGU-0000av-Rc for qemu-devel@nongnu.org; Mon, 12 Aug 2013 14:54:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V8xGJ-0000wT-F0 for qemu-devel@nongnu.org; Mon, 12 Aug 2013 14:54:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20377) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8xGJ-0000wG-62 for qemu-devel@nongnu.org; Mon, 12 Aug 2013 14:54:39 -0400 Date: Mon, 12 Aug 2013 21:56:13 +0300 From: "Michael S. Tsirkin" Message-ID: <20130812185612.GA1616@redhat.com> References: <1376310634-20778-1-git-send-email-mst@redhat.com> <1376310634-20778-3-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2 for-1.6 v2 2/2] loader: put FW CFG ROM files into RAM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: pbonzini@redhat.com, Anthony Liguori , Laszlo Ersek , qemu-devel@nongnu.org, Gerd Hoffmann On Mon, Aug 12, 2013 at 07:37:21PM +0100, Peter Maydell wrote: > On 12 August 2013 19:16, Michael S. Tsirkin wrote: > > +static void *rom_set_mr(Rom *rom, Object *owner, const char *name) > > +{ > > + /* > > + * Migration code expects that all RAM blocks are full pages. > > + * Round MR size up to satisfy this condition. > > + */ > > + unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size); > > + void *data = g_malloc0(size); > > + > > + memcpy(data, rom->data, rom->datasize); > > + > > + rom->mr = g_malloc(sizeof(*rom->mr)); > > + memory_region_init_ram_ptr(rom->mr, owner, name, size, data); > > + memory_region_set_readonly(rom->mr, true); > > + vmstate_register_ram_global(rom->mr); > > So having thought about this a little I think the right answer > here is "don't use memory_region_init_ram_ptr()". At the moment > in-tree we have five users of this function: > > hw/display/g364fb.c > hw/i386/kvm/pci-assign.c > hw/misc/ivshmem.c > hw/misc/vfio.c > target-ppc/kvm.c > > The last four of these all absolutely have to have the guest > use a specific host pointer, typically the result of mmap()ing > something [shared file, PCI device, KVM_ALLOCATE_RMA fd, etc]. > The first one I think should be converted to use > memory_region_init_ram() instead, because it doesn't need > to use a particular buffer. > > Similarly, what you're trying to do here doesn't require > that the guest sees any specific host pointer, so you should > just use memory_region_init_ram(). I was concerned that we are wasting resources here. In particular, huge page memory might get allocated and there's no need for it as it's never mapped into guest. Still if Paolo is OK with this too, I'll switch, and do + memory_region_init_ram_ptr(...) + data = memory_region_get_ram_ptr(rom->mr); Paolo could you please confirm? > > We should add an assert to the _init_ram_ptr functions that > checks that the size is OK, as well. At least for pci-assign and vfio it's the wrong thing to do - they block migration so we don't need the ram to be a multiple of migration page size. > I seem to recall having a conversation with Paolo along these > lines a few months back (we fixed the exynos devices which > were incorrectly using the _ram_ptr function); he can correct > me if I'm off-base here. > > -- PMM