Re: [Qemu-devel] [PATCH 3/4] i386/acpi: add XSDT

[Shannon Zhao]

On 2015/6/7 17:42, Michael S. Tsirkin wrote:
> On Fri, Jun 05, 2015 at 10:38:24AM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/6/5 0:21, Michael S. Tsirkin wrote:
>>> At the moment it mirrors RSDT exactly.
>>>
>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>> ---
>>>  include/hw/acpi/acpi-defs.h | 15 ++++++++++++---
>>>  include/hw/acpi/aml-build.h |  2 ++
>>>  hw/acpi/aml-build.c         | 41 ++++++++++++++++++++++++++++-------------
>>>  hw/i386/acpi-build.c        | 29 ++++++++++++++++++++++++-----
>>>  4 files changed, 66 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index 59cf277..fe1dc7d 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -201,13 +201,22 @@ enum {
>>>   */
>>>  struct AcpiRsdtDescriptorRev1
>>>  {
>>> -    ACPI_TABLE_HEADER_DEF       /* ACPI common table header */
>>> -    uint32_t table_offset_entry[0];  /* Array of pointers to other */
>>> -    /* ACPI tables */
>>> +    ACPI_TABLE_HEADER_DEF           /* ACPI common table header */
>>> +    uint32_t table_offset_entry[0]; /* Array of pointers to other ACPI 
>>> tables */
>>>  } QEMU_PACKED;
>>>  typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
>>>  
>>>  /*
>>> + * ACPI 2.0 Extended System Description Table (XSDT)
>>> + */
>>> +struct AcpiXsdtDescriptor
>>> +{
>>> +    ACPI_TABLE_HEADER_DEF           /* ACPI common table header */
>>> +    uint64_t table_offset_entry[0]; /* Array of pointers to other ACPI 
>>> tables */
>>> +} QEMU_PACKED;
>>> +typedef struct AcpiXsdtDescriptor AcpiXsdtDescriptor;
>>> +
>>> +/*
>>>   * ACPI 1.0 Firmware ACPI Control Structure (FACS)
>>>   */
>>>  struct AcpiFacsDescriptorRev1
>>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>> index edc1dfa..0a00402 100644
>>> --- a/include/hw/acpi/aml-build.h
>>> +++ b/include/hw/acpi/aml-build.h
>>> @@ -287,5 +287,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables);
>>>  void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
>>>  void
>>>  build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
>>> +void
>>> +build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
>>>  
>>>  #endif
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index 4c930b6..d0e52c4 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -1200,25 +1200,40 @@ void acpi_build_tables_cleanup(AcpiBuildTables 
>>> *tables, bool mfre)
>>>  }
>>>  
>>>  /* Build rsdt table */
>>> -void
>>> -build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
>>> +static void
>>> +build_sdt(GArray *table_data, GArray *linker, GArray *table_offsets,
>>> +          const char *sig, int entry_size)
>>>  {
>>> -    AcpiRsdtDescriptorRev1 *rsdt;
>>> -    size_t rsdt_len;
>>> +    AcpiTableHeader *sdt;
>>> +    size_t sdt_len;
>>>      int i;
>>> -    const int table_data_len = (sizeof(uint32_t) * table_offsets->len);
>>> +    const int table_data_len = (entry_size * table_offsets->len);
>>> +    char *entries;
>>>  
>>> -    rsdt_len = sizeof(*rsdt) + table_data_len;
>>> -    rsdt = acpi_data_push(table_data, rsdt_len);
>>> -    memcpy(rsdt->table_offset_entry, table_offsets->data, table_data_len);
>>> +    sdt_len = sizeof(*sdt) + table_data_len;
>>> +    sdt = acpi_data_push(table_data, sdt_len);
>>> +    entries = ((char *)sdt) + sizeof(*sdt);
>>> +    memcpy(entries, table_offsets->data, table_data_len);
>>>      for (i = 0; i < table_offsets->len; ++i) {
>>> -        /* rsdt->table_offset_entry to be filled by Guest linker */
>>> +        /* entry to be filled by Guest linker */
>>>          bios_linker_loader_add_pointer(linker,
>>>                                         ACPI_BUILD_TABLE_FILE,
>>>                                         ACPI_BUILD_TABLE_FILE,
>>> -                                       table_data, 
>>> &rsdt->table_offset_entry[i],
>>> -                                       sizeof(uint32_t));
>>> +                                       table_data, entries + i * 
>>> entry_size,
>>> +                                       entry_size);
>>>      }
>>> -    build_header(linker, table_data,
>>> -                 (void *)rsdt, "RSDT", rsdt_len, 1);
>>> +    build_header(linker, table_data, sdt, sig, sdt_len, 1);
>>
>> The table revision should update as well. Make it as a parameter of
>> build_sdt.
> 
> How do you figure this?
> The spec I have lists XSDT revision as 1.
>

Oh, sorry, ignore this.

>>> +}
>>> +
>>> +void
>>> +build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
>>> +{
>>> +    build_sdt(table_data, linker, table_offsets, "RSDT", sizeof(uint32_t));
>>> +}
>>> +
>>> +/* Build xsdt table */
>>> +void
>>> +build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
>>> +{
>>> +    build_sdt(table_data, linker, table_offsets, "XSDT", sizeof(uint64_t));
>>>  }
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index aaa149b..3551a08 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -1593,7 +1593,7 @@ build_dsdt(GArray *table_data, GArray *linker, 
>>> AcpiMiscInfo *misc)
>>>  }
>>>  
>>>  static GArray *
>>> -build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>>> +build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt, unsigned 
>>> xsdt)
>>
>> uint64_t xsdt?
> 
> No, because it's an offset within the blob, not the physical address.
> We can't really ask bios to allocate > 4G blob for us.
> 
>>>  {
>>>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>>>  
>>> @@ -1602,16 +1602,33 @@ build_rsdp(GArray *rsdp_table, GArray *linker, 
>>> unsigned rsdt)
>>>  
>>>      memcpy(&rsdp->signature, "RSD PTR ", 8);
>>>      memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
>>> +
>>> +    rsdp->revision = 0x02;
>>> +    rsdp->length = cpu_to_le32(sizeof *rsdp);
>>> +
>>>      rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
>>>      /* Address to be filled by Guest linker */
>>>      bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
>>>                                     ACPI_BUILD_TABLE_FILE,
>>>                                     rsdp_table, 
>>> &rsdp->rsdt_physical_address,
>>>                                     sizeof rsdp->rsdt_physical_address);
>>> +
>>> +    rsdp->xsdt_physical_address = cpu_to_le32(xsdt);
>>
>> cpu_to_le64(xsdt) ?
> 
> Good catch. Will fix, thanks!
> 
>>> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
>>> +                                   ACPI_BUILD_TABLE_FILE,
>>> +                                   rsdp_table, 
>>> &rsdp->xsdt_physical_address,
>>> +                                   sizeof rsdp->xsdt_physical_address);
>>>      rsdp->checksum = 0;
>>>      /* Checksum to be filled by Guest linker */
>>>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>>> -                                    rsdp, rsdp, sizeof *rsdp, 
>>> &rsdp->checksum);
>>> +                                    rsdp, rsdp, 
>>> offsetof(AcpiRsdpDescriptor, length),
>>> +                                    &rsdp->checksum);
>>> +
>>> +    rsdp->extended_checksum = 0;
>>> +    /* Checksum to be filled by Guest linker */
>>> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>>> +                                    rsdp, rsdp, sizeof *rsdp,
>>> +                                    &rsdp->extended_checksum);
>>>  
>>>      return rsdp_table;
>>>  }
>>> @@ -1665,7 +1682,7 @@ void acpi_build(PcGuestInfo *guest_info, 
>>> AcpiBuildTables *tables)
>>>  {
>>>      GArray *rsdt_table_offsets;
>>>      GArray *xsdt_table_offsets;
>>> -    unsigned facs, ssdt, dsdt, rsdt;
>>> +    unsigned facs, ssdt, dsdt, rsdt, xsdt;
>>>      AcpiCpuInfo cpu;
>>>      AcpiPmInfo pm;
>>>      AcpiMiscInfo misc;
>>> @@ -1762,12 +1779,14 @@ void acpi_build(PcGuestInfo *guest_info, 
>>> AcpiBuildTables *tables)
>>>          g_array_append_vals(tables_blob, u, len);
>>>      }
>>>  
>>> -    /* RSDT is pointed to by RSDP */
>>> +    /* RSDT and XSDT pointed to by RSDP */
>>>      rsdt = tables_blob->len;
>>>      build_rsdt(tables_blob, tables->linker, rsdt_table_offsets);
>>> +    xsdt = tables_blob->len;
>>> +    build_xsdt(tables_blob, tables->linker, xsdt_table_offsets);
>>>  
>>>      /* RSDP is in FSEG memory, so allocate it separately */
>>> -    build_rsdp(tables->rsdp, tables->linker, rsdt);
>>> +    build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
>>>  
>>>      /* We'll expose it all to Guest so we want to reduce
>>>       * chance of size changes.
>>>
>>
>> -- 
>> Shannon
> 
> .
> 

-- 
Shannon