| Message ID | 20210927131732.63801-3-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt_acpi_build: Generate DBG2 table | expand |
On Mon, 27 Sep 2021 15:17:31 +0200 Eric Auger <eric.auger@redhat.com> 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.auger@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) > +{ > + AcpiTable table = { .sig = "DBG2", .rev = 3, .oem_id = vms->oem_id, > + .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[] */ + > + 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); s/0x1000/vms->memmap[VIRT_UART].size/? > + > + /* 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);
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.auger@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." > + .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. > + 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
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.auger@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 / > > >> + .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. > >> + 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 >
On Wed, 6 Oct 2021 11:57:07 +0200 Eric Auger <eric.auger@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.auger@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 > > >
Hi, On 10/6/21 3:46 PM, Igor Mammedov wrote: > On Wed, 6 Oct 2021 11:57:07 +0200 > Eric Auger <eric.auger@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.auger@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 Hum OK, sorry was confused by the 2 different revision fields. I will fix that. > >>> >>>> + .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 OK Eric > >>> >>>> + 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 >>>
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) +{ + AcpiTable table = { .sig = "DBG2", .rev = 3, .oem_id = vms->oem_id, + .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[] */ + + 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);
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.auger@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(-)