On Wed, 6 Oct 2021 11:57:07 +0200 Eric Auger <eric.au...@redhat.com> wrote:
> Hi, > > On 10/6/21 11:15 AM, Andrew Jones wrote: > > On Mon, Sep 27, 2021 at 03:17:31PM +0200, Eric Auger wrote: > >> ARM SBBR specification mandates DBG2 table (Debug Port Table 2) > >> since v1.0 (ARM DEN0044F 8.3.1.7 DBG2). > >> > >> The DBG2 table 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 > >> "Microsoft Debug Port Table 2 (DBG2)" > >> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table?redirectedfrom=MSDN > >> > >> Signed-off-by: Eric Auger <eric.au...@redhat.com> > >> > >> --- > >> > >> v2 -> v3: > >> Took into account all comments from Igor on v2: > >> mostly style adjustment, revision references > >> > >> v1 -> v2: > >> - rebased on Igor's refactoring > >> --- > >> hw/arm/virt-acpi-build.c | 62 +++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 61 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > >> index 6cec97352b..257d0fee17 100644 > >> --- a/hw/arm/virt-acpi-build.c > >> +++ b/hw/arm/virt-acpi-build.c > >> @@ -616,6 +616,63 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, > >> VirtMachineState *vms) > >> acpi_table_end(linker, &table); > >> } > >> > >> +/* Debug Port Table 2 (DBG2) */ > >> +static void > >> +build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > >> > > nit: I didn't think QEMU liked this style, i.e. QEMU prefers > > > > static void build_dbg2(GArray *table_data, BIOSLinker *linker, > > VirtMachineState *vms) > > > > Eh, I see that hw/arm/virt-acpi-build.c is full of the format you have > > here, so never mind. > > > >> +{ > >> + AcpiTable table = { .sig = "DBG2", .rev = 3, .oem_id = vms->oem_id, > > Can you explain where the .rev = 3 comes from? The spec says "For this > > version of the specification, this value is 0." > The above revision field belongs to the acpi table header. Not to be > mixed with the Revision field of the DBG2 table > which is set below (set to 0): > > + build_append_int_noprefix(table_data, 0, 1); /* Revision */ > > Besides that's a good question. I have rev=3 here but that must come from a > copy/paste. when googling I > found > > https://lists.denx.de/pipermail/u-boot/2015-June/217134.html > /header->revision = 1; /* ACPI 1.0/2.0: 1, ACPI 3.0: 2, ACPI 4.0: 3 */ > Not sure if 3 is the right value though? Igor, please could you advise? > Thanks Eric / > "Table 1. Debug Port Table 2 format" says table revision must be 0 > > > > > >> + .oem_table_id = vms->oem_table_id }; > >> + int dbg2devicelength; > >> + const char name[] = "COM0"; > >> + const int namespace_length = sizeof(name); > >> + > >> + acpi_table_begin(&table, table_data); > >> + > >> + dbg2devicelength = 22 /* BaseAddressRegister[] offset */ + > >> + 12 /* BaseAddressRegister[] */ + > >> + 4 /* AddressSize[] */ + > > I'd personally prefer the '+' before the comment, but maybe Igor has a > > special ACPI code format preference here. indeed '+' before comment should look better > > > >> + namespace_length /* NamespaceString[] */; > >> + > >> + /* OffsetDbgDeviceInfo */ > >> + build_append_int_noprefix(table_data, 44, 4); > >> + /* NumberDbgDeviceInfo */ > >> + build_append_int_noprefix(table_data, 1, 4); > >> + > >> + /* Table 2. Debug Device Information structure format */ > >> + build_append_int_noprefix(table_data, 0, 1); /* Revision */ > >> + build_append_int_noprefix(table_data, dbg2devicelength, 2); /* Length > >> */ > >> + /* NumberofGenericAddressRegisters */ > >> + build_append_int_noprefix(table_data, 1, 1); > >> + /* NameSpaceStringLength */ > >> + build_append_int_noprefix(table_data, namespace_length, 2); > >> + build_append_int_noprefix(table_data, 38, 2); /* > >> NameSpaceStringOffset */ > >> + build_append_int_noprefix(table_data, 0, 2); /* OemDataLength */ > >> + /* OemDataOffset (0 means no OEM data) */ > >> + build_append_int_noprefix(table_data, 0, 2); > >> + > >> + /* Port Type */ > >> + build_append_int_noprefix(table_data, 0x8000 /* Serial */, 2); > >> + /* Port Subtype */ > >> + build_append_int_noprefix(table_data, 0x3 /* ARM PL011 UART */, 2); > >> + build_append_int_noprefix(table_data, 0, 2); /* Reserved */ > >> + /* BaseAddressRegisterOffset */ > >> + build_append_int_noprefix(table_data, 22, 2); > >> + /* AddressSizeOffset */ > >> + build_append_int_noprefix(table_data, 34, 2); > >> + > >> + /* BaseAddressRegister[] */ > >> + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, > >> + vms->memmap[VIRT_UART].base); > >> + > >> + /* AddressSize[] */ > >> + build_append_int_noprefix(table_data, 0x1000 /* Register Space */, 4); > >> + > >> + /* NamespaceString[] */ > >> + g_array_append_vals(table_data, name, namespace_length); > >> + > >> + acpi_table_end(linker, &table); > >> +}; > >> + > >> /* > >> * ACPI spec, Revision 5.1 Errata A > >> * 5.2.12 Multiple APIC Description Table (MADT) > >> @@ -875,7 +932,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); > >> > >> @@ -898,6 +955,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); > >> -- > >> 2.26.3 > >> > > Thanks, > > drew > > >