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

Commit Message

Eric Auger Aug. 10, 2021, 8:30 a.m. UTC

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

Signed-off-by: Eric Auger <[e-mail]>

---

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

Comments

Ard Biesheuvel Aug. 10, 2021, 9:36 a.m. UTC | #1

On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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 <[e-mail]>
>
> ---
>
> 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
>

Eric Auger Aug. 10, 2021, 10:25 a.m. UTC | #2

Hello Ard,
On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
> On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
I don't know exactly what they are. Adding Al and Jon in the loop they
have more information about this.
How did you resolve the issue for EDK2
(DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
>  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.
It seems there are kernel FW test suites that check all mandated tables
are available and they currently fail for ARM virt.
Indeed from a function pov, I don't know much about its usage on ARM.

Maybe the SBBR spec should not flag the DBG2 as mandatory and test
suites shall be updated. I think this should be clarified at ARM then,
all the more so if there are legal issues as its spec is owned by Microsoft?

Thanks

Eric
>
>
>> Signed-off-by: Eric Auger <[e-mail]>
>>
>> ---
>>
>> 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
>>

Igor Mammedov Aug. 10, 2021, 10:52 a.m. UTC | #3

On Tue, 10 Aug 2021 10:30:57 +0200
Eric Auger <[e-mail]> 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
> 
> Signed-off-by: Eric Auger <[e-mail]>

using packed structures for composing ACPI tables is discouraged,
pls use build_append_int_noprefix() API instead. You can look at
build_amd_iommu() as an example.

PS:
Also note field comments format.
/it should be verbatim copy of entry name from respective table in spec/

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

Andrew Jones Aug. 10, 2021, 11:55 a.m. UTC | #4

On Tue, Aug 10, 2021 at 12:25:07PM +0200, Eric Auger wrote:
> Hello Ard,
> On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
> > On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
> I don't know exactly what they are. Adding Al and Jon in the loop they
> have more information about this.
> How did you resolve the issue for EDK2
> (DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
> >  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.
> It seems there are kernel FW test suites that check all mandated tables
> are available and they currently fail for ARM virt.
> Indeed from a function pov, I don't know much about its usage on ARM.

There's also a bug with getting console output on tty0 with graphical VMs

https://bugzilla.redhat.com/show_bug.cgi?id=1661288

Discussion that includes DBG2 as a possible solution starts around comment
47. I just skimmed the BZ again though and there doesn't appear to be a
clear consensus that DBG2 is the solution.

Thanks,
drew

> 
> Maybe the SBBR spec should not flag the DBG2 as mandatory and test
> suites shall be updated. I think this should be clarified at ARM then,
> all the more so if there are legal issues as its spec is owned by Microsoft?
> 
> Thanks
> 
> Eric
> >
> >
> >> Signed-off-by: Eric Auger <[e-mail]>
> >>
> >> ---
> >>
> >> 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
> >>
>

Eric Auger Aug. 10, 2021, 12:05 p.m. UTC | #5

Hi Igor,

On 8/10/21 12:52 PM, Igor Mammedov wrote:
> On Tue, 10 Aug 2021 10:30:57 +0200
> Eric Auger <[e-mail]> 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
>>
>> Signed-off-by: Eric Auger <[e-mail]>
> using packed structures for composing ACPI tables is discouraged,
> pls use build_append_int_noprefix() API instead. You can look at
> build_amd_iommu() as an example.

I understand this for new code outside of hw/arm/virt-acpi-build.c.
However build_append_int_noprefix() is not (yet) used in
virt-acpi-build.c so this would bring heterogeneity. So does it mean
that any new ACPI description introduced there should also adopt this
new style? Drew will suggest to migrate everything but well ;-)

Thanks

Eric
>
> PS:
> Also note field comments format.
> /it should be verbatim copy of entry name from respective table in spec/
>
>> ---
>>
>> 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

Eric Auger Aug. 10, 2021, 12:16 p.m. UTC | #6

Hi Drew,

On 8/10/21 1:55 PM, Andrew Jones wrote:
> On Tue, Aug 10, 2021 at 12:25:07PM +0200, Eric Auger wrote:
>> Hello Ard,
>> On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
>>> On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
>> I don't know exactly what they are. Adding Al and Jon in the loop they
>> have more information about this.
>> How did you resolve the issue for EDK2
>> (DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
>>>  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.
>> It seems there are kernel FW test suites that check all mandated tables
>> are available and they currently fail for ARM virt.
>> Indeed from a function pov, I don't know much about its usage on ARM.
> There's also a bug with getting console output on tty0 with graphical VMs
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1661288
>
> Discussion that includes DBG2 as a possible solution starts around comment
> 47. I just skimmed the BZ again though and there doesn't appear to be a
> clear consensus that DBG2 is the solution.

Yes I read that thread as well. Seems DBG2 would not fix the issue
related to the BZ. DBG2 contains less info than SPCR, it misses
interrupt specification, baud rates, etc, I guess it cannot be used
standalone to specify the serial console...
It seems the SPCR is currently interpreted by Linux as the definition of
the primary console to be used and if it is generated it superses tty0.
So I understood a workaround could be to generate it only if no
graphical device is setup or change the kernel, which was attempted by
Alper, without success yet (comment #65). Also SPCR is said mandatory by
SBBR.

Thanks

Eric
>
> Thanks,
> drew
>
>> Maybe the SBBR spec should not flag the DBG2 as mandatory and test
>> suites shall be updated. I think this should be clarified at ARM then,
>> all the more so if there are legal issues as its spec is owned by Microsoft?
>>
>> Thanks
>>
>> Eric
>>>
>>>> Signed-off-by: Eric Auger <[e-mail]>
>>>>
>>>> ---
>>>>
>>>> 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
>>>>

Igor Mammedov Aug. 10, 2021, 12:20 p.m. UTC | #7

On Tue, 10 Aug 2021 14:05:40 +0200
Eric Auger <[e-mail]> wrote:

> Hi Igor,
> 
> On 8/10/21 12:52 PM, Igor Mammedov wrote:
> > On Tue, 10 Aug 2021 10:30:57 +0200
> > Eric Auger <[e-mail]> 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
> >>
> >> Signed-off-by: Eric Auger <[e-mail]>  
> > using packed structures for composing ACPI tables is discouraged,
> > pls use build_append_int_noprefix() API instead. You can look at
> > build_amd_iommu() as an example.  
> 
> I understand this for new code outside of hw/arm/virt-acpi-build.c.
> However build_append_int_noprefix() is not (yet) used in
> virt-acpi-build.c so this would bring heterogeneity. So does it mean
> that any new ACPI description introduced there should also adopt this
> new style? Drew will suggest to migrate everything but well ;-)


new style should be used for any new ACPI code.
There is a series on list that converts old style to new style across tree
(currently planned for merging into 6.2)
https://www.mail-archive.com/[e-mail]/msg822151.html
So I'm going to push back any new patches that use old style
to reduce time on rewriting others code when it could be written
using preferred style.

PS:
Perhaps you'd like to review ARM related patches.


> Thanks
> 
> Eric
> >
> > PS:
> > Also note field comments format.
> > /it should be verbatim copy of entry name from respective table in spec/
> >  
> >> ---
> >>
> >> 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  
>

Eric Auger Aug. 10, 2021, 12:56 p.m. UTC | #8

Hi Igor,

On 8/10/21 2:20 PM, Igor Mammedov wrote:
> On Tue, 10 Aug 2021 14:05:40 +0200
> Eric Auger <[e-mail]> wrote:
>
>> Hi Igor,
>>
>> On 8/10/21 12:52 PM, Igor Mammedov wrote:
>>> On Tue, 10 Aug 2021 10:30:57 +0200
>>> Eric Auger <[e-mail]> 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
>>>>
>>>> Signed-off-by: Eric Auger <[e-mail]>  
>>> using packed structures for composing ACPI tables is discouraged,
>>> pls use build_append_int_noprefix() API instead. You can look at
>>> build_amd_iommu() as an example.  
>> I understand this for new code outside of hw/arm/virt-acpi-build.c.
>> However build_append_int_noprefix() is not (yet) used in
>> virt-acpi-build.c so this would bring heterogeneity. So does it mean
>> that any new ACPI description introduced there should also adopt this
>> new style? Drew will suggest to migrate everything but well ;-)
>
> new style should be used for any new ACPI code.
> There is a series on list that converts old style to new style across tree
> (currently planned for merging into 6.2)
> https://www.mail-archive.com/[e-mail]/msg822151.html
Ak OK I missed that refactoring. Now I better understand ;-)
> So I'm going to push back any new patches that use old style
> to reduce time on rewriting others code when it could be written
> using preferred style.
>
> PS:
> Perhaps you'd like to review ARM related patches.

Sure!

Thanks

Eric
>
>
>> Thanks
>>
>> Eric
>>> PS:
>>> Also note field comments format.
>>> /it should be verbatim copy of entry name from respective table in spec/
>>>  
>>>> ---
>>>>
>>>> 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

Samer El-Haj-Mahmoud Aug. 10, 2021, 1:10 p.m. UTC | #9

> -----Original Message-----
> From: Eric Auger <[e-mail]>
> Sent: Tuesday, August 10, 2021 6:25 AM
> To: Ard Biesheuvel <[e-mail]>
> Cc: [e-mail]; Michael S. Tsirkin <[e-mail]>; Igor
> Mammedov <[e-mail]>; Philippe Mathieu-Daudé
> <[e-mail]>; Peter Maydell <[e-mail]>; Shannon
> Zhao <[e-mail]>; qemu-arm <[e-mail]>;
> [e-mail]; Andrew Jones <[e-mail]>;
> [e-mail]; Samer El-Haj-Mahmoud <Samer.El-Haj-
> [e-mail]>; Al Stone <[e-mail]>; [e-mail]
> Subject: Re: [PATCH for-6.2] hw/arm/virt_acpi_build: Generate DBG2 table
>
> Hello Ard,
> On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
> > On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
> I don't know exactly what they are. Adding Al and Jon in the loop they
> have more information about this.
> How did you resolve the issue for EDK2
> (DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
> >  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.
> It seems there are kernel FW test suites that check all mandated tables
> are available and they currently fail for ARM virt.
> Indeed from a function pov, I don't know much about its usage on ARM.
>
> Maybe the SBBR spec should not flag the DBG2 as mandatory and test
> suites shall be updated. I think this should be clarified at ARM then,
> all the more so if there are legal issues as its spec is owned by Microsoft?
>

DBG2 has been required in SBBR since SBBR ver 1.0 (published 2016, with the 0.9 draft since 2014)
https://developer.arm.com/documentation/den0044/b/?lang=en

SBBR requires DBG2 because Windows requires it on all systems: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-system-description-tables#debug-port-table-2-dbg2 , and Windows is one of the key OSes targeted by SBBR.

The DBG2 (and SPCR) spec license issue has been resolved since August 2015. Microsoft updated both specs with identical license language, giving patent rights for implementations under the Microsoft Community Promise, and the Open OWF 1.0. This Foundation.

DBG2: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table
SPCR: https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table



> Thanks
>
> Eric
> >
> >
> >> Signed-off-by: Eric Auger <[e-mail]>
> >>
> >> ---
> >>
> >> 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
> >>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Ard Biesheuvel Aug. 10, 2021, 1:54 p.m. UTC | #10

On Tue, 10 Aug 2021 at 15:11, Samer El-Haj-Mahmoud
<[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Eric Auger <[e-mail]>
> > Sent: Tuesday, August 10, 2021 6:25 AM
> > To: Ard Biesheuvel <[e-mail]>
> > Cc: [e-mail]; Michael S. Tsirkin <[e-mail]>; Igor
> > Mammedov <[e-mail]>; Philippe Mathieu-Daudé
> > <[e-mail]>; Peter Maydell <[e-mail]>; Shannon
> > Zhao <[e-mail]>; qemu-arm <[e-mail]>;
> > [e-mail]; Andrew Jones <[e-mail]>;
> > [e-mail]; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > [e-mail]>; Al Stone <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH for-6.2] hw/arm/virt_acpi_build: Generate DBG2 table
> >
> > Hello Ard,
> > On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
> > > On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
> > I don't know exactly what they are. Adding Al and Jon in the loop they
> > have more information about this.
> > How did you resolve the issue for EDK2
> > (DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
> > >  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.
> > It seems there are kernel FW test suites that check all mandated tables
> > are available and they currently fail for ARM virt.
> > Indeed from a function pov, I don't know much about its usage on ARM.
> >
> > Maybe the SBBR spec should not flag the DBG2 as mandatory and test
> > suites shall be updated. I think this should be clarified at ARM then,
> > all the more so if there are legal issues as its spec is owned by Microsoft?
> >
>
> DBG2 has been required in SBBR since SBBR ver 1.0 (published 2016, with the 0.9 draft since 2014)
> https://developer.arm.com/documentation/den0044/b/?lang=en
>
> SBBR requires DBG2 because Windows requires it on all systems: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-system-description-tables#debug-port-table-2-dbg2 , and Windows is one of the key OSes targeted by SBBR.
>
> The DBG2 (and SPCR) spec license issue has been resolved since August 2015. Microsoft updated both specs with identical license language, giving patent rights for implementations under the Microsoft Community Promise, and the Open OWF 1.0. This Foundation.
>
> DBG2: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table
> SPCR: https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
>

Thanks Samer, for stating this on record here - and apologies for
suggesting that this was another frivolous addition to a recent SBBR
revision.

As for the difference between the two: SPCR describes the serial
console, which is an actual interactive console used for maintenance,
which exists in addition to the full blown Windows GUI, which is
always the primary interface.

DBG2 is used as a debug port, which is used for the kernel debugger,
if I am not mistaken. So SPCR and DBG2 are complementary, and it does
make sense to have both.

Eric Auger Aug. 10, 2021, 2:33 p.m. UTC | #11

Hi Samer, Ard,

On 8/10/21 3:54 PM, Ard Biesheuvel wrote:
> On Tue, 10 Aug 2021 at 15:11, Samer El-Haj-Mahmoud
> <[e-mail]> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Eric Auger <[e-mail]>
>>> Sent: Tuesday, August 10, 2021 6:25 AM
>>> To: Ard Biesheuvel <[e-mail]>
>>> Cc: [e-mail]; Michael S. Tsirkin <[e-mail]>; Igor
>>> Mammedov <[e-mail]>; Philippe Mathieu-Daudé
>>> <[e-mail]>; Peter Maydell <[e-mail]>; Shannon
>>> Zhao <[e-mail]>; qemu-arm <[e-mail]>;
>>> [e-mail]; Andrew Jones <[e-mail]>;
>>> [e-mail]; Samer El-Haj-Mahmoud <Samer.El-Haj-
>>> [e-mail]>; Al Stone <[e-mail]>; [e-mail]
>>> Subject: Re: [PATCH for-6.2] hw/arm/virt_acpi_build: Generate DBG2 table
>>>
>>> Hello Ard,
>>> On 8/10/21 11:36 AM, Ard Biesheuvel wrote:
>>>> On Tue, 10 Aug 2021 at 10:31, Eric Auger <[e-mail]> 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?
>>> I don't know exactly what they are. Adding Al and Jon in the loop they
>>> have more information about this.
>>> How did you resolve the issue for EDK2
>>> (DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c)?
>>>>  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.
>>> It seems there are kernel FW test suites that check all mandated tables
>>> are available and they currently fail for ARM virt.
>>> Indeed from a function pov, I don't know much about its usage on ARM.
>>>
>>> Maybe the SBBR spec should not flag the DBG2 as mandatory and test
>>> suites shall be updated. I think this should be clarified at ARM then,
>>> all the more so if there are legal issues as its spec is owned by Microsoft?
>>>
>> DBG2 has been required in SBBR since SBBR ver 1.0 (published 2016, with the 0.9 draft since 2014)
>> https://developer.arm.com/documentation/den0044/b/?lang=en
>>
>> SBBR requires DBG2 because Windows requires it on all systems: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-system-description-tables#debug-port-table-2-dbg2 , and Windows is one of the key OSes targeted by SBBR.
>>
>> The DBG2 (and SPCR) spec license issue has been resolved since August 2015. Microsoft updated both specs with identical license language, giving patent rights for implementations under the Microsoft Community Promise, and the Open OWF 1.0. This Foundation.
OK thank you for confirming all the previously known DBG2 legal issues
were resolved.
>>
>> DBG2: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table
>> SPCR: https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
>>
> Thanks Samer, for stating this on record here - and apologies for
> suggesting that this was another frivolous addition to a recent SBBR
> revision.
>
> As for the difference between the two: SPCR describes the serial
> console, which is an actual interactive console used for maintenance,
> which exists in addition to the full blown Windows GUI, which is
> always the primary interface.
https://bugzilla.redhat.com/show_bug.cgi?id=1661288#c56
seems to contradict the above statement. Do I understand correctly that
if the SPCR is exposed and if "console=ttyAMA0 console=tty0" is not set
in the kernel params, the serial becomes the primary console instead of
the tty0 (hence your choice of developping console preference DXE in
tianocore)?

Anyway I will respin this patch and use the build_append_int_noprefix() API.

Thanks

Eric
>
> DBG2 is used as a debug port, which is used for the kernel debugger,
> if I am not mistaken. So SPCR and DBG2 are complementary, and it does
> make sense to have both.
>

Patch

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