From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnLk-00071z-Tl for qemu-devel@nongnu.org; Tue, 17 Feb 2015 13:58:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNnLf-0002jn-JS for qemu-devel@nongnu.org; Tue, 17 Feb 2015 13:58:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53241) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnLf-0002jg-BY for qemu-devel@nongnu.org; Tue, 17 Feb 2015 13:58:19 -0500 Date: Tue, 17 Feb 2015 19:58:11 +0100 From: "Michael S. Tsirkin" Message-ID: <20150217185811.GA28982@redhat.com> References: <1424167517-21866-1-git-send-email-mst@redhat.com> <1424167517-21866-3-git-send-email-mst@redhat.com> <20150217145208.68fdd4c2@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150217145208.68fdd4c2@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/4] acpi-build: fix ACPI RAM management List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, Anthony Liguori , Richard Henderson On Tue, Feb 17, 2015 at 02:52:08PM +0100, Igor Mammedov wrote: > On Tue, 17 Feb 2015 11:05:39 +0100 > "Michael S. Tsirkin" wrote: > > > This fixes multiple issues around ACPI RAM management: > > > > RSDP and linker RAM aren't currently marked dirty > > on update, so they won't be migrated correctly. > > > > Let's handle all tables in the same way: set correct size (assert if > > too big), update, mark RAM dirty. > > > > This also drops assert checking that table size didn't change: table > > size is fundamentally dynamic and depends on hw configuration, > > just set the correct size and use that (memory core asserts if size is > > too large). > > > > This also means we can drop tracking table size, memory core does this > > for us now. > > > > Signed-off-by: Michael S. Tsirkin > > --- > > hw/i386/acpi-build.c | 43 +++++++++++++++++++++++-------------------- > > 1 file changed, 23 insertions(+), 20 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 1dfdf35..e78d6cb 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -1266,13 +1266,12 @@ typedef > > struct AcpiBuildState { > > /* Copy of table in RAM (for patching). */ > > ram_addr_t table_ram; > > - uint32_t table_size; > > /* Is table patched? */ > > uint8_t patched; > > PcGuestInfo *guest_info; > > void *rsdp; > > + ram_addr_t rsdp_ram; > > ram_addr_t linker_ram; > > - uint32_t linker_size; > > } AcpiBuildState; > > > > static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) > > @@ -1455,6 +1454,17 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > > g_array_free(table_offsets, true); > > } > > > > +static void acpi_ram_update(ram_addr_t ram, GArray *data) > > +{ > > + uint32_t size = acpi_data_len(data); > > + > > + /* Make sure RAM size is correct - in case it got changed e.g. by migration */ > > + qemu_ram_resize(ram, size, &error_abort); > > + > > + memcpy(qemu_get_ram_ptr(ram), data->data, size); > > + cpu_physical_memory_set_dirty_range_nocode(ram, size); > > +} > > + > > static void acpi_build_update(void *build_opaque, uint32_t offset) > > { > > AcpiBuildState *build_state = build_opaque; > > @@ -1470,21 +1480,15 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > > > > acpi_build(build_state->guest_info, &tables); > > > > - assert(acpi_data_len(tables.table_data) == build_state->table_size); > > + acpi_ram_update(build_state->table_ram, tables.table_data); > > > > - /* Make sure RAM size is correct - in case it got changed by migration */ > > - qemu_ram_resize(build_state->table_ram, build_state->table_size, > > - &error_abort); > > - > > - memcpy(qemu_get_ram_ptr(build_state->table_ram), tables.table_data->data, > > - build_state->table_size); > > - memcpy(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp)); > > - memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data, > > - build_state->linker_size); > > - > > - cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram, > > - build_state->table_size); > > + if (build_state->rsdp) { > > + memcpy(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp)); > > + } else { > > + acpi_ram_update(build_state->rsdp_ram, tables.rsdp); > > + } > > > > + acpi_ram_update(build_state->linker_ram, tables.linker); > > acpi_build_tables_cleanup(&tables, true); > > } > > > > @@ -1545,11 +1549,9 @@ void acpi_setup(PcGuestInfo *guest_info) > > ACPI_BUILD_TABLE_FILE, > > ACPI_BUILD_TABLE_MAX_SIZE); > > assert(build_state->table_ram != RAM_ADDR_MAX); > > - build_state->table_size = acpi_data_len(tables.table_data); > > > > build_state->linker_ram = > > acpi_add_rom_blob(build_state, tables.linker, "etc/table-loader", 0); > > - build_state->linker_size = acpi_data_len(tables.linker); > > > > fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > @@ -1564,10 +1566,11 @@ void acpi_setup(PcGuestInfo *guest_info) > > acpi_build_update, build_state, > > tables.rsdp->data, acpi_data_len(tables.rsdp)); > > build_state->rsdp = tables.rsdp->data; > > + build_state->rsdp_ram = (ram_addr_t)-1; > I'd drop this and > > > } else { > > - build_state->rsdp = qemu_get_ram_ptr( > > - acpi_add_rom_blob(build_state, tables.rsdp, ACPI_BUILD_RSDP_FILE, 0) > > - ); > > + build_state->rsdp = NULL; > this as unnecessary We've been here, I prefer explict initialization. > > + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, > > + ACPI_BUILD_RSDP_FILE, 0); > > } > > > > qemu_register_reset(acpi_build_reset, build_state); > > Otherwise looks as a very nice improvement of current mess