Re: [PATCH v2 31/35] acpi: arm/virt: build_spcr: use acpi_init_table()/acpi_table_composed() instead of build_header()

[Eric Auger]

Hi Igor,

On 7/8/21 5:46 PM, Igor Mammedov wrote:
> it replaces error-prone pointer arithmetic for build_header() API,
> with 2 calls to start and finish table creation,
> which hides offsets magic from API user.
> 
> while at it, replace packed structure with endian agnostic
> build_append_FOO() API.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: drjones@redhat.com
> CC: peter.maydell@linaro.org
> CC: shannon.zhaosl@gmail.com
> CC: qemu-arm@nongnu.org
> ---
>  include/hw/acpi/acpi-defs.h | 32 -----------------
>  hw/arm/virt-acpi-build.c    | 68 ++++++++++++++++++++++---------------
>  2 files changed, 41 insertions(+), 59 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 6f2f08a9de..012c4ffb3a 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -117,38 +117,6 @@ typedef struct AcpiFadtData {
>  #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
>  #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
>  
> -/*
> - * Serial Port Console Redirection Table (SPCR), Rev. 1.02
> - *
> - * For .interface_type see Debug Port Table 2 (DBG2) serial port
> - * subtypes in Table 3, Rev. May 22, 2012
> - */
> -struct AcpiSerialPortConsoleRedirection {
> -    ACPI_TABLE_HEADER_DEF
> -    uint8_t  interface_type;
> -    uint8_t  reserved1[3];
> -    struct AcpiGenericAddress base_address;
> -    uint8_t  interrupt_types;
> -    uint8_t  irq;
> -    uint32_t gsi;
> -    uint8_t  baud;
> -    uint8_t  parity;
> -    uint8_t  stopbits;
> -    uint8_t  flowctrl;
> -    uint8_t  term_type;
> -    uint8_t  reserved2;
> -    uint16_t pci_device_id;
> -    uint16_t pci_vendor_id;
> -    uint8_t  pci_bus;
> -    uint8_t  pci_slot;
> -    uint8_t  pci_func;
> -    uint32_t pci_flags;
> -    uint8_t  pci_seg;
> -    uint32_t reserved3;
> -} QEMU_PACKED;
> -typedef struct AcpiSerialPortConsoleRedirection
> -               AcpiSerialPortConsoleRedirection;
> -
>  /*
>   * ACPI 1.0 Firmware ACPI Control Structure (FACS)
>   */
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a07540affb..e8553dcae5 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -349,39 +349,53 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>      acpi_table_composed(linker, &table);
>  }
>  
> +/*
> + * Serial Port Console Redirection Table (SPCR)
> + * Rev: 1.07
> + */
was
/*
 * Serial Port Console Redirection Table (SPCR), Rev. 1.02
 */

By the way
https://uefi.org/acpi/specs does not list 1.02 nor 1.07
https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
?

>  static void
>  build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> -    AcpiSerialPortConsoleRedirection *spcr;
> -    const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART];
> -    int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE;
> -    int spcr_start = table_data->len;
> -
> -    spcr = acpi_data_push(table_data, sizeof(*spcr));
> -
> -    spcr->interface_type = 0x3;    /* ARM PL011 UART */
> -
> -    spcr->base_address.space_id = AML_AS_SYSTEM_MEMORY;
> -    spcr->base_address.bit_width = 8;
> -    spcr->base_address.bit_offset = 0;
> -    spcr->base_address.access_width = 1;
> -    spcr->base_address.address = cpu_to_le64(uart_memmap->base);
> -
> -    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> -    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
> +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> +                        .oem_table_id = vms->oem_table_id };
>  
> -    spcr->baud = 3;                /* Baud Rate: 3 = 9600 */
> -    spcr->parity = 0;              /* No Parity */
> -    spcr->stopbits = 1;            /* 1 Stop bit */
> -    spcr->flowctrl = (1 << 1);     /* Bit[1] = RTS/CTS hardware flow control 
> */
> -    spcr->term_type = 0;           /* Terminal Type: 0 = VT100 */
> +    acpi_init_table(&table, table_data);
>  
> -    spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
> -    spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
> +    /* Interface Type */
> +    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> +    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> +    /* Base Address */
> +    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1,
> +                     vms->memmap[VIRT_UART].base);
> +    /* Interrupt Type */
> +    build_append_int_noprefix(table_data,
> +        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> +    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> +    /* Global System Interrupt */
> +    build_append_int_noprefix(table_data,
> +                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> +    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> +    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
> +    /* Stop Bits */
> +    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> +    /* Flow Control */
> +    build_append_int_noprefix(table_data,
> +        (1 << 1) /* RTS/CTS hardware flow control */, 1);
> +    /* Terminal Type */
> +    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> +    build_append_int_noprefix(table_data, 0, 1); /* Language */
reserved2 -> language in 1.07? but that's fine
maybe just mention it in the commit msg?
> +    /* PCI Device ID  */
> +    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> +    /* PCI Vendor ID */
> +    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> +    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
> +    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
> +    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
> +    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
> +    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
> +    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>  
> -    build_header(linker, table_data, (void *)(table_data->data + spcr_start),
> -                 "SPCR", table_data->len - spcr_start, 2, vms->oem_id,
> -                 vms->oem_table_id);
> +    acpi_table_composed(linker, &table);
>  }
>  
>  /*
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric