Many thanks for review.
On Thu, Feb 09, 2017 at 02:24:39AM +0000, Zheng, Lv wrote:
Thanks for the reminder.
Here goes my opinions.
> From: Devel [mailto:email@example.com] On Behalf Of Al Stone
> Sent: Friday, January 27, 2017 1:05 AM
> Subject: Re: [Devel] [PATCH v2] EFI: fix build for AARCH64
> On 01/03/2017 05:21 AM, Leif Lindholm wrote:
> > AARCH64 build was left out of initial version, so add to .dsc and set
> > ACPI_MACHINE_WIDTH correctly.
> > Also, acpidump.inf specified explicit per-architecture (but identical)
> > CFLAGS. Rather than duplicating this further, use the same setting for
> > all architectures until there is actually a need to diverge.
> > Also update README to make it obvious AArch64 is supported.
> > Signed-off-by: Leif Lindholm <leif.lindholm(a)linaro.org>
> > ---
> > v2 contains a fix for a non-functional issue, which could generate
> > warnings when building with CLANG.
> > generate/efi/AcpiPkg.dsc | 2 +-
> > generate/efi/README | 1 +
> > generate/efi/acpidump/acpidump.inf | 6 ++----
> > source/include/platform/acefi.h | 2 +-
> > 4 files changed, 5 insertions(+), 6 deletions(-)
> > diff --git a/generate/efi/AcpiPkg.dsc b/generate/efi/AcpiPkg.dsc
> > index 323d39c..9302555 100644
> > --- a/generate/efi/AcpiPkg.dsc
> > +++ b/generate/efi/AcpiPkg.dsc
> > @@ -18,7 +18,7 @@
> > PLATFORM_VERSION = 1.0
> > DSC_SPECIFICATION = 0x00010005
> > OUTPUT_DIRECTORY = Build/Acpi
> > - SUPPORTED_ARCHITECTURES = IA32|X64
> > + SUPPORTED_ARCHITECTURES = AARCH64|IA32|X64
> > BUILD_TARGETS = DEBUG|RELEASE
> > SKUID_IDENTIFIER = DEFAULT
> > diff --git a/generate/efi/README b/generate/efi/README
> > index 7550e9d..b3ec3ff 100644
> > --- a/generate/efi/README
> > +++ b/generate/efi/README
> > @@ -20,6 +20,7 @@ But the porting has only been tested in a Linux environment.
> > You can find built EFI binaries (e.x., acpidump.efi) in the following
> > folders:
> > + Build/Acpi/DEBUG_GCC47/AARCH64: aarch64 targets
> > Build/Acpi/DEBUG_GCC47/IA32: i386 targets
> > Build/Acpi/DEBUG_GCC47/X64: x86_64 targets
Could you reorder this list in the README file?
New support should be listed below the old supports.
And I believe EFI is much more widely used on IA32 and X64 platforms.
That's probably true, I was going for alphabetical order :)
If we are sorting by order of most widely used, I would think x86_64
should go first. So please clarify which order you would prefer.
> > diff --git a/generate/efi/acpidump/acpidump.inf
> > index 8d2c979..e5f21c0 100644
> > --- a/generate/efi/acpidump/acpidump.inf
> > +++ b/generate/efi/acpidump/acpidump.inf
> > @@ -55,7 +55,5 @@
> > BaseLib
> > [BuildOptions]
> > - MSFT:*_*_IA32_CC_FLAGS = /Oi- /WX- /D_EDK2_EFI /DACPI_DUMP_APP
> > - MSFT:*_*_X64_CC_FLAGS = /Oi- /WX- /D_EDK2_EFI /DACPI_DUMP_APP
> > - GCC:*_*_IA32_CC_FLAGS = -U__linux__ -U_LINUX -D_EDK2_EFI -DACPI_DUMP_APP
> iwithprefix include
> > - GCC:*_*_X64_CC_FLAGS = -U__linux__ -U_LINUX -D_EDK2_EFI -DACPI_DUMP_APP
> iwithprefix include
> > + MSFT:*_*_*_CC_FLAGS = /Oi- /WX- /D_EDK2_EFI /DACPI_DUMP_APP
> > + GCC:*_*_*_CC_FLAGS = -U__linux__ -U_LINUX -D_EDK2_EFI -DACPI_DUMP_APP
Maybe this should be in a separate patch.
I could, but this is actually part of the main build fix. It's been a
while since I wrote it, but I think the -U_LINUX was the change that
made it possible to build at all, whereas the changes below made the
application function properly.
> > diff --git a/source/include/platform/acefi.h
> > index 8328ab6..75ef89a 100644
> > --- a/source/include/platform/acefi.h
> > +++ b/source/include/platform/acefi.h
> > @@ -140,7 +140,7 @@
> > #define VOID void
> > -#if defined(__ia64__) || defined(__x86_64__)
> > +#if defined(__aarch64__) || defined(__ia64__) || defined(__x86_64__)
> > #define ACPI_MACHINE_WIDTH 64
TBH, I don't have any idea on how aarch64 EFI wrapper should be.
So what we can do is just relying on the community.
So I'm OK with this change.
But it seems this commit should also define __aarch64__ in accygwin.h.
Is this a mistake?
That is possibly true, but I don't have a Windows environment
available to test in.
Evan, Alexei? Could either of you test this?
Thanks and best regards
> It's been over a month since this really very simple patch was submitted. In
> the meantime, acpidump.efi does not build without it. Any particular reason
> this has not been merged? Leif isn't the only user that needs it.
> Al Stone
> Software Engineer
> Red Hat, Inc.
> Devel mailing list