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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/28/2016 07:03 AM, Shannon Zhao wrote:
>
> On 2016/6/27 20:05, Boris Ostrovsky wrote:
>>
>> On 06/27/2016 06:29 AM, Julien Grall wrote:
>>> (CC Boris and Doug)
>>>
>>> Hi Shannon,
>>>
>>> On 27/06/16 07:01, Shannon Zhao wrote:
>>>> On 2016/6/24 1:03, Julien Grall wrote:
>>>>> 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.
>>> I am not sure if we plan to introduce Kconfig for tools. If not, you
>>> can add an option to the configure to enable/disable ACPI for guest.
>>>
>>> This would be gated by the presence of "iasl".
>>>
>>> [...]
>>>
>>>>>> 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.
>>> I would prefer if we discuss about it now in order to avoid code
>>> duplication (I have CCed Boris).
>>>
>>> For instance we can create a new directory under tools for mk_dsdt.c.
>>> The main could be different, although it might be possible to gate ARM
>>> options, and the rest of the code would be shared.
>>
>> So I think we decided earlier to keep ARM and x86 ACPI builders
>> separate, at least for now. 
> I think so as well.
>
>> However, looking at the Makefile and mk_dsdt
>> I wonder whether it would make sense to put the builders in the same
>> directory (I am currently using tools/libacpi) so that those two files
>> can be kept common as much as possible, with the sources being
>> different. E.g. something like
>>
>> tools/libacpi:
>>     Makefile
>>     mk_dsdt.c
>>     acpi_x86.[ch]
>>     acpi_arm.[ch]
>>     *asl
>>     etc.
>>
>> The objects will be built in tools/libxl (there will be no libacpi.so)
>> but the infrastructure and sources will live together.
> I'm fine with this. But I think the patch moving the codes into
> tools/libacpi should be posted firstly, since this series depend on it.
> Boris, could you please send that patch? Then I can add the
> corresponding ARM patch on top of that.


I thought I had it almost ready yesterday but I encountered a problem
that I need to resolve before I post it. If I don't get it fixed in the
next couple of days I will send you a link to my repository so that you
can see what I have.

-boris


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