Re: [Qemu-devel] [PATCH v4 09/20] hw/arm/virt-acpi-build: Generate GTDT table

[Shannon Zhao]

On 2015/4/9 20:42, Alex Bennée wrote:
> 
> Shannon Zhao <address@hidden> writes:
> 
>> From: Shannon Zhao <address@hidden>
>>
>> ACPI v5.1 defines GTDT for ARM devices as a place to describe timer
>> related information in the system. The Arch Timer interrupts must
>> be provided for GTDT.
>>
>> Signed-off-by: Shannon Zhao <address@hidden>
>> Signed-off-by: Shannon Zhao <address@hidden>
>> ---
>>  hw/arm/virt-acpi-build.c    | 30 ++++++++++++++++++++++++++++++
>>  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index c8245ef..a7aba75 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -176,6 +176,33 @@ static void acpi_dsdt_add_virtio(Aml *scope, const 
>> hwaddr *mmio_addrs,
>>      }
>>  }
>>  
>> +/* GTDT */
>> +static void
>> +build_gtdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>> +{
>> +    int gtdt_start = table_data->len;
>> +    const struct acpi_gtdt_info *info = guest_info->gtdt_info;
>> +    AcpiGenericTimerTable *gtdt;
>> +
>> +    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 = info->timer_s_el1;
>> +    gtdt->secure_el1_flags = ACPI_EDGE_SENSITIVE;
>> +
>> +    gtdt->non_secure_el1_interrupt = info->timer_ns_el1;
>> +    gtdt->non_secure_el1_flags = ACPI_EDGE_SENSITIVE;
>> +
>> +    gtdt->virtual_timer_interrupt = info->timer_virt;
>> +    gtdt->virtual_timer_flags = ACPI_EDGE_SENSITIVE;
>> +
>> +    gtdt->non_secure_el2_interrupt = info->timer_ns_el2;
>> +    gtdt->non_secure_el2_flags = ACPI_EDGE_SENSITIVE;
>> +
>> +    build_header(linker, table_data,
>> +                 (void *)(table_data->data + gtdt_start), "GTDT",
>> +                 table_data->len - gtdt_start, 1);
>> +}
>> +
>>  /* MADT */
>>  static void
>>  build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>> @@ -318,6 +345,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
>> AcpiBuildTables *tables)
>>      acpi_add_table(table_offsets, tables_blob);
>>      build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
>>  
>> +    acpi_add_table(table_offsets, tables_blob);
>> +    build_gtdt(tables_blob, tables->linker, guest_info);
>> +
>>      /* Cleanup memory that's no longer used. */
>>      g_array_free(table_offsets, true);
>>  }
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index e343728..ee40a5e 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -318,6 +318,43 @@ struct AcpiMadtGenericDistributor {
>>  typedef struct AcpiMadtGenericDistributor AcpiMadtGenericDistributor;
>>  
>>  /*
>> + * Generic Timer Description Table (GTDT)
>> + */
>> +
>> +#define ACPI_GTDT_INTERRUPT_MODE        (1)
> 
> (1 << 0) would be consistent 
> 

Ok

>> +#define ACPI_GTDT_INTERRUPT_POLARITY    (1<<1)
>> +#define ACPI_GTDT_ALWAYS_ON             (1<<2)
> 
> Also spaces (n << m)
> 

will fix.

>> +
>> +/* Triggering */
>> +
>> +#define ACPI_LEVEL_SENSITIVE            (uint8_t) 0x00
>> +#define ACPI_EDGE_SENSITIVE             (uint8_t) 0x01
>> +
>> +/* Polarity */
>> +
>> +#define ACPI_ACTIVE_HIGH                (uint8_t) 0x00
>> +#define ACPI_ACTIVE_LOW                 (uint8_t) 0x01
>> +#define ACPI_ACTIVE_BOTH                (uint8_t) 0x02
> 
> I'd wrap those cast defines, e.g:
> 
>     #define ACPI_ACTIVE_BOTH                ((uint8_t) 0x02)
> 

Ok, will fix. Thanks.

>> +
>> +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;
>> +
>> +/*
>>   * HPET Description Table
>>   */
>>  struct Acpi20Hpet {
>