Re: [PATCH v3 11/35] acpi: nvdimm_build_ssdt: use acpi_table_begin()/acpi_table_end() instead of build_header()

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 20 Sep 2021 18:41:30 +0200
Eric Auger <eauger@redhat.com> wrote:

> Hi Igor,
> 
> On 9/7/21 4:47 PM, Igor Mammedov wrote:
> > it replaces error-prone pointer arithmetic for build_header() API,
> > with 2 calls to start and finish table creation,
> > which hides offsets magic from API user.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/
> > 
> > CC: xiaoguangrong.eric@gmail.com
> > ---
> >  hw/acpi/nvdimm.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 15f6ca82ca..e050b47c2b 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -1274,14 +1274,15 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> >                                NVDIMMState *nvdimm_state,
> >                                uint32_t ram_slots, const char *oem_id)
> >  {
> > +    int mem_addr_offset;
> >      Aml *ssdt, *sb_scope, *dev;
> > -    int mem_addr_offset, nvdimm_ssdt;
> > +    AcpiTable table = { .sig = "SSDT", .rev = 1,
> > +                        .oem_id = oem_id, .oem_table_id = "NVDIMM" };
> >  
> >      acpi_add_table(table_offsets, table_data);
> >  
> > +    acpi_table_begin(&table, table_data);
> >      ssdt = init_aml_allocator();
> > -    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > -
> >      sb_scope = aml_scope("\\_SB");
> >  
> >      dev = aml_device("NVDR");
> > @@ -1310,8 +1311,6 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> >      aml_append(sb_scope, dev);
> >      aml_append(ssdt, sb_scope);
> >  
> > -    nvdimm_ssdt = table_data->len;
> > -
> >      /* copy AML table into ACPI tables blob and patch header there */
> >      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> >      mem_addr_offset = build_append_named_dword(table_data,
> > @@ -1323,10 +1322,13 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
> >          NVDIMM_DSM_MEM_FILE, 0);
> > -    build_header(linker, table_data,
> > -        (void *)(table_data->data + nvdimm_ssdt),
> > -                 "SSDT", table_data->len - nvdimm_ssdt, 1, oem_id, "NVDIMM");
> >      free_aml_allocator();
> > +    /*
> > +     * must be executed as the last so that pointer patching command above
> > +     * would be executed by guest before it recalculates checksum which were
> > +     * scheduled by acpi_table_composed()  
> s/acpi_table_composed/acpi_table_end.
fixed it up:

    /*                                                                           
     * must be executed as the last so that pointer patching command above       
     * would be executed by guest before it recalculated checksum which were     
     * scheduled by acpi_table_end()                                             
     */ 
 
> also the sentence may need some rewording.
> 
> Wonder if that kind of comment still is useful. Maybe this should be put
> once in the doc comment of acpi_table_end() in [PATCH v3 01/35] acpi:
> add helper routines to initialize ACPI tables, for future users to pay
> attention that it shall be called at the very end.

I'll keep comment for now, and add patch to add something similar to
acpi_table_end() later or drop it if I come up with a way to actually
enforce expected linker order.

 
> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 
> 
> > +     */
> > +    acpi_table_end(linker, &table);
> >  }
> >  
> >  void nvdimm_build_srat(GArray *table_data)
> >   
>