Re: [PATCH v2 07/17] libxl/arm: Construct ACPI GTDT table

[Julien Grall <julien.grall@arm.com>]

Hi,

On 23/06/16 16:00, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Shannon Zhao wrote:

[...]

>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>> +{
>> +    struct acpi_table_gtdt *gtdt;
>> +    size_t size = sizeof(*gtdt);
>> +
>> +    gtdt = libxl__zalloc(gc, size);
>> +
>> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
>> +    gtdt->non_secure_el1_flags =
>> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
>> +    gtdt->virtual_timer_flags =
>> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +
>> +    make_acpi_header(&gtdt->header, "GTDT", size, 2);
>> +
>> +    acpitables[GTDT].table = gtdt;
>> +    acpitables[GTDT].size = size;
>> +    /* Align to 64bit. */
>> +    dom->acpitable_size += ROUNDUP(acpitables[GTDT].size, 3);
>> +}
>
> Many of this tables look pretty much static. Any reason why we can't
> define them and initialize them on an header somewhere like:
>
>      struct acpi_gtdt xen_acpi_gtdt {
>          .non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
>          .non_secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE) | (ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>          .virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
>          .virtual_timer_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)|(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>      };
>
> it would make the code shorter and easier to read.

I agree with Stefano on this point.

>>   int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>>                           libxl__domain_build_state *state,
>>                           struct xc_dom_image *dom)
>> @@ -132,6 +159,7 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>>
>>       make_acpi_rsdp(gc, dom);
>>       make_acpi_xsdt(gc, dom);
>> +    make_acpi_gtdt(gc, dom);
>>
>>       return 0;
>>   }
>> --
>> 2.0.4
>>
>>
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel