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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/28/2016 09:41 AM, Boris Ostrovsky wrote:
> 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.


This may take me longer than I thought so here is the repo:

git://oss.oracle.com/git/bostrovs/xen.git:acpi_v1_wip


-boris



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