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@gmail.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))
-#define __START_KERNEL (__START_KERNEL_map + __PHYSICAL_START)
+#define __START_KERNEL (__START_KERNEL_map + LOAD_PHYSICAL_ADDR)
#ifdef CONFIG_X86_64
#include <asm/page_64_types.h>
--
2.34.1
* Wei Yang <richard.weiyang@gmail.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@gmail.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. Thanks, Ingo
On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote: > >* Wei Yang <richard.weiyang@gmail.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@gmail.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. So is this ok to keep the open-coded definition? >Thanks, > > Ingo -- Wei Yang Help you, Help me
* Wei Yang <richard.weiyang@gmail.com> wrote:
> On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote:
> >
> >* Wei Yang <richard.weiyang@gmail.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@gmail.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
On Thu, Mar 14, 2024 at 10:25:53AM +0100, Ingo Molnar wrote:
>
>* Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote:
>> >
>> >* Wei Yang <richard.weiyang@gmail.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@gmail.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().
>
linkage.h defines ALIGN to __ALIGN which is different from what we expect.
>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'
I tried below command by append '\<' to ALIGN
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.
>
Here is grep result
arch/x86/boot/compressed/vmlinux.lds.S: .data : ALIGN(0x1000) {
arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(. + 4, 0x200);
arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(L1_CACHE_BYTES);
arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(8); /* For convenience during zeroing */
arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); /* keep ZO size page aligned */
arch/x86/kernel/vmlinux.lds.S:#define X86_ALIGN_RODATA_BEGIN . = ALIGN(HPAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE); \
arch/x86/kernel/vmlinux.lds.S:#define ALIGN_ENTRY_TEXT_BEGIN . = ALIGN(PMD_SIZE);
arch/x86/kernel/vmlinux.lds.S:#define ALIGN_ENTRY_TEXT_END . = ALIGN(PMD_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PMD_SIZE); \
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); \
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PMD_SIZE); \
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); \
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(__vvar_page + PAGE_SIZE, PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); /* keep VO_INIT_SIZE page aligned */
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE);
arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(4);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(16);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(PAGE_SIZE);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(128);
arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(4);
arch/x86/um/vdso/vdso-layout.lds.S: . = ALIGN(0x100);
It looks all happens in link script, not assembly source code.
And other grep result with "ALIGN" are:
SYM_CODE_START_NOALIGN
SYM_CODE_START_LOCAL_NOALIGN
SYM_FUNC_START_NOALIGN
SYM_FUNC_START_LOCAL_NOALIGN
SYM_INNER_LABEL_ALIGN
which are defined in linkage.h.
>Thanks,
>
> Ingo
--
Wei Yang
Help you, Help me
On Fri, Mar 15, 2024 at 12:57:37AM +0000, Wei Yang wrote:
>On Thu, Mar 14, 2024 at 10:25:53AM +0100, Ingo Molnar wrote:
>>
>>* Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>>> On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote:
>>> >
>>> >* Wei Yang <richard.weiyang@gmail.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@gmail.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().
>>
>
>linkage.h defines ALIGN to __ALIGN which is different from what we expect.
>
>>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'
>
>I tried below command by append '\<' to ALIGN
>
> 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.
>>
>
>Here is grep result
>
>arch/x86/boot/compressed/vmlinux.lds.S: .data : ALIGN(0x1000) {
>arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(. + 4, 0x200);
>arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(L1_CACHE_BYTES);
>arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(8); /* For convenience during zeroing */
>arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/boot/compressed/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); /* keep ZO size page aligned */
>arch/x86/kernel/vmlinux.lds.S:#define X86_ALIGN_RODATA_BEGIN . = ALIGN(HPAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE); \
>arch/x86/kernel/vmlinux.lds.S:#define ALIGN_ENTRY_TEXT_BEGIN . = ALIGN(PMD_SIZE);
>arch/x86/kernel/vmlinux.lds.S:#define ALIGN_ENTRY_TEXT_END . = ALIGN(PMD_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PMD_SIZE); \
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); \
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PMD_SIZE); \
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); \
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(__vvar_page + PAGE_SIZE, PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(8);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(PAGE_SIZE); /* keep VO_INIT_SIZE page aligned */
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE);
>arch/x86/kernel/vmlinux.lds.S: . = ALIGN(HPAGE_SIZE);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(4);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(16);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(PAGE_SIZE);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(128);
>arch/x86/realmode/rm/realmode.lds.S: . = ALIGN(4);
>arch/x86/um/vdso/vdso-layout.lds.S: . = ALIGN(0x100);
>
>It looks all happens in link script, not assembly source code.
>
>And other grep result with "ALIGN" are:
>
> SYM_CODE_START_NOALIGN
> SYM_CODE_START_LOCAL_NOALIGN
> SYM_FUNC_START_NOALIGN
> SYM_FUNC_START_LOCAL_NOALIGN
> SYM_INNER_LABEL_ALIGN
>
>which are defined in linkage.h.
>
Hi, Ingo
Take another look into the usage.
In linkage.h, we have following definition:
#ifdef __ASSEMBLY__
#ifndef LINKER_SCRIPT
#define ALIGN __ALIGN
#endif
#endif
We would include linkage.h from .S and .lds.S. We both define __ASSEMBLY__ in
command line when building these target, but we would define LINKER_SCRIPT
when building .lds from .lds.S. This introduces the different behavior of
ALIGN.
* For .S, ALING is replaced by __ALIGN and then to .balign xxx
* For .lds, ALIGN is is not expanded, which is a lds keyword
Also linkage.h may be included in .c, e.g. head64.c. But we don't define
__ASSEMBLY__ in command line, which leads the ALIGN undefined until kernel.h
is included.
>>Thanks,
>>
>> Ingo
>
>--
>Wei Yang
>Help you, Help me
--
Wei Yang
Help you, Help me
On Wed, Mar 13, 2024 at 11:29:33AM +0100, Ingo Molnar wrote: > >* Wei Yang <richard.weiyang@gmail.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@gmail.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. Sure, will send v2. > >Thanks, > > Ingo -- Wei Yang Help you, Help me
On 13.03.24 г. 9:58 ч., Wei Yang 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@gmail.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)) Why don't you simply define LOAD_PHYSICAL_ADDR via ALIGN(CONFIG_PHYSICAL_START, CONFIG_PHYSICAL_ALING) it's way more readable. > > -#define __START_KERNEL (__START_KERNEL_map + __PHYSICAL_START) > +#define __START_KERNEL (__START_KERNEL_map + LOAD_PHYSICAL_ADDR) > > #ifdef CONFIG_X86_64 > #include <asm/page_64_types.h>
On Wed, Mar 13, 2024 at 12:29:37PM +0200, Nikolay Borisov wrote: > > >On 13.03.24 г. 9:58 ч., Wei Yang 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@gmail.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)) > >Why don't you simply define LOAD_PHYSICAL_ADDR via >ALIGN(CONFIG_PHYSICAL_START, CONFIG_PHYSICAL_ALING) it's way more readable. > Sure, will do it. >> -#define __START_KERNEL (__START_KERNEL_map + __PHYSICAL_START) >> +#define __START_KERNEL (__START_KERNEL_map + LOAD_PHYSICAL_ADDR) >> #ifdef CONFIG_X86_64 >> #include <asm/page_64_types.h> -- Wei Yang Help you, Help me
The following commit has been merged into the x86/build branch of tip:
Commit-ID: 29b0aab841da3cade64c7e41e99e9f4645e65fb1
Gitweb: https://git.kernel.org/tip/29b0aab841da3cade64c7e41e99e9f4645e65fb1
Author: Wei Yang <richard.weiyang@gmail.com>
AuthorDate: Wed, 13 Mar 2024 07:58:37
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 22 Mar 2024 11:36:45 +01:00
x86/boot: Replace __PHYSICAL_START with LOAD_PHYSICAL_ADDR
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@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20240313075839.8321-3-richard.weiyang@gmail.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 a38cc0a..12cbc57 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 86bd431..acc1620 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))
-#define __START_KERNEL (__START_KERNEL_map + __PHYSICAL_START)
+#define __START_KERNEL (__START_KERNEL_map + LOAD_PHYSICAL_ADDR)
#ifdef CONFIG_X86_64
#include <asm/page_64_types.h>
© 2016 - 2026 Red Hat, Inc.