Re: [Xen-devel] [PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP

["Michael S. Tsirkin" <mst@xxxxxxxxxx>]

On Thu, Nov 08, 2018 at 03:16:23PM +0100, Igor Mammedov wrote:
> On Mon,  5 Nov 2018 02:40:28 +0100
> Samuel Ortiz <sameo@xxxxxxxxxxxxxxx> wrote:
> 
> > XSDT is the 64-bit version of the legacy ACPI RSDT (Root System
> > Description Table). RSDT only allow for 32-bit addressses and have thus
> > been deprecated. Since ACPI version 2.0, RSDPs should point at XSDTs and
> > no longer RSDTs, although RSDTs are still supported for backward
> > compatibility.
> > 
> > Since version 2.0, RSDPs should add an extended checksum, a complete table
> > length and a version field to the table.
> 
> This patch re-implements what arm/virt board already does
> and fixes checksum bug in the later and at the same time
> without a user (within the patch).
> 
> I'd suggest redo it a way similar to FADT refactoring
>   patch 1: fix checksum bug in virt/arm
>   patch 2: update reference tables in test
>   patch 3: introduce AcpiRsdpData similar to commit 937d1b587
>              (both arm and x86) wich stores all data in hos byte order
>   patch 4: convert arm's impl. to build_append_int_noprefix() API (commit 
> 5d7a334f7)
>            ... move out to aml-build.c
>   patch 5: reuse generalized arm's build_rsdp() for x86, dropping x86 
> specific one
>       amending it to generate rev1 variant defined by revision in AcpiRsdpData
>       (commit dd1b2037a)
> 
>   'make check V=1' shouldn't observe any ACPI tables changes after patch 2.

And your next suggestion is to add patch 6.  I guess it's doable but
this will make a single patch a 6 patch series. At this rate this series
will be at 200 patches easily.

Automated checks are cool but hey it's easy to see what changed in a
disassembled table, and we do not update them blindly. So just note in
the comment that there's a table change for ARM and expected files need
to be updated and we should be fine IMHO.

> > Signed-off-by: Samuel Ortiz <sameo@xxxxxxxxxxxxxxx>
> > ---
> >  include/hw/acpi/aml-build.h |  3 +++
> >  hw/acpi/aml-build.c         | 37 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index c9bcb32d81..3580d0ce90 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -393,6 +393,9 @@ void
> >  build_rsdp(GArray *table_data,
> >             BIOSLinker *linker, unsigned rsdt_tbl_offset);
> >  void
> > +build_rsdp_xsdt(GArray *table_data,
> > +                BIOSLinker *linker, unsigned xsdt_tbl_offset);
> > +void
> >  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
> >             const char *oem_id, const char *oem_table_id);
> >  void
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 51b608432f..a030d40674 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1651,6 +1651,43 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, 
> > GArray *table_offsets,
> >                   (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, oem_table_id);
> >  }
> >  
> > +/* RSDP pointing at an XSDT */
> > +void
> > +build_rsdp_xsdt(GArray *rsdp_table,
> > +                BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > +{
> > +    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > +    unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > +    unsigned xsdt_pa_offset =
> > +        (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
> > +    unsigned xsdt_offset =
> > +        (char *)&rsdp->length - rsdp_table->data;

There's a cleaner way to get at the offsets than pointer math:
1. save rsdp_table length before you push
2. add offset_of for fields

If switching to build_append_int_noprefix then it's even
easier - just save length before you append the int
you intend to patch.


> > +
> > +    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
> > +                             true /* fseg memory */);
> > +
> > +    memcpy(&rsdp->signature, "RSD PTR ", 8);
> > +    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> > +    rsdp->length = cpu_to_le32(sizeof(*rsdp));
> > +    /* version 2, we will use the XSDT pointer */
> > +    rsdp->revision = 0x02;
> > +
> > +    /* Address to be filled by Guest linker */
> > +    bios_linker_loader_add_pointer(linker,
> > +        ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> > +        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> > +
> > +    /* Legacy checksum to be filled by Guest linker */
> > +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > +        (char *)rsdp - rsdp_table->data, xsdt_offset,
> > +        (char *)&rsdp->checksum - rsdp_table->data);
> > +
> > +    /* Extended checksum to be filled by Guest linker */
> > +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > +        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
> > +        (char *)&rsdp->extended_checksum - rsdp_table->data);
> > +}
> > +
> >  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> >                         uint64_t len, int node, MemoryAffinityFlags flags)
> >  {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel