Message ID | 1403036041-13188-1-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Jun 17, 2014 at 10:14:01PM +0200, Igor Mammedov wrote: > linker and RSDP tables are build only once, so if later > during rebuild sizes of ACPI tables change pointers will > be patched incorrectly due to wrong offsets. > > To fix it rebuild linker and RSDP tables along with > the rest of ACPI tables so that they would have correct > offsets. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> If you are adding a new blob you are breaking cross-version migration. Need to handle that depending on pc version as appropriate. > --- > hw/i386/acpi-build.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index fab25ad..8301bb4 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1862,6 +1862,10 @@ struct AcpiBuildState { > /* Copy of table in RAM (for patching). */ > uint8_t *table_ram; > uint32_t table_size; > + uint8_t *linker_ram; > + uint32_t linker_size; > + uint8_t *rsdp_ram; > + uint32_t rsdp_size; > /* Is table patched? */ > uint8_t patched; > PcGuestInfo *guest_info; > @@ -1998,6 +2002,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > assert(acpi_data_len(tables.table_data) == build_state->table_size); > memcpy(build_state->table_ram, tables.table_data->data, > build_state->table_size); > + memcpy(build_state->linker_ram, tables.linker->data, > + build_state->linker_size); > + memcpy(build_state->rsdp_ram, tables.rsdp->data, > + build_state->rsdp_size); > > acpi_build_tables_cleanup(&tables, true); > } > @@ -2060,14 +2068,13 @@ void acpi_setup(PcGuestInfo *guest_info) > ACPI_BUILD_TABLE_FILE); > build_state->table_size = acpi_data_len(tables.table_data); > > - acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader"); > + build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker, > + "etc/table-loader"); > + build_state->linker_size = acpi_data_len(tables.linker); > > - /* > - * RSDP is small so it's easy to keep it immutable, no need to > - * bother with ROM blobs. > - */ > - fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE, > - tables.rsdp->data, acpi_data_len(tables.rsdp)); > + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, > + ACPI_BUILD_RSDP_FILE); > + build_state->rsdp_size = acpi_data_len(tables.rsdp); > > qemu_register_reset(acpi_build_reset, build_state); > acpi_build_reset(build_state); > -- > 1.9.3
On Tue, 2014-06-17 at 23:19 +0300, Michael S. Tsirkin wrote: > On Tue, Jun 17, 2014 at 10:14:01PM +0200, Igor Mammedov wrote: > > linker and RSDP tables are build only once, so if later > > during rebuild sizes of ACPI tables change pointers will > > be patched incorrectly due to wrong offsets. > > > > To fix it rebuild linker and RSDP tables along with > > the rest of ACPI tables so that they would have correct > > offsets. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > If you are adding a new blob you are breaking cross-version migration. > Need to handle that depending on pc version as appropriate. Besides the above comment, Tested-by: Marcel Apfelbaum <marcel.a@redhat.com> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> Thanks, Marcel > > > --- > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index fab25ad..8301bb4 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -1862,6 +1862,10 @@ struct AcpiBuildState { > > /* Copy of table in RAM (for patching). */ > > uint8_t *table_ram; > > uint32_t table_size; > > + uint8_t *linker_ram; > > + uint32_t linker_size; > > + uint8_t *rsdp_ram; > > + uint32_t rsdp_size; > > /* Is table patched? */ > > uint8_t patched; > > PcGuestInfo *guest_info; > > @@ -1998,6 +2002,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > > assert(acpi_data_len(tables.table_data) == build_state->table_size); > > memcpy(build_state->table_ram, tables.table_data->data, > > build_state->table_size); > > + memcpy(build_state->linker_ram, tables.linker->data, > > + build_state->linker_size); > > + memcpy(build_state->rsdp_ram, tables.rsdp->data, > > + build_state->rsdp_size); > > > > acpi_build_tables_cleanup(&tables, true); > > } > > @@ -2060,14 +2068,13 @@ void acpi_setup(PcGuestInfo *guest_info) > > ACPI_BUILD_TABLE_FILE); > > build_state->table_size = acpi_data_len(tables.table_data); > > > > - acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader"); > > + build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker, > > + "etc/table-loader"); > > + build_state->linker_size = acpi_data_len(tables.linker); > > > > - /* > > - * RSDP is small so it's easy to keep it immutable, no need to > > - * bother with ROM blobs. > > - */ > > - fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE, > > - tables.rsdp->data, acpi_data_len(tables.rsdp)); > > + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, > > + ACPI_BUILD_RSDP_FILE); > > + build_state->rsdp_size = acpi_data_len(tables.rsdp); > > > > qemu_register_reset(acpi_build_reset, build_state); > > acpi_build_reset(build_state); > > -- > > 1.9.3
On Wed, Jun 18, 2014 at 12:04:31PM +0300, Marcel Apfelbaum wrote: > On Tue, 2014-06-17 at 23:19 +0300, Michael S. Tsirkin wrote: > > On Tue, Jun 17, 2014 at 10:14:01PM +0200, Igor Mammedov wrote: > > > linker and RSDP tables are build only once, so if later > > > during rebuild sizes of ACPI tables change pointers will > > > be patched incorrectly due to wrong offsets. > > > > > > To fix it rebuild linker and RSDP tables along with > > > the rest of ACPI tables so that they would have correct > > > offsets. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > If you are adding a new blob you are breaking cross-version migration. > > Need to handle that depending on pc version as appropriate. > Besides the above comment, > Tested-by: Marcel Apfelbaum <marcel.a@redhat.com> > Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Thanks, > Marcel This will need a bit more motivation to go in than is provided here. > > > > > --- > > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index fab25ad..8301bb4 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -1862,6 +1862,10 @@ struct AcpiBuildState { > > > /* Copy of table in RAM (for patching). */ > > > uint8_t *table_ram; > > > uint32_t table_size; > > > + uint8_t *linker_ram; > > > + uint32_t linker_size; > > > + uint8_t *rsdp_ram; > > > + uint32_t rsdp_size; > > > /* Is table patched? */ > > > uint8_t patched; > > > PcGuestInfo *guest_info; > > > @@ -1998,6 +2002,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > > > assert(acpi_data_len(tables.table_data) == build_state->table_size); > > > memcpy(build_state->table_ram, tables.table_data->data, > > > build_state->table_size); > > > + memcpy(build_state->linker_ram, tables.linker->data, > > > + build_state->linker_size); > > > + memcpy(build_state->rsdp_ram, tables.rsdp->data, > > > + build_state->rsdp_size); > > > > > > acpi_build_tables_cleanup(&tables, true); > > > } > > > @@ -2060,14 +2068,13 @@ void acpi_setup(PcGuestInfo *guest_info) > > > ACPI_BUILD_TABLE_FILE); > > > build_state->table_size = acpi_data_len(tables.table_data); > > > > > > - acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader"); > > > + build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker, > > > + "etc/table-loader"); > > > + build_state->linker_size = acpi_data_len(tables.linker); > > > > > > - /* > > > - * RSDP is small so it's easy to keep it immutable, no need to > > > - * bother with ROM blobs. > > > - */ > > > - fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE, > > > - tables.rsdp->data, acpi_data_len(tables.rsdp)); > > > + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, > > > + ACPI_BUILD_RSDP_FILE); > > > + build_state->rsdp_size = acpi_data_len(tables.rsdp); > > > > > > qemu_register_reset(acpi_build_reset, build_state); > > > acpi_build_reset(build_state); > > > -- > > > 1.9.3 > >
On Wed, 18 Jun 2014 12:19:10 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jun 18, 2014 at 12:04:31PM +0300, Marcel Apfelbaum wrote: > > On Tue, 2014-06-17 at 23:19 +0300, Michael S. Tsirkin wrote: > > > On Tue, Jun 17, 2014 at 10:14:01PM +0200, Igor Mammedov wrote: > > > > linker and RSDP tables are build only once, so if later > > > > during rebuild sizes of ACPI tables change pointers will > > > > be patched incorrectly due to wrong offsets. > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > the rest of ACPI tables so that they would have correct > > > > offsets. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > If you are adding a new blob you are breaking cross-version migration. > > > Need to handle that depending on pc version as appropriate. > > Besides the above comment, > > Tested-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > Thanks, > > Marcel > > This will need a bit more motivation to go in than is > provided here. I'll address comment above and also will dig up example why it was needed in a month old dynamic ACPI tables series. > > > > > > > > --- > > > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index fab25ad..8301bb4 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -1862,6 +1862,10 @@ struct AcpiBuildState { > > > > /* Copy of table in RAM (for patching). */ > > > > uint8_t *table_ram; > > > > uint32_t table_size; > > > > + uint8_t *linker_ram; > > > > + uint32_t linker_size; > > > > + uint8_t *rsdp_ram; > > > > + uint32_t rsdp_size; > > > > /* Is table patched? */ > > > > uint8_t patched; > > > > PcGuestInfo *guest_info; > > > > @@ -1998,6 +2002,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) > > > > assert(acpi_data_len(tables.table_data) == build_state->table_size); > > > > memcpy(build_state->table_ram, tables.table_data->data, > > > > build_state->table_size); > > > > + memcpy(build_state->linker_ram, tables.linker->data, > > > > + build_state->linker_size); > > > > + memcpy(build_state->rsdp_ram, tables.rsdp->data, > > > > + build_state->rsdp_size); > > > > > > > > acpi_build_tables_cleanup(&tables, true); > > > > } > > > > @@ -2060,14 +2068,13 @@ void acpi_setup(PcGuestInfo *guest_info) > > > > ACPI_BUILD_TABLE_FILE); > > > > build_state->table_size = acpi_data_len(tables.table_data); > > > > > > > > - acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader"); > > > > + build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker, > > > > + "etc/table-loader"); > > > > + build_state->linker_size = acpi_data_len(tables.linker); > > > > > > > > - /* > > > > - * RSDP is small so it's easy to keep it immutable, no need to > > > > - * bother with ROM blobs. > > > > - */ > > > > - fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE, > > > > - tables.rsdp->data, acpi_data_len(tables.rsdp)); > > > > + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, > > > > + ACPI_BUILD_RSDP_FILE); > > > > + build_state->rsdp_size = acpi_data_len(tables.rsdp); > > > > > > > > qemu_register_reset(acpi_build_reset, build_state); > > > > acpi_build_reset(build_state); > > > > -- > > > > 1.9.3 > > > > >
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index fab25ad..8301bb4 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -1862,6 +1862,10 @@ struct AcpiBuildState { /* Copy of table in RAM (for patching). */ uint8_t *table_ram; uint32_t table_size; + uint8_t *linker_ram; + uint32_t linker_size; + uint8_t *rsdp_ram; + uint32_t rsdp_size; /* Is table patched? */ uint8_t patched; PcGuestInfo *guest_info; @@ -1998,6 +2002,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset) assert(acpi_data_len(tables.table_data) == build_state->table_size); memcpy(build_state->table_ram, tables.table_data->data, build_state->table_size); + memcpy(build_state->linker_ram, tables.linker->data, + build_state->linker_size); + memcpy(build_state->rsdp_ram, tables.rsdp->data, + build_state->rsdp_size); acpi_build_tables_cleanup(&tables, true); } @@ -2060,14 +2068,13 @@ void acpi_setup(PcGuestInfo *guest_info) ACPI_BUILD_TABLE_FILE); build_state->table_size = acpi_data_len(tables.table_data); - acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader"); + build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker, + "etc/table-loader"); + build_state->linker_size = acpi_data_len(tables.linker); - /* - * RSDP is small so it's easy to keep it immutable, no need to - * bother with ROM blobs. - */ - fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE, - tables.rsdp->data, acpi_data_len(tables.rsdp)); + build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp, + ACPI_BUILD_RSDP_FILE); + build_state->rsdp_size = acpi_data_len(tables.rsdp); qemu_register_reset(acpi_build_reset, build_state); acpi_build_reset(build_state);
linker and RSDP tables are build only once, so if later during rebuild sizes of ACPI tables change pointers will be patched incorrectly due to wrong offsets. To fix it rebuild linker and RSDP tables along with the rest of ACPI tables so that they would have correct offsets. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-build.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)