Re: [PATCH for-6.2] hw/arm/virt_acpi_build: Generate DBG2 table

[Ard Biesheuvel]

On Tue, 10 Aug 2021 at 10:31, Eric Auger <eric.auger@redhat.com> wrote:
>
> ARM SBBR specification mandates DBG2 table (Debug Port Table 2).
> this latter allows to describe one or more debug ports.
>
> Generate an DBG2 table featuring a single debug port, the PL011.
>
> The DBG2 specification can be found at:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table?redirectedfrom=MSDN
>

Have the legal issues around this table been resolved in the mean
time? Also, any clue why this table is mandatory to begin with? The
SBBR has been very trigger happy lately with making things mandatory
that aren't truly required from a functional perspective.


> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> Tested by comparing the content with the table generated
> by EDK2 along with the SBSA-REF machine (code generated by
> DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c).
>
> I reused the Generic Address Structure filled by QEMU in the SPCR, ie.
> bit_width = 8 and byte access. While EDK2 sets bit_width = 32 and
> dword access. Also the name exposed by acpica tools is different:
> 'COM0' in my case where '\_SB.COM0' in SBSA-REF case?
> ---
>  hw/arm/virt-acpi-build.c    | 77 ++++++++++++++++++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h | 50 ++++++++++++++++++++++++
>  2 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 037cc1fd82..35f27b41df 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -563,6 +563,78 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>                   vms->oem_table_id);
>  }
>
> +#define ACPI_DBG2_PL011_UART_LENGTH 0x1000
> +
> +/* DBG2 */
> +static void
> +build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +{
> +    int addr_offset, addrsize_offset, namespace_offset, namespace_length;
> +    const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART];
> +    struct AcpiGenericAddress *base_address;
> +    int dbg2_start = table_data->len;
> +    AcpiDbg2Device *dbg2dev;
> +    char name[] = "COM0";
> +    AcpiDbg2Table *dbg2;
> +    uint32_t *addr_size;
> +    uint8_t *namespace;
> +
> +    dbg2 = acpi_data_push(table_data, sizeof *dbg2);
> +    dbg2->info_offset = sizeof *dbg2;
> +    dbg2->info_count = 1;
> +
> +    /* debug device info structure */
> +
> +    dbg2dev = acpi_data_push(table_data, sizeof(AcpiDbg2Device));
> +
> +    dbg2dev->revision = 0;
> +    namespace_length = sizeof name;
> +    dbg2dev->length = sizeof *dbg2dev + sizeof(struct AcpiGenericAddress) +
> +                      4 + namespace_length;
> +    dbg2dev->register_count = 1;
> +
> +    addr_offset = sizeof *dbg2dev;
> +    addrsize_offset = addr_offset + sizeof(struct AcpiGenericAddress);
> +    namespace_offset = addrsize_offset + 4;
> +
> +    dbg2dev->namepath_length = cpu_to_le16(namespace_length);
> +    dbg2dev->namepath_offset = cpu_to_le16(namespace_offset);
> +    dbg2dev->oem_data_length = cpu_to_le16(0);
> +    dbg2dev->oem_data_offset = cpu_to_le16(0); /* No OEM data is present */
> +    dbg2dev->port_type = cpu_to_le16(ACPI_DBG2_SERIAL_PORT);
> +    dbg2dev->port_subtype = cpu_to_le16(ACPI_DBG2_ARM_PL011);
> +
> +    dbg2dev->base_address_offset = cpu_to_le16(addr_offset);
> +    dbg2dev->address_size_offset = cpu_to_le16(addrsize_offset);
> +
> +    /*
> +     * variable length content:
> +     * BaseAddressRegister[1]
> +     * AddressSize[1]
> +     * NamespaceString[1]
> +     */
> +
> +    base_address = acpi_data_push(table_data,
> +                                  sizeof(struct AcpiGenericAddress));
> +
> +    base_address->space_id = AML_SYSTEM_MEMORY;
> +    base_address->bit_width = 8;
> +    base_address->bit_offset = 0;
> +    base_address->access_width = 1;
> +    base_address->address = cpu_to_le64(uart_memmap->base);
> +
> +    addr_size = acpi_data_push(table_data, sizeof *addr_size);
> +    *addr_size = cpu_to_le32(ACPI_DBG2_PL011_UART_LENGTH);
> +
> +    namespace = acpi_data_push(table_data, namespace_length);
> +    memcpy(namespace, name, namespace_length);
> +
> +    build_header(linker, table_data,
> +                 (void *)(table_data->data + dbg2_start), "DBG2",
> +                 table_data->len - dbg2_start, 3, vms->oem_id,
> +                 vms->oem_table_id);
> +}
> +
>  /* MADT */
>  static void
>  build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> @@ -790,7 +862,7 @@ void virt_acpi_build(VirtMachineState *vms, 
> AcpiBuildTables *tables)
>      dsdt = tables_blob->len;
>      build_dsdt(tables_blob, tables->linker, vms);
>
> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
> +    /* FADT MADT GTDT MCFG SPCR DBG2 pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
>
> @@ -813,6 +885,9 @@ void virt_acpi_build(VirtMachineState *vms, 
> AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_spcr(tables_blob, tables->linker, vms);
>
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_dbg2(tables_blob, tables->linker, vms);
> +
>      if (vms->ras) {
>          build_ghes_error_table(tables->hardware_errors, tables->linker);
>          acpi_add_table(table_offsets, tables_blob);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index cf9f44299c..bdb2ebed2c 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -618,4 +618,54 @@ struct AcpiIortRC {
>  } QEMU_PACKED;
>  typedef struct AcpiIortRC AcpiIortRC;
>
> +/* DBG2 */
> +
> +/* Types for port_type field above */
> +
> +#define ACPI_DBG2_SERIAL_PORT       0x8000
> +#define ACPI_DBG2_1394_PORT         0x8001
> +#define ACPI_DBG2_USB_PORT          0x8002
> +#define ACPI_DBG2_NET_PORT          0x8003
> +
> +/* Subtypes for port_subtype field above */
> +
> +#define ACPI_DBG2_16550_COMPATIBLE  0x0000
> +#define ACPI_DBG2_16550_SUBSET      0x0001
> +#define ACPI_DBG2_ARM_PL011         0x0003
> +#define ACPI_DBG2_ARM_SBSA_32BIT    0x000D
> +#define ACPI_DBG2_ARM_SBSA_GENERIC  0x000E
> +#define ACPI_DBG2_ARM_DCC           0x000F
> +#define ACPI_DBG2_BCM2835           0x0010
> +
> +#define ACPI_DBG2_1394_STANDARD     0x0000
> +
> +#define ACPI_DBG2_USB_XHCI          0x0000
> +#define ACPI_DBG2_USB_EHCI          0x0001
> +
> +/* Debug Device Information Subtable */
> +
> +struct AcpiDbg2Device {
> +    uint8_t  revision;
> +    uint16_t length;
> +    uint8_t  register_count; /* Number of base_address registers */
> +    uint16_t namepath_length;
> +    uint16_t namepath_offset;
> +    uint16_t oem_data_length;
> +    uint16_t oem_data_offset;
> +    uint16_t port_type;
> +    uint16_t port_subtype;
> +    uint8_t  reserved[2];
> +    uint16_t base_address_offset;
> +    uint16_t address_size_offset;
> +}  QEMU_PACKED;
> +typedef struct AcpiDbg2Device AcpiDbg2Device;
> +
> +struct AcpiDbg2Table {
> +    ACPI_TABLE_HEADER_DEF /* ACPI common table header */
> +    uint32_t info_offset; /* offset to the first debug struct */
> +    uint32_t info_count;  /* number of debug device info struct entries */
> +    uint8_t  dbg2_device_info[];
> +} QEMU_PACKED;
> +typedef struct AcpiDbg2Table AcpiDbg2Table;
> +
>  #endif
> --
> 2.26.3
>