Re: [PATCH v2 11/17] libxl/arm: Construct ACPI DSDT table

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/6/24 1:03, Julien Grall wrote:
> Hi Shannon,
> 
> On 23/06/16 04:16, Shannon Zhao wrote:
> 
> [...]
> 
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 264b6ef..5347480 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -77,7 +77,29 @@ endif
>>
>>   LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
>>   LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
>> libxl_libfdt_compat.o
>> -LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o
>> +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o libxl_dsdt_anycpu_arm.o
>> +
>> +vpath iasl $(PATH)
>> +libxl_mk_dsdt_arm: libxl_mk_dsdt_arm.c
>> +    $(CC) $(CFLAGS) -o $@ libxl_mk_dsdt_arm.c
>> +
>> +libxl_dsdt_anycpu_arm.asl: libxl_empty_dsdt_arm.asl libxl_mk_dsdt_arm
>> +    awk 'NR > 1 {print s} {s=$$0}' $< > $@
>> +    ./libxl_mk_dsdt_arm >> $@
>> +
>> +libxl_dsdt_anycpu_arm.c: %.c: iasl %.asl
>> +    iasl -vs -p $* -tc $*.asl
>> +    sed -e 's/AmlCode/$*/g' $*.hex >$@
>> +    echo "int $*_len=sizeof($*);" >>$@
>> +    rm -f $*.aml $*.hex
>> +
> 
> I don't like the idea to add iasl as a dependency for all ARM platforms.
> For instance ARMv7 platform will not use ACPI, but we still ask users to
> install iasl. So I think we should allow the user to opt-in/opt-out for
> ACPI.
> 
> Any opinions?
> 
I agree. But how to exclude for ARMv7. I notice it only has the option
CONFIG_ARM which doesn't distinguish ARM32 and ARM64.

>> +iasl:
>> +    @echo
>> +    @echo "ACPI ASL compiler (iasl) is needed"
>> +    @echo "Download and install Intel ACPI CA from"
>> +    @echo "http://acpica.org/downloads/"
>> +    @echo
>> +    @exit 1
> 
> It is really a pain to discover the dependency in the middle of a build.
> The presence of iasl should be done by the configure.
> 
>>
>>   libxl_arm_acpi.o: libxl_arm_acpi.c
>>       $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c
>> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
>> index 353d774..45fc354 100644
>> --- a/tools/libxl/libxl_arm_acpi.c
>> +++ b/tools/libxl/libxl_arm_acpi.c
>> @@ -54,6 +54,9 @@ enum {
>>       NUMS,
>>   };
>>
>> +extern unsigned char libxl_dsdt_anycpu_arm[];
>> +extern int libxl_dsdt_anycpu_arm_len;
> 
> Not sure this is the right place to mention it, but I don't find the
> actual declaration.
> 
The declarations are in libxl_dsdt_anycpu_arm.c which is generated by
iasl. You can see that in the Makefile above.

> Both variables should be const and _hidden. Also, the *_len should be at
> least const int.
> 
>> +
>>   struct acpitable {
>>       void *table;
>>       size_t size;
>> @@ -256,6 +259,17 @@ static void make_acpi_fadt(libxl__gc *gc, struct
>> xc_dom_image *dom)
>>       dom->acpitable_size += ROUNDUP(acpitables[FADT].size, 3);
>>   }
>>
>> +static void make_acpi_dsdt(libxl__gc *gc, struct xc_dom_image *dom)
>> +{
>> +    acpitables[DSDT].table = libxl__zalloc(gc,
>> libxl_dsdt_anycpu_arm_len);
>> +    memcpy(acpitables[DSDT].table, libxl_dsdt_anycpu_arm,
>> +           libxl_dsdt_anycpu_arm_len);
>> +
>> +    acpitables[DSDT].size = libxl_dsdt_anycpu_arm_len;
>> +    /* Align to 64bit. */
>> +    dom->acpitable_size += ROUNDUP(acpitables[DSDT].size, 3);
>> +}
>> +
>>   int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>>                           libxl__domain_build_state *state,
>>                           struct xc_dom_image *dom)
>> @@ -284,6 +298,7 @@ int libxl__prepare_acpi(libxl__gc *gc,
>> libxl_domain_build_info *info,
>>       return rc;
>>
>>       make_acpi_fadt(gc, dom);
>> +    make_acpi_dsdt(gc, dom);
>>
>>       return 0;
>>   }
>> diff --git a/tools/libxl/libxl_arm_acpi.h b/tools/libxl/libxl_arm_acpi.h
>> index 9b58de6..b0fd9ce 100644
>> --- a/tools/libxl/libxl_arm_acpi.h
>> +++ b/tools/libxl/libxl_arm_acpi.h
>> @@ -19,6 +19,8 @@
>>
>>   #include <xc_dom.h>
>>
>> +#define DOMU_MAX_VCPUS 128
>> +
> 
> I would rather define the maximum number of VCPUS in public/arch_arm.h
> to avoid defining the current number of vCPUs supported in multiple places.
> 
>>   int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>>                           libxl__domain_build_state *state,
>>                           struct xc_dom_image *dom);
>> diff --git a/tools/libxl/libxl_empty_dsdt_arm.asl
>> b/tools/libxl/libxl_empty_dsdt_arm.asl
>> new file mode 100644
>> index 0000000..005fa6a
>> --- /dev/null
>> +++ b/tools/libxl/libxl_empty_dsdt_arm.asl
>> @@ -0,0 +1,22 @@
>> +/******************************************************************************
>>
>> + * DSDT for Xen ARM DomU
>> + *
>> + * Copyright (c) 2004, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but
>> WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
>> License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> along with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +DefinitionBlock ("DSDT.aml", "DSDT", 3, "XenARM", "Xen DSDT", 1)
>> +{
>> +
>> +}
>> diff --git a/tools/libxl/libxl_mk_dsdt_arm.c
>> b/tools/libxl/libxl_mk_dsdt_arm.c
>> new file mode 100644
>> index 0000000..96fadbd
>> --- /dev/null
>> +++ b/tools/libxl/libxl_mk_dsdt_arm.c
> 
> Can we share the code from tools/firmware/acpi/mk_dsdt.c?
>
Yeah, we can share push_block(), pop_block() stmt() and indent() but the
main() function is totally different since there are only the processor
device objects for ARM DSDT but there are many other things in x86.

I think that since Boris will move the codes under
tools/firmware/hvmloader/acpi to other place, after that we could see
how to share codes then.

>> @@ -0,0 +1,94 @@
>> +#include <stdio.h>
>> +#include <stdarg.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include "libxl_arm_acpi.h"
>> +
>> +static unsigned int indent_level;
>> +
>> +static void indent(void)
>> +{
>> +    unsigned int i;
>> +    for ( i = 0; i < indent_level; i++ )
>> +        printf("    ");
>> +}
>> +
>> +static __attribute__((format(printf, 2, 3)))
>> +void _stmt(const char *name, const char *fmt, ...)
>> +{
>> +    va_list args;
>> +
>> +    indent();
>> +    printf("%s", name);
>> +
>> +    if ( !fmt )
>> +        return;
>> +
>> +    printf(" ( ");
>> +    va_start(args, fmt);
>> +    vprintf(fmt, args);
>> +    va_end(args);
>> +    printf(" )");
>> +}
>> +
>> +#define stmt(n, f, a...)                        \
>> +    do {                                        \
>> +        _stmt(n, f , ## a );                    \
>> +        printf("\n");                           \
>> +    } while (0)
>> +
>> +#define push_block(n, f, a...)                  \
>> +    do {                                        \
>> +        _stmt(n, f , ## a );                    \
>> +        printf(" {\n");                         \
>> +        indent_level++;                         \
>> +    } while (0)
>> +
>> +static void pop_block(void)
>> +{
>> +    indent_level--;
>> +    indent();
>> +    printf("}\n");
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    unsigned int cpu, max_cpus = DOMU_MAX_VCPUS;
>> +
>> +    /**** DSDT DefinitionBlock start ****/
>> +    /* (we append to existing DSDT definition block) */
>> +    indent_level++;
>> +
>> +    /**** Processor start ****/
>> +    push_block("Scope", "\\_SB");
>> +
>> +    /* Define processor objects and control methods. */
>> +    for ( cpu = 0; cpu < max_cpus; cpu++)
>> +    {
>> +        push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu,
>> cpu);
>> +
>> +        stmt("Name", "_HID, \"ACPI0007\"");
>> +        stmt("Name", "_UID, %d", cpu);
>> +
>> +        pop_block();
>> +    }
>> +
>> +    pop_block();
>> +    /**** Processor end ****/
>> +
>> +    pop_block();
>> +    /**** DSDT DefinitionBlock end ****/
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Regards,
> 

-- 
Shannon


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