| Message ID | 20210810083057.99651-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-6.2] hw/arm/virt_acpi_build: Generate DBG2 table | expand |
On Tue, 10 Aug 2021 at 10:31, Eric Auger <eric.auger@redhat.com> wrote: > > ARM SBBR specification mandates DBG2 table (Debug Port Table 2). > this latter allows to describe one or more debug ports. > > Generate an DBG2 table featuring a single debug port, the PL011. > > The DBG2 specification can be found at: > https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table?redirectedfrom=MSDN > Have the legal issues around this table been resolved in the mean time? Also, any clue why this table is mandatory to begin with? The SBBR has been very trigger happy lately with making things mandatory that aren't truly required from a functional perspective. > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > Tested by comparing the content with the table generated > by EDK2 along with the SBSA-REF machine (code generated by > DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c). > > I reused the Generic Address Structure filled by QEMU in the SPCR, ie. > bit_width = 8 and byte access. While EDK2 sets bit_width = 32 and > dword access. Also the name exposed by acpica tools is different: > 'COM0' in my case where '\_SB.COM0' in SBSA-REF case? > --- > hw/arm/virt-acpi-build.c | 77 ++++++++++++++++++++++++++++++++++++- > include/hw/acpi/acpi-defs.h | 50 ++++++++++++++++++++++++ > 2 files changed, 126 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 037cc1fd82..35f27b41df 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -563,6 +563,78 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > vms->oem_table_id); > } > > +#define ACPI_DBG2_PL011_UART_LENGTH 0x1000 > + > +/* DBG2 */ > +static void > +build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > +{ > + int addr_offset, addrsize_offset, namespace_offset, namespace_length; > + const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART]; > + struct AcpiGenericAddress *base_address; > + int dbg2_start = table_data->len; > + AcpiDbg2Device *dbg2dev; > + char name[] = "COM0"; > + AcpiDbg2Table *dbg2; > + uint32_t *addr_size; > + uint8_t *namespace; > + > + dbg2 = acpi_data_push(table_data, sizeof *dbg2); > + dbg2->info_offset = sizeof *dbg2; > + dbg2->info_count = 1; > + > + /* debug device info structure */ > + > + dbg2dev = acpi_data_push(table_data, sizeof(AcpiDbg2Device)); > + > + dbg2dev->revision = 0; > + namespace_length = sizeof name; > + dbg2dev->length = sizeof *dbg2dev + sizeof(struct AcpiGenericAddress) + > + 4 + namespace_length; > + dbg2dev->register_count = 1; > + > + addr_offset = sizeof *dbg2dev; > + addrsize_offset = addr_offset + sizeof(struct AcpiGenericAddress); > + namespace_offset = addrsize_offset + 4; > + > + dbg2dev->namepath_length = cpu_to_le16(namespace_length); > + dbg2dev->namepath_offset = cpu_to_le16(namespace_offset); > + dbg2dev->oem_data_length = cpu_to_le16(0); > + dbg2dev->oem_data_offset = cpu_to_le16(0); /* No OEM data is present */ > + dbg2dev->port_type = cpu_to_le16(ACPI_DBG2_SERIAL_PORT); > + dbg2dev->port_subtype = cpu_to_le16(ACPI_DBG2_ARM_PL011); > + > + dbg2dev->base_address_offset = cpu_to_le16(addr_offset); > + dbg2dev->address_size_offset = cpu_to_le16(addrsize_offset); > + > + /* > + * variable length content: > + * BaseAddressRegister[1] > + * AddressSize[1] > + * NamespaceString[1] > + */ > + > + base_address = acpi_data_push(table_data, > + sizeof(struct AcpiGenericAddress)); > + > + base_address->space_id = AML_SYSTEM_MEMORY; > + base_address->bit_width = 8; > + base_address->bit_offset = 0; > + base_address->access_width = 1; > + base_address->address = cpu_to_le64(uart_memmap->base); > + > + addr_size = acpi_data_push(table_data, sizeof *addr_size); > + *addr_size = cpu_to_le32(ACPI_DBG2_PL011_UART_LENGTH); > + > + namespace = acpi_data_push(table_data, namespace_length); > + memcpy(namespace, name, namespace_length); > + > + build_header(linker, table_data, > + (void *)(table_data->data + dbg2_start), "DBG2", > + table_data->len - dbg2_start, 3, vms->oem_id, > + vms->oem_table_id); > +} > + > /* MADT */ > static void > build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > @@ -790,7 +862,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > dsdt = tables_blob->len; > build_dsdt(tables_blob, tables->linker, vms); > > - /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */ > + /* FADT MADT GTDT MCFG SPCR DBG2 pointed to by RSDT */ > acpi_add_table(table_offsets, tables_blob); > build_fadt_rev5(tables_blob, tables->linker, vms, dsdt); > > @@ -813,6 +885,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > acpi_add_table(table_offsets, tables_blob); > build_spcr(tables_blob, tables->linker, vms); > > + acpi_add_table(table_offsets, tables_blob); > + build_dbg2(tables_blob, tables->linker, vms); > + > if (vms->ras) { > build_ghes_error_table(tables->hardware_errors, tables->linker); > acpi_add_table(table_offsets, tables_blob); > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index cf9f44299c..bdb2ebed2c 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -618,4 +618,54 @@ struct AcpiIortRC { > } QEMU_PACKED; > typedef struct AcpiIortRC AcpiIortRC; > > +/* DBG2 */ > + > +/* Types for port_type field above */ > + > +#define ACPI_DBG2_SERIAL_PORT 0x8000 > +#define ACPI_DBG2_1394_PORT 0x8001 > +#define ACPI_DBG2_USB_PORT 0x8002 > +#define ACPI_DBG2_NET_PORT 0x8003 > + > +/* Subtypes for port_subtype field above */ > + > +#define ACPI_DBG2_16550_COMPATIBLE 0x0000 > +#define ACPI_DBG2_16550_SUBSET 0x0001 > +#define ACPI_DBG2_ARM_PL011 0x0003 > +#define ACPI_DBG2_ARM_SBSA_32BIT 0x000D > +#define ACPI_DBG2_ARM_SBSA_GENERIC 0x000E > +#define ACPI_DBG2_ARM_DCC 0x000F > +#define ACPI_DBG2_BCM2835 0x0010 > + > +#define ACPI_DBG2_1394_STANDARD 0x0000 > + > +#define ACPI_DBG2_USB_XHCI 0x0000 > +#define ACPI_DBG2_USB_EHCI 0x0001 > + > +/* Debug Device Information Subtable */ > + > +struct AcpiDbg2Device { > + uint8_t revision; > + uint16_t length; > + uint8_t register_count; /* Number of base_address registers */ > + uint16_t namepath_length; > + uint16_t namepath_offset; > + uint16_t oem_data_length; > + uint16_t oem_data_offset; > + uint16_t port_type; > + uint16_t port_subtype; > + uint8_t reserved[2]; > + uint16_t base_address_offset; > + uint16_t address_size_offset; > +} QEMU_PACKED; > +typedef struct AcpiDbg2Device AcpiDbg2Device; > + > +struct AcpiDbg2Table { > + ACPI_TABLE_HEADER_DEF /* ACPI common table header */ > + uint32_t info_offset; /* offset to the first debug struct */ > + uint32_t info_count; /* number of debug device info struct entries */ > + uint8_t dbg2_device_info[]; > +} QEMU_PACKED; > +typedef struct AcpiDbg2Table AcpiDbg2Table; > + > #endif > -- > 2.26.3 >
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. 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 >>
On Tue, 10 Aug 2021 10:30:57 +0200 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 > > Signed-off-by: Eric Auger <eric.auger@redhat.com> 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
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. 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 > >> >
Hi Igor, On 8/10/21 12:52 PM, Igor Mammedov wrote: > On Tue, 10 Aug 2021 10:30:57 +0200 > 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 >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > 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
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 >>>>
On Tue, 10 Aug 2021 14:05:40 +0200 Eric Auger <eric.auger@redhat.com> wrote: > Hi Igor, > > On 8/10/21 12:52 PM, Igor Mammedov wrote: > > On Tue, 10 Aug 2021 10:30:57 +0200 > > 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 > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > 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/qemu-devel@nongnu.org/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 >
Hi Igor, On 8/10/21 2:20 PM, Igor Mammedov wrote: > On Tue, 10 Aug 2021 14:05:40 +0200 > Eric Auger <eric.auger@redhat.com> wrote: > >> Hi Igor, >> >> On 8/10/21 12:52 PM, Igor Mammedov wrote: >>> On Tue, 10 Aug 2021 10:30:57 +0200 >>> 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 >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> 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/qemu-devel@nongnu.org/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
> -----Original Message----- > From: Eric Auger <eric.auger@redhat.com> > Sent: Tuesday, August 10, 2021 6:25 AM > To: Ard Biesheuvel <ardb@kernel.org> > Cc: eric.auger.pro@gmail.com; Michael S. Tsirkin <mst@redhat.com>; Igor > Mammedov <imammedo@redhat.com>; Philippe Mathieu-Daudé > <philmd@redhat.com>; Peter Maydell <peter.maydell@linaro.org>; Shannon > Zhao <shannon.zhaosl@gmail.com>; qemu-arm <qemu-arm@nongnu.org>; > qemu-devel@nongnu.org; Andrew Jones <drjones@redhat.com>; > gshan@redhat.com; Samer El-Haj-Mahmoud <Samer.El-Haj- > Mahmoud@arm.com>; Al Stone <ahs3@redhat.com>; jcm@redhat.com > 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 <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. > > 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 <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 > >> 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.
On Tue, 10 Aug 2021 at 15:11, Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com> wrote: > > > > > -----Original Message----- > > From: Eric Auger <eric.auger@redhat.com> > > Sent: Tuesday, August 10, 2021 6:25 AM > > To: Ard Biesheuvel <ardb@kernel.org> > > Cc: eric.auger.pro@gmail.com; Michael S. Tsirkin <mst@redhat.com>; Igor > > Mammedov <imammedo@redhat.com>; Philippe Mathieu-Daudé > > <philmd@redhat.com>; Peter Maydell <peter.maydell@linaro.org>; Shannon > > Zhao <shannon.zhaosl@gmail.com>; qemu-arm <qemu-arm@nongnu.org>; > > qemu-devel@nongnu.org; Andrew Jones <drjones@redhat.com>; > > gshan@redhat.com; Samer El-Haj-Mahmoud <Samer.El-Haj- > > Mahmoud@arm.com>; Al Stone <ahs3@redhat.com>; jcm@redhat.com > > 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 <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. > > > > 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.
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 > <Samer.El-Haj-Mahmoud@arm.com> wrote: >> >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Sent: Tuesday, August 10, 2021 6:25 AM >>> To: Ard Biesheuvel <ardb@kernel.org> >>> Cc: eric.auger.pro@gmail.com; Michael S. Tsirkin <mst@redhat.com>; Igor >>> Mammedov <imammedo@redhat.com>; Philippe Mathieu-Daudé >>> <philmd@redhat.com>; Peter Maydell <peter.maydell@linaro.org>; Shannon >>> Zhao <shannon.zhaosl@gmail.com>; qemu-arm <qemu-arm@nongnu.org>; >>> qemu-devel@nongnu.org; Andrew Jones <drjones@redhat.com>; >>> gshan@redhat.com; Samer El-Haj-Mahmoud <Samer.El-Haj- >>> Mahmoud@arm.com>; Al Stone <ahs3@redhat.com>; jcm@redhat.com >>> 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 <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. >>> >>> 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. >
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
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 <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(-)