Re: [Qemu-arm] [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 06:53, Shannon Zhao wrote:
> 
> 
> On 2017/12/8 23:02, Peter Maydell wrote:
>> Add the second UART to the ACPI tables.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>> Pure guesswork, as I don't have a UEFI setup to hand and
>> am not familiar with ACPI table formats either...
>> ---
>>  hw/arm/virt-acpi-build.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3d78ff6..a38287b 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker 
>> *linker,
>>  static void
>>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>  {
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>      Aml *scope, *dsdt;
>>      const MemMapEntry *memmap = vms->memmap;
>>      const int *irqmap = vms->irqmap;
>> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>> +    if (!vmc->no_second_uart) {
>> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
>> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
>> +    }
>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>>
> Reviewed-by: Shannon Zhao <address@hidden>
> 

(Responding here so I don't have to manually update Shannon's email
address while replying to patch 3/3 directly.)

acpi_dsdt_add_uart() does:

    Aml *dev = aml_device("COM0");
    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
    aml_append(dev, aml_name_decl("_UID", aml_int(0)));

The device name should be unique. Plus, within the same _HID, the _UID
should be unique as well. I suggest adding another unsigned parameter to
acpi_dsdt_add_uart(), and hooking it into COMx and _UID.


BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
can see from the firmware code, the firmware will use the PL011 whose
description comes first in the DTB (and ignore the other PL011), in an
fdt_next_node() traversal. Is that OK for the intended use case?
(Perhaps I should have asked this under the second patch, which updates
the DTB generator.)

Thanks,
Laszlo