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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jun 16, 2016 at 09:25:20AM -0400, Boris Ostrovsky wrote:
> 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.
> 

I myself have long been following the convention that any complex
decision should be made inside libxl because it just has more
information than libxc and a plethora of machinery at its disposal.
Libxc should merely be a wrapper for the underlying hypervisor
interface. There are, of course, exceptions that the libxc functions
look a bit more complex because we want to provide some compatibility
where we can, but keep in mind that libxc is meant to be unstable so
that semantics is subject to change at any time.

Overtime the evolution of libraries will certainly leave some areas
unclear.  The E820 code is an example that libxc is not able to make
decision on its own and we move that to libxl. I wouldn't want libxc to
know how to construct memory map for RDM + vNUMA + whatever comes next.

Wei.

> -boris
> 

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