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

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

[Andrii Anisov]

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(-)

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:
-- 
2.7.4


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

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

[Julien Grall]

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:
>

-- 
Julien Grall

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

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

[Andrii Anisov]

> 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.

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

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

[Wei Liu]

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.

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

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

[Andrii Anisov]

> 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.

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

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

[Wei Liu]

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.

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

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

[Andrii Anisov]

> 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.

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

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

[Jan Beulich]

>>> 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


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

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

[Wei Liu]

On Wed, Nov 23, 2016 at 04:12:28PM +0200, 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.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html

What is the exact rune you got when compiling mk_dsdt ?

If everything it set up properly you should be using a cross-build gcc
which should have the correct architecture?

Can you try this (untested) patch?


diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
index ccc32c9..2b9d5b8 100644
--- a/tools/libacpi/Makefile
+++ b/tools/libacpi/Makefile
@@ -44,7 +44,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl
 	rm -f $(addprefix $(ACPI_BUILD_DIR)/, $*.aml $*.hex)
  
 $(MK_DSDT): mk_dsdt.c
-	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
+	$(CC) $(CFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
 
 $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl $(MK_DSDT)
 	# Remove last bracket

> 
> Sincerely,
> Andrii Anisov.

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

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

[Julien Grall]

Hi Wei,

On 23/11/16 14:29, Wei Liu wrote:
> On Wed, Nov 23, 2016 at 04:12:28PM +0200, 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.
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html
>
> What is the exact rune you got when compiling mk_dsdt ?
>
> If everything it set up properly you should be using a cross-build gcc
> which should have the correct architecture?
>
> Can you try this (untested) patch?
>
>
> diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
> index ccc32c9..2b9d5b8 100644
> --- a/tools/libacpi/Makefile
> +++ b/tools/libacpi/Makefile
> @@ -44,7 +44,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl
>  	rm -f $(addprefix $(ACPI_BUILD_DIR)/, $*.aml $*.hex)
>
>  $(MK_DSDT): mk_dsdt.c
> -	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
> +	$(CC) $(CFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c

That will not work, CC will build an ARM binary. However mk_dsdt is used 
to generate the static ACPI table integrated in the libxl, so mk_dsdt 
needs to be built with HOSTCC.

I think the code should use CONFIG_ARM_64/CONFIG_ARM_32 rather than 
__*__ one.

Regards,

-- 
Julien Grall

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

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

[Wei Liu]

On Wed, Nov 23, 2016 at 02:34:04PM +0000, Julien Grall wrote:
> Hi Wei,
> 
> On 23/11/16 14:29, Wei Liu wrote:
> >On Wed, Nov 23, 2016 at 04:12:28PM +0200, 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.
> >>
> >>[1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html
> >
> >What is the exact rune you got when compiling mk_dsdt ?
> >
> >If everything it set up properly you should be using a cross-build gcc
> >which should have the correct architecture?
> >
> >Can you try this (untested) patch?
> >
> >
> >diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
> >index ccc32c9..2b9d5b8 100644
> >--- a/tools/libacpi/Makefile
> >+++ b/tools/libacpi/Makefile
> >@@ -44,7 +44,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl
> > 	rm -f $(addprefix $(ACPI_BUILD_DIR)/, $*.aml $*.hex)
> >
> > $(MK_DSDT): mk_dsdt.c
> >-	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
> >+	$(CC) $(CFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
> 
> That will not work, CC will build an ARM binary. However mk_dsdt is used to
> generate the static ACPI table integrated in the libxl, so mk_dsdt needs to
> be built with HOSTCC.

Yes, you're right.

> 
> I think the code should use CONFIG_ARM_64/CONFIG_ARM_32 rather than __*__
> one.
> 

That might work -- provided they are defined even in a cross-build.

> Regards,
> 
> -- 
> Julien Grall

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

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

[Julien Grall]

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.
>

-- 
Julien Grall

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

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

[Andrii Anisov]

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.

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Andrew Cooper]

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

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

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

[Andrii Anisov]

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.

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