[Qemu-devel] [PATCH 0/4] acpi: fix RSDP and linker memory management

[Qemu-devel] [PATCH 0/4] acpi: fix RSDP and linker memory management

[Michael S. Tsirkin]

This cleans up handling for rsdp and linker tables, which many people found
confusing, and fixes bugs around linker and rsdp migration.

Paolo, could you review/ack exec.c patch please?

Michael S. Tsirkin (4):
  exec: round up size on MR resize
  acpi-build: fix RSDP/linker RAM migration
  acpi: has_immutable_rsdp->!rsdp_in_ram
  acpi-build: simplify rsdp management for legacy

 include/hw/i386/pc.h |  2 +-
 exec.c               |  2 ++
 hw/i386/acpi-build.c | 55 ++++++++++++++++++++++++++++------------------------
 hw/i386/pc_piix.c    |  6 +++---
 hw/i386/pc_q35.c     |  6 +++---
 5 files changed, 39 insertions(+), 32 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/4] exec: round up size on MR resize

[Michael S. Tsirkin]

Block size must fundamentally be a multiple of target page size.
Aligning automatically removes need to worry about the alignment
from callers.

Note: the only caller of qemu_ram_resize (acpi) already happens to have
size padded to a power of 2, but we would like to drop the padding in
ACPI core, and don't want to expose target page size knowledge to ACPI.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/exec.c b/exec.c
index 6b79ad1..2433406 100644
--- a/exec.c
+++ b/exec.c
@@ -1298,6 +1298,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
 
     assert(block);
 
+    newsize = TARGET_PAGE_ALIGN(newsize);
+
     if (block->used_length == newsize) {
         return 0;
     }
-- 
MST

Re: [Qemu-devel] [PATCH 1/4] exec: round up size on MR resize

[Igor Mammedov]

On Tue, 17 Feb 2015 11:05:36 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Block size must fundamentally be a multiple of target page size.
> Aligning automatically removes need to worry about the alignment
> from callers.
> 
> Note: the only caller of qemu_ram_resize (acpi) already happens to have
> size padded to a power of 2, but we would like to drop the padding in
> ACPI core, and don't want to expose target page size knowledge to ACPI.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 6b79ad1..2433406 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1298,6 +1298,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
>  
>      assert(block);
>  
> +    newsize = TARGET_PAGE_ALIGN(newsize);
> +
>      if (block->used_length == newsize) {
>          return 0;
>      }

Re: [Qemu-devel] [PATCH 1/4] exec: round up size on MR resize

[Paolo Bonzini]

On 17/02/2015 14:45, Igor Mammedov wrote:
> On Tue, 17 Feb 2015 11:05:36 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> Block size must fundamentally be a multiple of target page size.
>> Aligning automatically removes need to worry about the alignment
>> from callers.
>>
>> Note: the only caller of qemu_ram_resize (acpi) already happens to have
>> size padded to a power of 2, but we would like to drop the padding in
>> ACPI core, and don't want to expose target page size knowledge to ACPI.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
>> ---
>>  exec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/exec.c b/exec.c
>> index 6b79ad1..2433406 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1298,6 +1298,8 @@ int qemu_ram_resize(ram_addr_t base, ram_addr_t newsize, Error **errp)
>>  
>>      assert(block);
>>  
>> +    newsize = TARGET_PAGE_ALIGN(newsize);
>> +
>>      if (block->used_length == newsize) {
>>          return 0;
>>      }
> 

Acked-by: Paolo Bonzini <ponzini@redhat.com>

[Qemu-devel] [PATCH 2/4] acpi-build: fix ACPI RAM management

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 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;
     } 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;
+        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);
-- 
MST

Re: [Qemu-devel] [PATCH 2/4] acpi-build: fix ACPI RAM management

[Igor Mammedov]

On Tue, 17 Feb 2015 11:05:39 +0100
"Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com>
> ---
>  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 

> +        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

Re: [Qemu-devel] [PATCH 2/4] acpi-build: fix ACPI RAM management

[Michael S. Tsirkin]

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" <mst@redhat.com> 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 <mst@redhat.com>
> > ---
> >  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

[Qemu-devel] [PATCH 3/4] acpi: has_immutable_rsdp->!rsdp_in_ram

[Michael S. Tsirkin]

As comment in acpi-build.c notes, RSDP is not really immutable.  So it's
really a question of whether it's in RAM, name the variable accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/i386/pc.h | 2 +-
 hw/i386/acpi-build.c | 2 +-
 hw/i386/pc_piix.c    | 6 +++---
 hw/i386/pc_q35.c     | 6 +++---
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b0a80cf..0c595c5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -104,7 +104,7 @@ struct PcGuestInfo {
     int legacy_acpi_table_size;
     bool has_acpi_build;
     bool has_reserved_memory;
-    bool has_immutable_rsdp;
+    bool rsdp_in_ram;
 };
 
 /* parallel.c */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e78d6cb..ffa3f00 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1556,7 +1556,7 @@ void acpi_setup(PcGuestInfo *guest_info)
     fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
-    if (guest_info->has_immutable_rsdp) {
+    if (!guest_info->rsdp_in_ram) {
         /*
          * Keep for compatibility with old machine types.
          * Though RSDP is small, its contents isn't immutable, so
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e586c7b..13ff561 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -60,7 +60,7 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
 static bool has_acpi_build = true;
-static bool has_immutable_rsdp;
+static bool rsdp_in_ram = true;
 static int legacy_acpi_table_size;
 static bool smbios_defaults = true;
 static bool smbios_legacy_mode;
@@ -169,7 +169,7 @@ static void pc_init1(MachineState *machine,
 
     guest_info->isapc_ram_fw = !pci_enabled;
     guest_info->has_reserved_memory = has_reserved_memory;
-    guest_info->has_immutable_rsdp = has_immutable_rsdp;
+    guest_info->rsdp_in_ram = rsdp_in_ram;
 
     if (smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -312,7 +312,7 @@ static void pc_init_pci(MachineState *machine)
 
 static void pc_compat_2_2(MachineState *machine)
 {
-    has_immutable_rsdp = true;
+    rsdp_in_ram = false;
     x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6151f2f..c0f21fe 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,7 +50,7 @@
 #define MAX_SATA_PORTS     6
 
 static bool has_acpi_build = true;
-static bool has_immutable_rsdp;
+static bool rsdp_in_ram = true;
 static bool smbios_defaults = true;
 static bool smbios_legacy_mode;
 static bool smbios_uuid_encoded = true;
@@ -155,7 +155,7 @@ static void pc_q35_init(MachineState *machine)
     guest_info->isapc_ram_fw = false;
     guest_info->has_acpi_build = has_acpi_build;
     guest_info->has_reserved_memory = has_reserved_memory;
-    guest_info->has_immutable_rsdp = has_immutable_rsdp;
+    guest_info->rsdp_in_ram = rsdp_in_ram;
 
     /* Migration was not supported in 2.0 for Q35, so do not bother
      * with this hack (see hw/i386/acpi-build.c).
@@ -291,7 +291,7 @@ static void pc_q35_init(MachineState *machine)
 
 static void pc_compat_2_2(MachineState *machine)
 {
-    has_immutable_rsdp = true;
+    rsdp_in_ram = false;
     x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
     x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);
-- 
MST

Re: [Qemu-devel] [PATCH 3/4] acpi: has_immutable_rsdp->!rsdp_in_ram

[Igor Mammedov]

On Tue, 17 Feb 2015 11:05:42 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> As comment in acpi-build.c notes, RSDP is not really immutable.  So it's
> really a question of whether it's in RAM, name the variable accordingly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/i386/pc.h | 2 +-
>  hw/i386/acpi-build.c | 2 +-
>  hw/i386/pc_piix.c    | 6 +++---
>  hw/i386/pc_q35.c     | 6 +++---
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b0a80cf..0c595c5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -104,7 +104,7 @@ struct PcGuestInfo {
>      int legacy_acpi_table_size;
>      bool has_acpi_build;
>      bool has_reserved_memory;
> -    bool has_immutable_rsdp;
> +    bool rsdp_in_ram;
>  };
>  
>  /* parallel.c */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e78d6cb..ffa3f00 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1556,7 +1556,7 @@ void acpi_setup(PcGuestInfo *guest_info)
>      fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> -    if (guest_info->has_immutable_rsdp) {
> +    if (!guest_info->rsdp_in_ram) {
>          /*
>           * Keep for compatibility with old machine types.
>           * Though RSDP is small, its contents isn't immutable, so
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e586c7b..13ff561 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -60,7 +60,7 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>  static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>  
>  static bool has_acpi_build = true;
> -static bool has_immutable_rsdp;
> +static bool rsdp_in_ram = true;
>  static int legacy_acpi_table_size;
>  static bool smbios_defaults = true;
>  static bool smbios_legacy_mode;
> @@ -169,7 +169,7 @@ static void pc_init1(MachineState *machine,
>  
>      guest_info->isapc_ram_fw = !pci_enabled;
>      guest_info->has_reserved_memory = has_reserved_memory;
> -    guest_info->has_immutable_rsdp = has_immutable_rsdp;
> +    guest_info->rsdp_in_ram = rsdp_in_ram;
>  
>      if (smbios_defaults) {
>          MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -312,7 +312,7 @@ static void pc_init_pci(MachineState *machine)
>  
>  static void pc_compat_2_2(MachineState *machine)
>  {
> -    has_immutable_rsdp = true;
> +    rsdp_in_ram = false;
>      x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
>      x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
>      x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 6151f2f..c0f21fe 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -50,7 +50,7 @@
>  #define MAX_SATA_PORTS     6
>  
>  static bool has_acpi_build = true;
> -static bool has_immutable_rsdp;
> +static bool rsdp_in_ram = true;
>  static bool smbios_defaults = true;
>  static bool smbios_legacy_mode;
>  static bool smbios_uuid_encoded = true;
> @@ -155,7 +155,7 @@ static void pc_q35_init(MachineState *machine)
>      guest_info->isapc_ram_fw = false;
>      guest_info->has_acpi_build = has_acpi_build;
>      guest_info->has_reserved_memory = has_reserved_memory;
> -    guest_info->has_immutable_rsdp = has_immutable_rsdp;
> +    guest_info->rsdp_in_ram = rsdp_in_ram;
>  
>      /* Migration was not supported in 2.0 for Q35, so do not bother
>       * with this hack (see hw/i386/acpi-build.c).
> @@ -291,7 +291,7 @@ static void pc_q35_init(MachineState *machine)
>  
>  static void pc_compat_2_2(MachineState *machine)
>  {
> -    has_immutable_rsdp = true;
> +    rsdp_in_ram = false;
>      x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
>      x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
>      x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);

[Qemu-devel] [PATCH 4/4] acpi-build: simplify rsdp management for legacy

[Michael S. Tsirkin]

For legacy machine types, rsdp is not in RAM, so we need a copy of rsdp
for fw cfg. We previously used g_array_free with false parameter,
but this seems to confuse people.
This also wastes a bit of memory as the buffer is unused for new
machine types.

Let's just use plain g_memdup, and free original memory together with
the array.

TODO: rationalize tcpalog memory management, and get rid of the mfre
parameter.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ffa3f00..f712277 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1257,7 +1257,7 @@ static inline void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
 {
     void *linker_data = bios_linker_loader_cleanup(tables->linker);
     g_free(linker_data);
-    g_array_free(tables->rsdp, mfre);
+    g_array_free(tables->rsdp, true);
     g_array_free(tables->table_data, true);
     g_array_free(tables->tcpalog, mfre);
 }
@@ -1560,12 +1560,14 @@ void acpi_setup(PcGuestInfo *guest_info)
         /*
          * Keep for compatibility with old machine types.
          * Though RSDP is small, its contents isn't immutable, so
-         * update it along with the rest of tables on guest access.
+         * we'll update it along with the rest of tables on guest access.
          */
+        uint32_t rsdp_size = acpi_data_len(tables.rsdp);
+
+        build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
         fw_cfg_add_file_callback(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
                                  acpi_build_update, build_state,
-                                 tables.rsdp->data, acpi_data_len(tables.rsdp));
-        build_state->rsdp = tables.rsdp->data;
+                                 build_state->rsdp, rsdp_size);
         build_state->rsdp_ram = (ram_addr_t)-1;
     } else {
         build_state->rsdp = NULL;
-- 
MST

Re: [Qemu-devel] [PATCH 4/4] acpi-build: simplify rsdp management for legacy

[Igor Mammedov]

On Tue, 17 Feb 2015 11:05:45 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> For legacy machine types, rsdp is not in RAM, so we need a copy of rsdp
> for fw cfg. We previously used g_array_free with false parameter,
> but this seems to confuse people.
> This also wastes a bit of memory as the buffer is unused for new
> machine types.
> 
> Let's just use plain g_memdup, and free original memory together with
> the array.
> 
> TODO: rationalize tcpalog memory management, and get rid of the mfre
> parameter.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/acpi-build.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index ffa3f00..f712277 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1257,7 +1257,7 @@ static inline void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>  {
>      void *linker_data = bios_linker_loader_cleanup(tables->linker);
>      g_free(linker_data);
> -    g_array_free(tables->rsdp, mfre);
> +    g_array_free(tables->rsdp, true);
>      g_array_free(tables->table_data, true);
>      g_array_free(tables->tcpalog, mfre);
>  }
> @@ -1560,12 +1560,14 @@ void acpi_setup(PcGuestInfo *guest_info)
>          /*
>           * Keep for compatibility with old machine types.
>           * Though RSDP is small, its contents isn't immutable, so
> -         * update it along with the rest of tables on guest access.
> +         * we'll update it along with the rest of tables on guest access.
>           */
> +        uint32_t rsdp_size = acpi_data_len(tables.rsdp);
> +
> +        build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
>          fw_cfg_add_file_callback(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
>                                   acpi_build_update, build_state,
> -                                 tables.rsdp->data, acpi_data_len(tables.rsdp));
> -        build_state->rsdp = tables.rsdp->data;
> +                                 build_state->rsdp, rsdp_size);
>          build_state->rsdp_ram = (ram_addr_t)-1;
>      } else {
>          build_state->rsdp = NULL;