Re: [PATCH RESEND 05/14] libxl/arm: Construct ACPI GTDT table

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

Hello,

On 06/06/16 12:40, Stefano Stabellini wrote:
> On Tue, 31 May 2016, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Construct GTDT table with the interrupt information of timers.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>   tools/libxl/libxl_arm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 9e99159..0fb4f69 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -3,6 +3,7 @@
>>   #include "libxl_libfdt_compat.h"
>>
>>   #include <xc_dom.h>
>> +#include <acpi_defs.h>
>>   #include <stdbool.h>
>>   #include <libfdt.h>
>>   #include <assert.h>
>> @@ -880,13 +881,85 @@ out:
>>       return rc;
>>   }
>>
>> +static void make_acpi_header(struct acpi_table_header *h, const char *sig,
>> +                             int len, uint8_t rev)
>> +{
>> +    memcpy(&h->signature, sig, 4);
>> +    h->length = len;
>> +    h->revision = rev;
>> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>> +    memcpy(h->oem_table_id + 4, sig, 4);
>> +    h->oem_revision = 1;
>> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
>> +    h->asl_compiler_revision = 1;
>> +    h->checksum = 0;
>> +}
>> +
>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>> +{
>> +    struct acpi_gtdt_descriptor *gtdt;
>> +
>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>> +
>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +    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", sizeof(*gtdt), 2);
>> +
>> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
>> +    /* Align to 64bit. */
>> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
>> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
>> +}
>> +
>> +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>> +                        libxl__domain_build_state *state,
>> +                        struct xc_dom_image *dom)
>> +{
>> +    const libxl_version_info *vers;
>> +
>> +    /* convenience aliases */
>> +    xc_domain_configuration_t *xc_config = &state->config;
>> +
>> +    vers = libxl_get_version_info(CTX);
>> +    if (vers == NULL)
>> +        return ERROR_FAIL;
>> +
>> +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
>> +        vers->xen_version_major, vers->xen_version_minor);
>> +
>> +    /* Alloc memory for ACPI blob placeholders. */
>> +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct acpitable_blob));
>> +    dom->acpitable_size = 0;
>> +
>> +    make_acpi_gtdt(gc, dom);
>> +
>> +    return 0;
>> +}
>> +
>>   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>>                                              libxl_domain_build_info *info,
>>                                              libxl__domain_build_state *state,
>>                                              struct xc_dom_image *dom)
>>   {
>> +    int rc;
>> +
>>       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
>> -    return prepare_dtb(gc, info, state, dom);
>> +    rc = prepare_dtb(gc, info, state, dom);
>> +    if (rc)
>> +        return rc;
>> +
>> +    return prepare_acpi(gc, info, state, dom);
>>   }
>
> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> config file enables them, with default off.

The VM system specification for ARM [1] mandates that both ACPI and DT 
should be provided and described the entire VM and its peripheral (see 
the section "Hardware Description").

Furthermore, the user may not know if the guest OS will use ACPI or DT 
For instance Redhat is using ACPI whilst Debian is using DT.

So we have to provide both by default. However, 32-bit domain should 
only have Device-Tree table created.

Anyway, the reason should have been described in the commit message. I 
would split this patch in two: introducing prepare ACPI and then GTDT 
table. So we can provide details in the commit message.

Regards,

[1] 
http://people.linaro.org/~christoffer.dall/VMSystemSpecificationForARM-v2.0-rc1.pdf

-- 
Julien Grall

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