lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ