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

[Phil Dennis-Jordan]

On Fri, Apr 7, 2017 at 9:44 PM, Ard Biesheuvel
<address@hidden> wrote:
> On 7 April 2017 at 10:36, Phil Dennis-Jordan <address@hidden> wrote:
>> 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?
>>
>
> Yes, better.
>
>>>>> 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.
>>
>
> Documentation/arm64/acpi_object_usage.txt in the kernel contains this under 
> FADT
>
>    For the DSDT that is also required, the X_DSDT field is to be used,
>    not the DSDT field.
>
> and so we should be using xdsdt anyway.

IME, what the documentation & standard say can diverge wildly from
what shipping OSes actually expect. That's all I'm saying. :-)

> *However*, this is all moot
> given that UEFI is in charge of populating these fields. What we
> generate here and what the guest sees are two different things that
> are almost completely disconnected when it comes to RSDP, RSDT/XSDT
> and the DSDT field in FADT.

Indeed. For x86 we have to care about SeaBIOS of course, I guess on
ARM there's no such concern.

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