Re: [Xen-devel] [PATCH RESEND 00/14] Xen ARM DomU ACPI support

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/16/2016 07:20 AM, Wei Liu wrote:
> On Tue, Jun 07, 2016 at 12:00:26PM +0100, Julien Grall wrote:
>> Hello Shannon,
>>
>> On 07/06/16 02:07, Shannon Zhao wrote:
>>> On 2016/6/6 23:48, Julien Grall wrote:
>>>> On 06/06/16 13:26, Wei Liu wrote:
>>>>> On Tue, May 31, 2016 at 01:02:52PM +0800, Shannon Zhao wrote:
>>>>>> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>>>>>>
>>>>>> The design of this feature is described as below.
>>>>>> Firstly, the toolstack (libxl) generates the ACPI tables according the
>>>>>> number of vcpus and gic controller.
>>>>>>
>>>>>> Then, it copies these ACPI tables to DomU memory space and passes
>>>>>> them to UEFI firmware through the "ARM multiboot" protocol.
>>>>>>
>>>>>> At last, UEFI gets the ACPI tables through the "ARM multiboot" protocol
>>>>>> and installs these tables like the usual way and passes both ACPI and DT
>>>>>> information to the Xen DomU.
>>>>>>
>>>>>> Currently libxl only generates RSDP, XSDT, GTDT, MADT, FADT, DSDT tables
>>>>>> since it's enough now.
>>>>>>
>>>>>> This has been tested using guest kernel with the Dom0 ACPI support
>>>>>> patches which could be fetched from:
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=efi/arm-xen
>>>>>>
>>>>>>
>>>>>> Shannon Zhao (14):
>>>>>>    libxl/arm: Fix the function name in error log
>>>>>>    libxl/arm: Factor out codes for generating DTB
>>>>>>    libxc: Add placeholders for ACPI tables blob and size
>>>>>>    tools: add ACPI tables relevant definitions
>>>>>>    libxl/arm: Construct ACPI GTDT table
>>>>>>    libxl/arm: Construct ACPI FADT table
>>>>>>    libxl/arm: Construct ACPI DSDT table
>>>>>>    libxl/arm: Construct ACPI MADT table
>>>>>>    libxl/arm: Construct ACPI XSDT table
>>>>>>    libxl/arm: Construct ACPI RSDP table
>>>>>>    libxl/arm: Initialize domain param HVM_PARAM_CALLBACK_IRQ
>>>>>>    libxl/arm: Add ACPI module
>>>>>>    libxl/arm: initialize memory information of ACPI blob
>>>>>>    libxc/xc_dom_core: Copy ACPI tables to guest memory space
>>>>>>
>>>>> After going through this series, I think the code mostly looks good.
>>>>>
>>>>> There is one higher level question: you seem to have put a lot of the
>>>>> table construction code in libxl, while I failed to see why specific
>>>>> libxl information is needed. Have you considered moving the code to
>>>>> libxc?
>>>> I would rather avoid to have the device tree built by libxl and ACPI
>>>> tables built by libxc.
>>>>
>>>> I don't remember why we have decided to implement DT building in libxl,
>>>> I guess it is because we need to know which GIC version is used (GICv2
>>>> vs GICv3).
>>>>
>>>> In the long run, we might also need to have more part of the firmware
>>>> configurable (performance monitor support, memory layout, UART...).
>>>>
>>>> Most of those information are available easily in libxl, we would to
>>>> export them of libxc.
>>> Yes, and one more reason is that to support ACPI, it also needs to add
>>> the ACPI multiboot module in DT.
>> I don't consider this as a reason for building the ACPI tables in libxl. I
>> am more concerned about building the firmwares in different place.
>>
>> If we decide to build the ACPI tables in libxc, then the code to build the
>> device tree should move in libxc too.
>>
> I've read this sub-thread and the other thread in which Boris replied, I
> think due to the fact that libxl has more information at hand and libxc
> is intended to be simple, I think having the building code in libxl
> makes sense.
>
> Shannon, Boris and Julien, what do you guys think? Can we agree on
> something so that Shannon can move on with next iteration?

I kind of agree that libxl is perhaps a better place but it would be
good to have something more than "libxc is intended to be simple" that
tells us what goes where. In other words -- what libxl is for and what
libxc is for.

For example libxl_x86.c and xc_dom86.c. The latter has a comment at the
top that says this is for arch-specific code. But there is plenty of
arch-specific stuff (e820) in libxl file.

-boris


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