Re: [PATCH v3 2/3] hw/arm/virt_acpi_build: Generate DBG2 table

[Eric Auger]

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