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

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/6/30 2:58, Boris Ostrovsky wrote:
> 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
Hi Boris,

Thanks a lot!
While I found there is a compiling problem of the last patch.

undefined reference to `libxl__dom_load_acpi'
collect2: error: ld returned 1 exit status

I think you should define the corresponding function for ARM.

Julien, Stefano,

Looks like Boris is going to use libxl__dom_load_acpi to load ACPI
tables, do we need to use it for ARM as well or load the tables in
xc_dom_build_image()?

Thanks,
-- 
Shannon


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