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

[Eric Auger]

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