diff mbox series

[v2,32/35] acpi: arm/virt: build_gtdt: use acpi_init_table()/acpi_table_composed() instead of build_header()

Message ID 20210708154617.1538485-33-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show
Series acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand

Commit Message

Igor Mammedov July 8, 2021, 3:46 p.m. UTC 
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 | 25 -------------
 hw/arm/virt-acpi-build.c    | 75 ++++++++++++++++++++++++-------------
 2 files changed, 48 insertions(+), 52 deletions(-)

Comments

Eric Auger Sept. 2, 2021, 4:07 p.m. UTC | #1 
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 | 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.auger@redhat.com>

Eric
diff mbox series

Patch

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);
 }
 
 /*