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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Jun 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> 
> Construct GTDT table with the interrupt information of timers.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_arm_acpi.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> index d5ffedf..de863f4 100644
> --- a/tools/libxl/libxl_arm_acpi.c
> +++ b/tools/libxl/libxl_arm_acpi.c
> @@ -39,6 +39,9 @@ typedef uint64_t u64;
>  #define ACPI_BUILD_APPNAME6 "XenARM"
>  #define ACPI_BUILD_APPNAME4 "Xen "
>  
> +#define ACPI_LEVEL_SENSITIVE            (u8) 0x00
> +#define ACPI_ACTIVE_LOW                 (u8) 0x01
> +
>  enum {
>      RSDP,
>      XSDT,
> @@ -110,6 +113,30 @@ static void make_acpi_xsdt(libxl__gc *gc, struct 
> xc_dom_image *dom)
>      dom->acpitable_size += ROUNDUP(acpitables[XSDT].size, 3);
>  }
>  
> +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.


>  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
> 
> 

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