diff mbox

[v1] libxl: Make an ACPI support build for ARM64 configurable.

Message ID 1479903908-13579-1-git-send-email-andrii.anisov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrii Anisov Nov. 23, 2016, 12:25 p.m. UTC
Make the libxl ACPI support build configurable by the same config
option as the hypervisor side support.

Signed-off-by: Andrii Anisov <andrii.anisov@gmail.com>
---
 tools/libxl/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Julien Grall Nov. 23, 2016, 12:45 p.m. UTC | #1
Hello,

On 23/11/16 12:25, Andrii Anisov wrote:
> Make the libxl ACPI support build configurable by the same config
> option as the hypervisor side support.

This will not work because Kconfig support only exists for the hypervisor.

Furthermore, people may want to disable ACPI for the host but still 
support ACPI for guest.

The way forward is to have an option in the configure to opt-out the 
support of guest ACPI and also remove the iasl check.

Regards,

>
> Signed-off-by: Andrii Anisov <andrii.anisov@gmail.com>
> ---
>  tools/libxl/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index f5053a0..a4e9319 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -90,7 +90,7 @@ acpi:
>
>  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o libxl_x86_acpi.o
>  LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
> -ifeq ($(CONFIG_ARM_64),y)
> +ifeq ($(CONFIG_ACPI),y)
>  DSDT_FILES-y = dsdt_anycpu_arm.c
>  LIBXL_OBJS-y += libxl_arm_acpi.o $(patsubst %.c,%.o,$(DSDT_FILES-y))
>  dsdt_anycpu_arm.c:
>
Andrii Anisov Nov. 23, 2016, 1:03 p.m. UTC | #2
> This will not work because Kconfig support only exists for the hypervisor.

So I totally confused where CONFIG_ARM_64 is defined for tools.

Sincerely,
Andrii Anisov.
Wei Liu Nov. 23, 2016, 1:12 p.m. UTC | #3
On Wed, Nov 23, 2016 at 03:03:54PM +0200, Andrii Anisov wrote:
> > This will not work because Kconfig support only exists for the hypervisor.
> 
> So I totally confused where CONFIG_ARM_64 is defined for tools.
> 

It is defined in config/arm64.mk, which is included by various
makefiles.


> Sincerely,
> Andrii Anisov.
Andrii Anisov Nov. 23, 2016, 1:59 p.m. UTC | #4
> It is defined in config/arm64.mk, which is included by various
> makefiles.
As I got, it is included by Config.mk which consequently included by
different makefiles in hypervisor, tools, docs, stubdom.
It looks like a piece of legasy configuration code, I see moves of
CONFIG_ options from it to Kconfig.

I guess extending that file is not acceptable, so I hope to be back
soon with modifications suggested by Julien:
>The way forward is to have an option in the configure to opt-out the support of guest ACPI and also remove the iasl check.

Sincerely,
Andrii Anisov.
Wei Liu Nov. 23, 2016, 2:05 p.m. UTC | #5
On Wed, Nov 23, 2016 at 03:59:54PM +0200, Andrii Anisov wrote:
> > It is defined in config/arm64.mk, which is included by various
> > makefiles.
> As I got, it is included by Config.mk which consequently included by
> different makefiles in hypervisor, tools, docs, stubdom.
> It looks like a piece of legasy configuration code, I see moves of
> CONFIG_ options from it to Kconfig.
> 
> I guess extending that file is not acceptable, so I hope to be back

I think it would be fine to add other CONFIG_* to those files.

> soon with modifications suggested by Julien:
> >The way forward is to have an option in the configure to opt-out the
> >support of guest ACPI and also remove the iasl check.

But before you write any code, let me try to understand the real issue
first: you're trying to cross-build ARM tools on x86, but x86
iasl doesn't support ARM ACPI definition(s), right?

> 
> Sincerely,
> Andrii Anisov.
Andrii Anisov Nov. 23, 2016, 2:12 p.m. UTC | #6
> But before you write any code, let me try to understand the real issue
> first: you're trying to cross-build ARM tools on x86, but x86
> iasl doesn't support ARM ACPI definition(s), right?
Well, as I stated here [1], I'm pretty far from ACPI and understanding
of what's going wrong with the compilation. But I have a strong
feeling that this option should be optional.

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html

Sincerely,
Andrii Anisov.
Jan Beulich Nov. 23, 2016, 2:22 p.m. UTC | #7
>>> On 23.11.16 at 15:05, <wei.liu2@citrix.com> wrote:
> But before you write any code, let me try to understand the real issue
> first: you're trying to cross-build ARM tools on x86, but x86
> iasl doesn't support ARM ACPI definition(s), right?

No, the problem is that Shannon has broken cross builds of libacpi's
mk_dsdt.c (see my reply to Andrii's original problem report at
lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html).

Jan
Julien Grall Nov. 23, 2016, 3:10 p.m. UTC | #8
Hello Andrii,

On 23/11/16 14:12, Andrii Anisov wrote:
>> But before you write any code, let me try to understand the real issue
>> first: you're trying to cross-build ARM tools on x86, but x86
>> iasl doesn't support ARM ACPI definition(s), right?
> Well, as I stated here [1], I'm pretty far from ACPI and understanding
> of what's going wrong with the compilation. But I have a strong
> feeling that this option should be optional.

The ACPI support is pretty much contained in a single file and I am not 
sure you will win much space. Can you explain why the ACPI guest support 
should be optional?

Regards,

>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html
>
> Sincerely,
> Andrii Anisov.
>
Andrii Anisov Nov. 23, 2016, 3:47 p.m. UTC | #9
Hello Julien,

> The ACPI support is pretty much contained in a single file and I am not sure you will win much space.
> Can you explain why the ACPI guest support should be optional?
This would increase the system configurability and would let one
shrink a system to a minimal footprint with required functionality
only. Such possibility is very useful in embedded applications.

Unfortunately I don't know the exact space impact of this particular
feature 'cause I can't build it. From other hand I do not need it in
my system. So I would like to have a way to drop not needed
functionality easily.
BTW, excessive functionality reduction is also a way to get more
stable and predictable system.

Sincerely,
Andrii Anisov.
Julien Grall Nov. 23, 2016, 7:28 p.m. UTC | #10
On 23/11/16 15:47, Andrii Anisov wrote:
> Hello Julien,

Hi Andrii,

>> The ACPI support is pretty much contained in a single file and I am not sure you will win much space.
>> Can you explain why the ACPI guest support should be optional?
> This would increase the system configurability and would let one
> shrink a system to a minimal footprint with required functionality
> only. Such possibility is very useful in embedded applications.
>
> Unfortunately I don't know the exact space impact of this particular
> feature 'cause I can't build it. From other hand I do not need it in
> my system. So I would like to have a way to drop not needed
> functionality easily.

I agree with this argument however...

> BTW, excessive functionality reduction is also a way to get more
> stable and predictable system.

this statement is not entirely true. There is no automatic test on all 
the possible configurations, although we have travis to test build (and 
not booting) a random Kconfig. Even if we try our best to see any 
problem when reviewing code, we cannot guarantee an error when using a 
different configuration.

And this is becoming a problem as today we have no way to know the 
configuration used when a problem is reported. This is because the 
Kconfig is not embedded in the Xen binary.

I got bitten quite a few times in my development because I had a binary 
but not the Kconfig. It was re-written because I forgot to add 
XEN_CONFIG_EXPERT on the command line.

I might be more inclined to make more feature optional when we will have 
a way to track the configuration of a hypervisor binary.

Regards,
Andrew Cooper Nov. 23, 2016, 7:32 p.m. UTC | #11
On 23/11/16 19:28, Julien Grall wrote:
>
>
> On 23/11/16 15:47, Andrii Anisov wrote:
>> Hello Julien,
>
> Hi Andrii,
>
>>> The ACPI support is pretty much contained in a single file and I am
>>> not sure you will win much space.
>>> Can you explain why the ACPI guest support should be optional?
>> This would increase the system configurability and would let one
>> shrink a system to a minimal footprint with required functionality
>> only. Such possibility is very useful in embedded applications.
>>
>> Unfortunately I don't know the exact space impact of this particular
>> feature 'cause I can't build it. From other hand I do not need it in
>> my system. So I would like to have a way to drop not needed
>> functionality easily.
>
> I agree with this argument however...
>
>> BTW, excessive functionality reduction is also a way to get more
>> stable and predictable system.
>
> this statement is not entirely true. There is no automatic test on all
> the possible configurations, although we have travis to test build
> (and not booting) a random Kconfig. Even if we try our best to see any
> problem when reviewing code, we cannot guarantee an error when using a
> different configuration.
>
> And this is becoming a problem as today we have no way to know the
> configuration used when a problem is reported. This is because the
> Kconfig is not embedded in the Xen binary.
>
> I got bitten quite a few times in my development because I had a
> binary but not the Kconfig. It was re-written because I forgot to add
> XEN_CONFIG_EXPERT on the command line.
>
> I might be more inclined to make more feature optional when we will
> have a way to track the configuration of a hypervisor binary.

+10 to this idea.

It isn't hard to compress .config and stash it in .rodata during
compilation, offering it back to the toolstack via a hypercall.  We
already do similar things for the XSM policy.

~Andrew
Andrii Anisov Nov. 28, 2016, 9:32 a.m. UTC | #12
Julien,

> There is no automatic test on all the possible configurations, although we have
> travis to test build (and not booting) a random Kconfig.

I guess there is no need to test booting of a "random" Kconfig within
the mainline anyway. The full-featured defconfig would be enough to be
provided by the mainline.

> Even if we try our best to see any problem when reviewing code, we cannot guarantee an error
> when using a different configuration.

From other hand, those who build their solution based on XEN, will
take care about their specific configurations. And get back to the
community with features and fixes if applicable.

> I got bitten quite a few times in my development because I had a binary but
> not the Kconfig.

I think everyone faced similar issues in their experience.

> I might be more inclined to make more feature optional when we will
> have a way to track the configuration of a hypervisor binary.
> +10 to this idea.

Having a .config built into the XEN binary is a cool feature.
But IMO it is useful rather during development time. Knowing a
production system configuration seems to be closer to an integration
area of responsibility.

> This is because the Kconfig is not embedded in the Xen binary.

This thread was started from the configuration of the tools, which are
not configured by Kconfig.
Is it already possible to get back tools configuration during runtime?

Sincerely,
Andrii Anisov.
diff mbox

Patch

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index f5053a0..a4e9319 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -90,7 +90,7 @@  acpi:
 
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o libxl_x86_acpi.o
 LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
-ifeq ($(CONFIG_ARM_64),y)
+ifeq ($(CONFIG_ACPI),y)
 DSDT_FILES-y = dsdt_anycpu_arm.c
 LIBXL_OBJS-y += libxl_arm_acpi.o $(patsubst %.c,%.o,$(DSDT_FILES-y))
 dsdt_anycpu_arm.c: