Re: [Qemu-devel] [PATCH] hw/arm/virt: generate 64-bit addressable ACPI objects

[Phil Dennis-Jordan]

On 7 April 2017 at 21:11, Laszlo Ersek <address@hidden> wrote:
> On 04/07/17 10:55, Andrew Jones wrote:
>> On Thu, Apr 06, 2017 at 10:22:09PM +0100, Ard Biesheuvel wrote:
>>> Our current ACPI table generation code limits the placement of ACPI
>>> tables to 32-bit addressable memory, in order to be able to emit the
>>> root pointer (RSDP) and root table (RSDT) using table types from the
>>> ACPI 1.0 days.
>>>
>>> Since ARM was not supported by ACPI before version 5.0, it makes sense
>>> to lift this restriction. This is not crucial for mach-virt, which is
>>> guaranteed to have some memory available below the 4 GB mark, but it
>>> is a nice to have for QEMU machines that do not have any 32-bit
>>> addressable, not unlike some real world 64-bit ARM systems.

"QEMU machines that do not have any 32-bit addressable memory," perhaps?

>>> Since we already emit a version of the RSDP root pointer that carries
>>> a 64-bit wide address for the 64-bit root table (XSDT), all we need to
>>> do is replace the RSDT generation with the generation of an XSDT table,
>>> and use a different slot in the FADT table to refer to the DSDT.
>>>
>>> Note that this only modifies the private interaction between QEMU and
>>> UEFI. Since these tables are all handled by the generic AcpiTableDxe
>>> driver which generates its own RSDP, RSDT and XSDT tables and manages
>>> the DSDT pointer in FADT itself, the tables that are present to the
>>> guest are identical, and so no mach-virt versioning is required for
>>> this change.
>
> The last paragraph could be dropped from the commit message (or trimmed
> a bit). Not really necessary, saying it just FYI. Namely, we don't
> version firmware blobs (i.e., we don't tie different firmware blobs to
> different versions of a machine type), and ACPI tables are considered
> part of the firmware (from the OS's POV), despite the fact that
> technically we generate them in QEMU. So, in my understanding, the ACPI
> content we generate never needs to consider machine types.
>
>>>
>>> Signed-off-by: Ard Biesheuvel <address@hidden>
>>> ---
>>>  hw/arm/virt-acpi-build.c    | 55 
>>> ++++++++++++++++++++++++++++++++++-----------
>>>  include/hw/acpi/acpi-defs.h | 11 +++++++++
>>>  2 files changed, 53 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index b173bd109b91..d18368094c00 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -391,12 +391,12 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>>>
>>>  /* RSDP */
>>>  static GArray *
>>> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned 
>>> rsdt_tbl_offset)
>>> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned 
>>> xsdt_tbl_offset)
>>>  {
>>>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
>>> -    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
>>> -    unsigned rsdt_pa_offset =
>>> -        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
>>> +    unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
>>> +    unsigned xsdt_pa_offset =
>>> +        (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
>>>
>>>      bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
>>>                               true /* fseg memory */);
>>> @@ -408,8 +408,8 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
>>> unsigned rsdt_tbl_offset)
>>>
>>>      /* Address to be filled by Guest linker */
>>>      bios_linker_loader_add_pointer(linker,
>>> -        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
>>> -        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
>>> +        ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
>>> +        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
>>>
>>>      /* Checksum to be filled by Guest linker */
>>>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>>> @@ -686,7 +686,7 @@ static void build_fadt(GArray *table_data, BIOSLinker 
>>> *linker,
>>>                         VirtMachineState *vms, unsigned dsdt_tbl_offset)
>>>  {
>>>      AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, 
>>> sizeof(*fadt));
>>> -    unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
>>> +    unsigned xdsdt_entry_offset = (char *)&fadt->Xdsdt - table_data->data;
>>>      uint16_t bootflags;
>>>
>>>      switch (vms->psci_conduit) {
>>> @@ -712,7 +712,7 @@ static void build_fadt(GArray *table_data, BIOSLinker 
>>> *linker,
>>>
>>>      /* DSDT address to be filled by Guest linker */
>>>      bios_linker_loader_add_pointer(linker,
>>> -        ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>>> +        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->Xdsdt),
>>>          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>>>
>>>      build_header(linker, table_data,

I don't know what it's like in ARM-Land, but X64 Windows does not take
kindly to a zero DSDT pointer in my experience. It might make sense to
make the choice between DSDT and XDSDT conditional on whether the
pointer is actually above 4GB? Mind you, I don't even know if
Microsoft makes ARM variants of Windows generally available, and
whether they currently work in Qemu; the FOSS OSes are typically more
lenient.

See also the discussion in this thread on qemu-devel:
https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05754.html
and a related issue on edk2-devel:
https://lists.01.org/pipermail/edk2-devel/2017-March/008460.html

>>> @@ -772,12 +772,41 @@ struct AcpiBuildState {
>>>      bool patched;
>>>  } AcpiBuildState;
>>>
>>> +/* Build xsdt table */
>>> +
>>> +static
>>> +void build_xsdt(GArray *table_data, BIOSLinker *linker, GArray 
>>> *table_offsets,
>>> +                const char *oem_id, const char *oem_table_id)
>>> +{
>>> +    int i;
>>> +    unsigned xsdt_entries_offset;
>>> +    AcpiXsdtDescriptorRev2 *xsdt;
>>> +    const unsigned table_data_len = (sizeof(uint64_t) * 
>>> table_offsets->len);
>>> +    const unsigned xsdt_entry_size = sizeof(xsdt->table_offset_entry[0]);
>>> +    const size_t xsdt_len = sizeof(*xsdt) + table_data_len;
>>> +
>>> +    xsdt = acpi_data_push(table_data, xsdt_len);
>>> +    xsdt_entries_offset = (char *)xsdt->table_offset_entry - 
>>> table_data->data;
>>> +    for (i = 0; i < table_offsets->len; ++i) {
>>> +        uint64_t ref_tbl_offset = g_array_index(table_offsets, uint32_t, 
>>> i);
>>> +        uint64_t xsdt_entry_offset = xsdt_entries_offset + xsdt_entry_size 
>>> * i;
>>> +
>>> +        /* xsdt->table_offset_entry to be filled by Guest linker */
>>> +        bios_linker_loader_add_pointer(linker,
>>> +            ACPI_BUILD_TABLE_FILE, xsdt_entry_offset, xsdt_entry_size,
>>> +            ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
>>> +    }
>>> +    build_header(linker, table_data,
>>> +                 (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, oem_table_id);
>>> +}
>>
>> build_xsdt should probably go in hw/acpi/aml-build.c
>>
>>> +
>>> +
>>>  static
>>>  void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>  {
>>>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>>      GArray *table_offsets;
>>> -    unsigned dsdt, rsdt;
>>> +    unsigned dsdt, xsdt;
>>>      GArray *tables_blob = tables->table_data;
>>>
>>>      table_offsets = g_array_new(false, true /* clear */,
>>> @@ -817,12 +846,12 @@ void virt_acpi_build(VirtMachineState *vms, 
>>> AcpiBuildTables *tables)
>>>          build_iort(tables_blob, tables->linker);
>>>      }
>>>
>>> -    /* RSDT is pointed to by RSDP */
>>> -    rsdt = tables_blob->len;
>>> -    build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
>>> +    /* XSDT is pointed to by RSDP */
>>> +    xsdt = tables_blob->len;
>>> +    build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
>>>
>>>      /* RSDP is in FSEG memory, so allocate it separately */
>>> -    build_rsdp(tables->rsdp, tables->linker, rsdt);
>>> +    build_rsdp(tables->rsdp, tables->linker, xsdt);
>>>
>>>      /* Cleanup memory that's no longer used. */
>>>      g_array_free(table_offsets, true);
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index 4cc3630e613e..bf37acf4c4c6 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -238,6 +238,17 @@ struct AcpiRsdtDescriptorRev1
>>>  typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
>>>
>>>  /*
>>> + * ACPI 2.0 eXtended System Description Table (XSDT)
>>> + */
>>> +struct AcpiXsdtDescriptorRev2
>>> +{
>>> +    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 AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
>>> +
>>> +/*
>>>   * ACPI 1.0 Firmware ACPI Control Structure (FACS)
>>>   */
>>>  struct AcpiFacsDescriptorRev1
>>> --
>>> 2.9.3
>>>
>>>
>>
>> Otherwise
>>
>> Reviewed-by: Andrew Jones <address@hidden>
>>
>
> Great, I didn't want to be the first one to provide public feedback. :)
>
> Personally I'm fine if we keep the XSDT generation local to
> "hw/arm/virt-acpi-build.c" for now, and we upstream it to
> "hw/acpi/aml-build.c" later (for example when i386 actually puts it to
> use -- which shouldn't be that far out, AIUI, since Phil will probably
> need XSDT for OSX guests). However, I also wouldn't mind if the XSDT
> builder were moved to "hw/acpi/aml-build.c" right now.

FWIW, OS X guests don't appear to care about an XSDT, and I'm not
planning any work in that area. My patch only affects the x86 FADT
(Rev1->Rev3). From what I saw, this is, aside from the struct
definition, completely independent from Qemu's ARM FADT, which I think
has always used the Rev5(.1?) variant.
(Are patches for 2.10 already being accepted? Michael previously said
I should wait until 2.9 is out, that's the only reason I'm still
hanging onto my patch.)

> So one way or another,
>
> Acked-by: Laszlo Ersek <address@hidden>
>
> Thanks
> Laszlo