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 <imamm...@redhat.com>
> ---
> CC: drjo...@redhat.com
> CC: peter.mayd...@linaro.org
> CC: shannon.zha...@gmail.com
> CC: qemu-...@nongnu.org
> ---
> include/hw/acpi/acpi-defs.h | 25 -------------
> hw/arm/virt-acpi-build.c | 75 ++++++++++++++++++++++++-------------
> 2 files changed, 48 insertions(+), 52 deletions(-)
>
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 012c4ffb3a..0b375e7589 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -131,29 +131,4 @@ struct AcpiFacsDescriptorRev1 {
> } QEMU_PACKED;
> typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
>
> -/*
> - * Generic Timer Description Table (GTDT)
> - */
> -#define ACPI_GTDT_INTERRUPT_MODE_LEVEL (0 << 0)
> -#define ACPI_GTDT_INTERRUPT_MODE_EDGE (1 << 0)
> -#define ACPI_GTDT_CAP_ALWAYS_ON (1 << 2)
> -
> -struct AcpiGenericTimerTable {
> - ACPI_TABLE_HEADER_DEF
> - uint64_t counter_block_addresss;
> - uint32_t reserved;
> - uint32_t secure_el1_interrupt;
> - uint32_t secure_el1_flags;
> - uint32_t non_secure_el1_interrupt;
> - uint32_t non_secure_el1_flags;
> - uint32_t virtual_timer_interrupt;
> - uint32_t virtual_timer_flags;
> - uint32_t non_secure_el2_interrupt;
> - uint32_t non_secure_el2_flags;
> - uint64_t counter_read_block_address;
> - uint32_t platform_timer_count;
> - uint32_t platform_timer_offset;
> -} QEMU_PACKED;
> -typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
> -
> #endif
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index e8553dcae5..8f28e82760 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -455,40 +455,61 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> acpi_table_composed(linker, &table);
> }
>
> -/* GTDT */
> +/*
> + * ACPI spec, Revision 5.1
> + * 5.2.24 Generic Timer Description Table (GTDT)
> + */
> static void
> build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> - int gtdt_start = table_data->len;
> - AcpiGenericTimerTable *gtdt;
> - uint32_t irqflags;
> -
> - if (vmc->claim_edge_triggered_timers) {
> - irqflags = ACPI_GTDT_INTERRUPT_MODE_EDGE;
> - } else {
> - irqflags = ACPI_GTDT_INTERRUPT_MODE_LEVEL;
> - }
> -
> - gtdt = acpi_data_push(table_data, sizeof *gtdt);
> - /* The interrupt values are the same with the device tree when adding 16
> */
> - gtdt->secure_el1_interrupt = cpu_to_le32(ARCH_TIMER_S_EL1_IRQ + 16);
> - gtdt->secure_el1_flags = cpu_to_le32(irqflags);
> -
> - gtdt->non_secure_el1_interrupt = cpu_to_le32(ARCH_TIMER_NS_EL1_IRQ + 16);
> - gtdt->non_secure_el1_flags = cpu_to_le32(irqflags |
> - ACPI_GTDT_CAP_ALWAYS_ON);
> + /*
> + * Table 5-117 Flag Definitions
> + * set only "Timer interrupt Mode" and assume "Timer Interrupt
> + * polarity" bit as '0: Interrupt is Active high'
> + */
> + uint32_t irqflags = vmc->claim_edge_triggered_timers ?
> + 1 : /* Interrupt is Edge triggered */
> + 0; /* Interrupt is Level triggered */
> + AcpiTable table = { .sig = "GTDT", .rev = 2, .oem_id = vms->oem_id,
> + .oem_table_id = vms->oem_table_id };
>
> - gtdt->virtual_timer_interrupt = cpu_to_le32(ARCH_TIMER_VIRT_IRQ + 16);
> - gtdt->virtual_timer_flags = cpu_to_le32(irqflags);
> + acpi_init_table(&table, table_data);
>
> - gtdt->non_secure_el2_interrupt = cpu_to_le32(ARCH_TIMER_NS_EL2_IRQ + 16);
> - gtdt->non_secure_el2_flags = cpu_to_le32(irqflags);
> + /* CntControlBase Physical Address */
> + /* FIXME: invalid value, should be 0xFFFFFFFFFFFFFFFF if not impl. ? */
> + build_append_int_noprefix(table_data, 0, 8);
> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> + /*
> + * FIXME: clarify comment:
> + * The interrupt values are the same with the device tree when adding 16
> + */
> + /* Secure EL1 timer GSIV */
> + build_append_int_noprefix(table_data, ARCH_TIMER_S_EL1_IRQ + 16, 4);
> + /* Secure EL1 timer Flags */
> + build_append_int_noprefix(table_data, irqflags, 4);
> + /* Non-Secure EL1 timer GSIV */
> + build_append_int_noprefix(table_data, ARCH_TIMER_NS_EL1_IRQ + 16, 4);
> + /* Non-Secure EL1 timer Flags */
> + build_append_int_noprefix(table_data, irqflags |
> + 1UL << 2, /* Always-on Capability */
> + 4);
> + /* Virtual timer GSIV */
> + build_append_int_noprefix(table_data, ARCH_TIMER_VIRT_IRQ + 16, 4);
> + /* Virtual Timer Flags */
> + build_append_int_noprefix(table_data, irqflags, 4);
> + /* Non-Secure EL2 timer GSIV */
> + build_append_int_noprefix(table_data, ARCH_TIMER_NS_EL2_IRQ + 16, 4);
> + /* Non-Secure EL2 timer Flags */
> + build_append_int_noprefix(table_data, irqflags, 4);
> + /* CntReadBase Physical address */
> + build_append_int_noprefix(table_data, 0, 8);
> + /* Platform Timer Count */
> + build_append_int_noprefix(table_data, 0, 4);
> + /* Platform Timer Offset */
> + build_append_int_noprefix(table_data, 0, 4);
>
> - build_header(linker, table_data,
> - (void *)(table_data->data + gtdt_start), "GTDT",
> - table_data->len - gtdt_start, 2, vms->oem_id,
> - vms->oem_table_id);
> + acpi_table_composed(linker, &table);
> }
>
> /*
>
Reviewed-by: Eric Auger <eric.au...@redhat.com>
Eric
