[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZfLCoZXQ4kp2TeB+@gmail.com>
Date: Thu, 14 Mar 2024 10:25:53 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Wei Yang <richard.weiyang@...il.com>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] x86/boot: replace __PHYSICAL_START with
LOAD_PHYSICAL_ADDR
* Wei Yang <richard.weiyang@...il.com> wrote:
> On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote:
> >
> >* Wei Yang <richard.weiyang@...il.com> wrote:
> >
> >> Both __PHYSICAL_START and LOAD_PHYSICAL_ADDR are defined to get aligned
> >> CONFIG_PHYSICAL_START, so we can replace __PHYSICAL_START with
> >> LOAD_PHYSICAL_ADDR. And then remove the definition of __PHYSICAL_START,
> >> which is only used to define __START_KERNEL.
> >>
> >> Since <asm/boot.h> includes <asm/pgtable_types.h>, which includes
> >> <asm/page_types.h>, it is fine to move definition from <asm/boot.h> to
> >> <asm/page_types.h>.
> >>
> >> Signed-off-by: Wei Yang <richard.weiyang@...il.com>
> >> ---
> >> arch/x86/include/asm/boot.h | 5 -----
> >> arch/x86/include/asm/page_types.h | 8 +++++---
> >> 2 files changed, 5 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/boot.h b/arch/x86/include/asm/boot.h
> >> index a38cc0afc90a..12cbc57d0128 100644
> >> --- a/arch/x86/include/asm/boot.h
> >> +++ b/arch/x86/include/asm/boot.h
> >> @@ -6,11 +6,6 @@
> >> #include <asm/pgtable_types.h>
> >> #include <uapi/asm/boot.h>
> >>
> >> -/* Physical address where kernel should be loaded. */
> >> -#define LOAD_PHYSICAL_ADDR ((CONFIG_PHYSICAL_START \
> >> - + (CONFIG_PHYSICAL_ALIGN - 1)) \
> >> - & ~(CONFIG_PHYSICAL_ALIGN - 1))
> >> -
> >> /* Minimum kernel alignment, as a power of two */
> >> #ifdef CONFIG_X86_64
> >> # define MIN_KERNEL_ALIGN_LG2 PMD_SHIFT
> >> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> >> index 86bd4311daf8..acc1620fd121 100644
> >> --- a/arch/x86/include/asm/page_types.h
> >> +++ b/arch/x86/include/asm/page_types.h
> >> @@ -31,10 +31,12 @@
> >>
> >> #define VM_DATA_DEFAULT_FLAGS VM_DATA_FLAGS_TSK_EXEC
> >>
> >> -#define __PHYSICAL_START ALIGN(CONFIG_PHYSICAL_START, \
> >> - CONFIG_PHYSICAL_ALIGN)
> >> +/* Physical address where kernel should be loaded. */
> >> +#define LOAD_PHYSICAL_ADDR ((CONFIG_PHYSICAL_START \
> >> + + (CONFIG_PHYSICAL_ALIGN - 1)) \
> >> + & ~(CONFIG_PHYSICAL_ALIGN - 1))
> >
> >I agree with this simplification, but the ALIGN() expression is far easier
> >to read, so please keep that one instead of the open-coded version.
> >
>
> I just tried to define LOAD_PHYSICAL_ADDR by ALIGN, but face a compile error
> on compressed/head_[32|64].o.
>
> $ make arch/x86/boot/compressed/head_64.o
> CALL scripts/checksyscalls.sh
> DESCEND objtool
> INSTALL libsubcmd_headers
> AS arch/x86/boot/compressed/head_64.o
> arch/x86/boot/compressed/head_64.S: Assembler messages:
> arch/x86/boot/compressed/head_64.S:154: Error: junk (0x1000000,0x200000)' after expression
> arch/x86/boot/compressed/head_64.S:154: Error: number of operands mismatch for 16' after expression
> arch/x86/boot/compressed/head_64.S:157: Error: junk mov'
> arch/x86/boot/compressed/head_64.S:330: Error: junk (0x1000000,0x200000)' after expression
> arch/x86/boot/compressed/head_64.S:330: Error: number of operands mismatch for 16' after expression
> arch/x86/boot/compressed/head_64.S:333: Error: junk movq'
>
> If my understanding is correct, the reason is linkage.h defines ALIGN, which
> is ".balign xxx". Maybe this is why original LOAD_PHYSICAL_ADDR doesn't use
> ALIGN.
linkage.h defines __ALIGN, which is different from ALIGN().
Also, a number of .S files seem to be using some sort of ALIGN() method
within arch/x86/, according to:
git grep 'ALIGN(' -- 'arch/x86/*.S'
> So is this ok to keep the open-coded definition?
It would be nice to figure out why ALIGN() appears to be working in other
cases in .S files, while not in this case.
Thanks,
Ingo
Powered by blists - more mailing lists