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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

On 2016/6/7 19:19, Julien Grall wrote:
> Hello Shannon,
> 
> On 31/05/16 06:02, Shannon Zhao wrote:
>> 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;
> 
> Should not we compute the checksum here?
> 
Sure.
>> +}
>> +
>> +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);
> 
> There is no secure EL1 for guest, so this should be 0.
> 
So why does DT add secure EL1 timer in make_timer_node()?

>> +    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. */
> 
> I am not sure what the comment is for.
> 
Oops, I planed to make the length of these tables aligned to 64bit, but
it turns out it's unnecessary. And I forgot to delete this.

>> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
>> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
>> +}
> 
> Regards,
> 

-- 
Shannon


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