Boris reported two build issues immediately after the FRED native patches
landed in the tip x86/fred branch, fix them.
Xin Li (2):
x86/fred: Fix build with clang
x86/fred: Fix build with CONFIG_IA32_EMULATION=n
arch/x86/entry/entry_64_fred.S | 3 +--
arch/x86/entry/entry_fred.c | 2 ++
2 files changed, 3 insertions(+), 2 deletions(-)
base-commit: a9f26154bf5478fc155309fc69128415f3a1be08
prerequisite-patch-id: 4dc72e7440d5412bf200dbccb9a41128ed30baec
--
2.43.0
As clang doesn't allow .fill to refernece a symbol before it's defined,
use asm_fred_entrypoint_user instead of asm_fred_entrypoint_kernel.
Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
Reported-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/lkml/20240126100050.GAZbOC0g3Rlr6otZcT@fat_crate.local/
Signed-off-by: Xin Li <[email protected]>
---
arch/x86/entry/entry_64_fred.S | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index eedf98de7538..5427e0da190d 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -43,13 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user, SYM_L_GLOBAL)
_ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
SYM_CODE_END(asm_fred_entrypoint_user)
-.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
-
/*
* The new RIP value that FRED event delivery establishes is
* (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
* ring 0, i.e., asm_fred_entrypoint_user + 256.
*/
+ .fill asm_fred_entrypoint_user + 256 - ., 1, 0xcc
.org asm_fred_entrypoint_user + 256
SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
FRED_ENTER
--
2.43.0
When CONFIG_IA32_EMULATION=n, int80_emulation() is NOT defined, fix it.
Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
Link: https://lore.kernel.org/lkml/20240126100519.GBZbOD3xFB0v3mp5B1@fat_crate.local/
Reported-by: Borislav Petkov (AMD) <[email protected]>
Signed-off-by: Xin Li <[email protected]>
---
arch/x86/entry/entry_fred.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
index 06d00c60ea64..ac120cbdaaf2 100644
--- a/arch/x86/entry/entry_fred.c
+++ b/arch/x86/entry/entry_fred.c
@@ -62,11 +62,13 @@ static noinstr void fred_intx(struct pt_regs *regs)
case X86_TRAP_OF:
return exc_overflow(regs);
+#ifdef CONFIG_IA32_EMULATION
/* INT80 */
case IA32_SYSCALL_VECTOR:
if (ia32_enabled())
return int80_emulation(regs);
fallthrough;
+#endif
default:
return exc_general_protection(regs, 0);
--
2.43.0
On January 27, 2024 1:37:27 AM PST, Xin Li <[email protected]> wrote:
>As clang doesn't allow .fill to refernece a symbol before it's defined,
>use asm_fred_entrypoint_user instead of asm_fred_entrypoint_kernel.
>
>Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
>Reported-by: Borislav Petkov (AMD) <[email protected]>
>Link: https://lore.kernel.org/lkml/20240126100050.GAZbOC0g3Rlr6otZcT@fat_crate.local/
>Signed-off-by: Xin Li <[email protected]>
>---
> arch/x86/entry/entry_64_fred.S | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
>index eedf98de7538..5427e0da190d 100644
>--- a/arch/x86/entry/entry_64_fred.S
>+++ b/arch/x86/entry/entry_64_fred.S
>@@ -43,13 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user, SYM_L_GLOBAL)
> _ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
> SYM_CODE_END(asm_fred_entrypoint_user)
>
>-.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
>-
> /*
> * The new RIP value that FRED event delivery establishes is
> * (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
> * ring 0, i.e., asm_fred_entrypoint_user + 256.
> */
>+ .fill asm_fred_entrypoint_user + 256 - ., 1, 0xcc
> .org asm_fred_entrypoint_user + 256
> SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
> FRED_ENTER
.fill and .org here are redundant; in fact, there two directives mean exactly the same thing except that .org implicitly subtracts the current offset.
> >diff --git a/arch/x86/entry/entry_64_fred.S
> >b/arch/x86/entry/entry_64_fred.S index eedf98de7538..5427e0da190d
> >100644
> >--- a/arch/x86/entry/entry_64_fred.S
> >+++ b/arch/x86/entry/entry_64_fred.S
> >@@ -43,13 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user,
> SYM_L_GLOBAL)
> > _ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
> > SYM_CODE_END(asm_fred_entrypoint_user)
> >
> >-.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
> >-
> > /*
> > * The new RIP value that FRED event delivery establishes is
> > * (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
> > * ring 0, i.e., asm_fred_entrypoint_user + 256.
> > */
> >+ .fill asm_fred_entrypoint_user + 256 - ., 1, 0xcc
> > .org asm_fred_entrypoint_user + 256
> > SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
> > FRED_ENTER
>
> .fill and .org here are redundant; in fact, there two directives mean exactly the
> same thing except that .org implicitly subtracts the current offset.
Ah, right, .fill already does the job!
I will remove .org.
On 1/27/24 11:46, Li, Xin3 wrote:
>>> diff --git a/arch/x86/entry/entry_64_fred.S
>>> b/arch/x86/entry/entry_64_fred.S index eedf98de7538..5427e0da190d
>>> 100644
>>> --- a/arch/x86/entry/entry_64_fred.S
>>> +++ b/arch/x86/entry/entry_64_fred.S
>>> @@ -43,13 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user,
>> SYM_L_GLOBAL)
>>> _ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
>>> SYM_CODE_END(asm_fred_entrypoint_user)
>>>
>>> -.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
>>> -
>>> /*
>>> * The new RIP value that FRED event delivery establishes is
>>> * (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
>>> * ring 0, i.e., asm_fred_entrypoint_user + 256.
>>> */
>>> + .fill asm_fred_entrypoint_user + 256 - ., 1, 0xcc
>>> .org asm_fred_entrypoint_user + 256
>>> SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
>>> FRED_ENTER
>>
>> .fill and .org here are redundant; in fact, there two directives mean exactly the
>> same thing except that .org implicitly subtracts the current offset.
>
> Ah, right, .fill already does the job!
>
> I will remove .org.
>
Incidentally, was there a problem with .org ..., 0xcc?
Not a criticism, I just want to know to better understand current
binutils limitations.
-hpa
> >>> @@ -43,13 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user,
> >> SYM_L_GLOBAL)
> >>> _ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
> >>> SYM_CODE_END(asm_fred_entrypoint_user)
> >>>
> >>> -.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
> >>> -
> >>> /*
> >>> * The new RIP value that FRED event delivery establishes is
> >>> * (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
> >>> * ring 0, i.e., asm_fred_entrypoint_user + 256.
> >>> */
> >>> + .fill asm_fred_entrypoint_user + 256 - ., 1, 0xcc
> >>> .org asm_fred_entrypoint_user + 256
> >>> SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
> >>> FRED_ENTER
> >>
> >> .fill and .org here are redundant; in fact, there two directives mean
> >> exactly the same thing except that .org implicitly subtracts the current offset.
> >
> > Ah, right, .fill already does the job!
> >
> > I will remove .org.
> >
>
> Incidentally, was there a problem with .org ..., 0xcc?
Oh, it's just that I didn't know .org can be used to fill.
> Not a criticism, I just want to know to better understand current binutils
> limitations.
>
> -hpa
Remove the .fill statement that referneces asm_fred_entrypoint_kernel()
before it's defined, which breaks clang build.
Use the .org directive instead to fill "int3" into the memory between
asm_fred_entrypoint_user() and asm_fred_entrypoint_kernel().
Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
Reported-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/lkml/20240126100050.GAZbOC0g3Rlr6otZcT@fat_crate.local/
Signed-off-by: Xin Li <[email protected]>
---
Change since v1:
* Use ".org ..., 0xcc" to fill "int3" into memory (H. Peter Anvin).
---
arch/x86/entry/entry_64_fred.S | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index eedf98de7538..a02bc6f3d2e6 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -43,14 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user, SYM_L_GLOBAL)
_ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
SYM_CODE_END(asm_fred_entrypoint_user)
-.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
-
/*
* The new RIP value that FRED event delivery establishes is
* (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
* ring 0, i.e., asm_fred_entrypoint_user + 256.
*/
- .org asm_fred_entrypoint_user + 256
+ .org asm_fred_entrypoint_user + 256, 0xcc
SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
FRED_ENTER
call fred_entry_from_kernel
--
2.43.0
On Sun, Jan 28, 2024 at 10:45:21PM -0800, Xin Li wrote:
> Remove the .fill statement that referneces asm_fred_entrypoint_kernel()
> before it's defined, which breaks clang build.
>
> Use the .org directive instead to fill "int3" into the memory between
> asm_fred_entrypoint_user() and asm_fred_entrypoint_kernel().
>
> Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
> Reported-by: Borislav Petkov (AMD) <[email protected]>
> Link: https://lore.kernel.org/lkml/20240126100050.GAZbOC0g3Rlr6otZcT@fat_crate.local/
> Signed-off-by: Xin Li <[email protected]>
> ---
>
> Change since v1:
> * Use ".org ..., 0xcc" to fill "int3" into memory (H. Peter Anvin).
> ---
> arch/x86/entry/entry_64_fred.S | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
> index eedf98de7538..a02bc6f3d2e6 100644
> --- a/arch/x86/entry/entry_64_fred.S
> +++ b/arch/x86/entry/entry_64_fred.S
> @@ -43,14 +43,12 @@ SYM_INNER_LABEL(asm_fred_exit_user, SYM_L_GLOBAL)
> _ASM_EXTABLE_TYPE(1b, asm_fred_entrypoint_user, EX_TYPE_ERETU)
> SYM_CODE_END(asm_fred_entrypoint_user)
>
> -.fill asm_fred_entrypoint_kernel - ., 1, 0xcc
> -
> /*
> * The new RIP value that FRED event delivery establishes is
> * (IA32_FRED_CONFIG & ~FFFH) + 256 for events that occur in
> * ring 0, i.e., asm_fred_entrypoint_user + 256.
> */
> - .org asm_fred_entrypoint_user + 256
> + .org asm_fred_entrypoint_user + 256, 0xcc
> SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)
> FRED_ENTER
> call fred_entry_from_kernel
> --
Considering how we're still very early in the game, I'm going to fold
those into the respective patches and rebase so that we have as clean
a branch as possible.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Sat, Jan 27, 2024 at 01:37:28AM -0800, Xin Li wrote:
> When CONFIG_IA32_EMULATION=n, int80_emulation() is NOT defined, fix it.
>
> Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
> Link: https://lore.kernel.org/lkml/20240126100519.GBZbOD3xFB0v3mp5B1@fat_crate.local/
> Reported-by: Borislav Petkov (AMD) <[email protected]>
> Signed-off-by: Xin Li <[email protected]>
> ---
> arch/x86/entry/entry_fred.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
> index 06d00c60ea64..ac120cbdaaf2 100644
> --- a/arch/x86/entry/entry_fred.c
> +++ b/arch/x86/entry/entry_fred.c
> @@ -62,11 +62,13 @@ static noinstr void fred_intx(struct pt_regs *regs)
> case X86_TRAP_OF:
> return exc_overflow(regs);
>
> +#ifdef CONFIG_IA32_EMULATION
> /* INT80 */
> case IA32_SYSCALL_VECTOR:
> if (ia32_enabled())
> return int80_emulation(regs);
> fallthrough;
> +#endif
>
> default:
> return exc_general_protection(regs, 0);
> --
That .config is still not happy after this:
ld: vmlinux.o: in function `fred_entry_from_user':
(.noinstr.text+0x177a): undefined reference to `do_fast_syscall_32'
make[2]: *** [scripts/Makefile.vmlinux:37: vmlinux] Error 1
make[1]: *** [/mnt/kernel/kernel/linux/Makefile:1158: vmlinux] Error 2
make: *** [Makefile:240: __sub-make] Error 2
I'm pushing the latest state I have here:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-fred
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> > When CONFIG_IA32_EMULATION=n, int80_emulation() is NOT defined, fix it.
> >
> > Fixes: 5e0636a41485 ("x86/fred: FRED entry/exit and dispatch code")
> > Link:
> > https://lore.kernel.org/lkml/20240126100519.GBZbOD3xFB0v3mp5B1@fat_cra
> > te.local/
> > Reported-by: Borislav Petkov (AMD) <[email protected]>
> > Signed-off-by: Xin Li <[email protected]>
> > ---
> > arch/x86/entry/entry_fred.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
> > index 06d00c60ea64..ac120cbdaaf2 100644
> > --- a/arch/x86/entry/entry_fred.c
> > +++ b/arch/x86/entry/entry_fred.c
> > @@ -62,11 +62,13 @@ static noinstr void fred_intx(struct pt_regs *regs)
> > case X86_TRAP_OF:
> > return exc_overflow(regs);
> >
> > +#ifdef CONFIG_IA32_EMULATION
> > /* INT80 */
> > case IA32_SYSCALL_VECTOR:
> > if (ia32_enabled())
> > return int80_emulation(regs);
> > fallthrough;
> > +#endif
> >
> > default:
> > return exc_general_protection(regs, 0);
> > --
>
> That .config is still not happy after this:
>
> ld: vmlinux.o: in function `fred_entry_from_user':
> (.noinstr.text+0x177a): undefined reference to `do_fast_syscall_32'
> make[2]: *** [scripts/Makefile.vmlinux:37: vmlinux] Error 1
> make[1]: *** [/mnt/kernel/kernel/linux/Makefile:1158: vmlinux] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
Sign, I'm sorry it still doesn’t work with GCC.
> I'm pushing the latest state I have here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=tip-x86-fred
Because clang can compile it, I checked the generated assembly code and
see that clang optimizes it by simply converting ia32_enabled() to false
when CONFIG_IA32_EMULATION=n thus never calling do_fast_syscall_32().
It looks to me that the following patch is better than adding another
#ifdef CONFIG_IA32_EMULATION around do_fast_syscall_32().
How do you think?
BTW, I have tested it with both GCC and clang with CONFIG_IA32_EMULATION=n.
Thanks!
-Xin
diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h
index c7ef6ea2fa99..01342d343c19 100644
--- a/arch/x86/include/asm/ia32.h
+++ b/arch/x86/include/asm/ia32.h
@@ -81,7 +81,7 @@ static inline void ia32_disable(void)
#else /* !CONFIG_IA32_EMULATION */
-static inline bool ia32_enabled(void)
+static __always_inline bool ia32_enabled(void)
{
return IS_ENABLED(CONFIG_X86_32);
}
On Tue, Jan 30, 2024 at 03:22:01PM +0000, Li, Xin3 wrote:
> How do you think?
Interesting. For some reason gcc doesn't constant-fold it away like
clang does.
> diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h
> index c7ef6ea2fa99..01342d343c19 100644
> --- a/arch/x86/include/asm/ia32.h
> +++ b/arch/x86/include/asm/ia32.h
> @@ -81,7 +81,7 @@ static inline void ia32_disable(void)
>
> #else /* !CONFIG_IA32_EMULATION */
>
> -static inline bool ia32_enabled(void)
> +static __always_inline bool ia32_enabled(void)
> {
> return IS_ENABLED(CONFIG_X86_32);
> }
Looks good to me. Lemme try it here.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> On Tue, Jan 30, 2024 at 03:22:01PM +0000, Li, Xin3 wrote:
> > How do you think?
>
> Interesting. For some reason gcc doesn't constant-fold it away like clang does.
Even more interesting, gcc doesn't complain it with the attached config
File in which CONFIG_X86_FRED=y and CONFIG_IA32_EMULATION not set.
I compared the 2 config files, nothing suspicious to me.
CCing PeterZ to give insights ????
> > diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h
> > index c7ef6ea2fa99..01342d343c19 100644
> > --- a/arch/x86/include/asm/ia32.h
> > +++ b/arch/x86/include/asm/ia32.h
> > @@ -81,7 +81,7 @@ static inline void ia32_disable(void)
> >
> > #else /* !CONFIG_IA32_EMULATION */
> >
> > -static inline bool ia32_enabled(void)
> > +static __always_inline bool ia32_enabled(void)
> > {
> > return IS_ENABLED(CONFIG_X86_32);
> > }
>
> Looks good to me. Lemme try it here.
Thank you very much!
-Xin
On Tue, Jan 30, 2024 at 04:30:34PM +0000, Li, Xin3 wrote:
> Even more interesting, gcc doesn't complain it with the attached config
> File in which CONFIG_X86_FRED=y and CONFIG_IA32_EMULATION not set.
Yes, CONFIG_IA32_EMULATION=n alone is not enough. That .config which
triggers it has something else which is causing this but I'm not sure
I want to go chase down what it is...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Jan 30, 2024 at 03:22:01PM +0000, Li, Xin3 wrote:
> diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h
> index c7ef6ea2fa99..01342d343c19 100644
> --- a/arch/x86/include/asm/ia32.h
> +++ b/arch/x86/include/asm/ia32.h
> @@ -81,7 +81,7 @@ static inline void ia32_disable(void)
>
> #else /* !CONFIG_IA32_EMULATION */
>
> -static inline bool ia32_enabled(void)
> +static __always_inline bool ia32_enabled(void)
> {
> return IS_ENABLED(CONFIG_X86_32);
So I always-inlined both versions since they're small enough, see diff
below. That works with this .config, I'll run some more overnight to
make sure...
Thx.
diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h
index c7ef6ea2fa99..4212c00c9708 100644
--- a/arch/x86/include/asm/ia32.h
+++ b/arch/x86/include/asm/ia32.h
@@ -69,7 +69,7 @@ extern void ia32_pick_mmap_layout(struct mm_struct *mm);
extern bool __ia32_enabled;
-static inline bool ia32_enabled(void)
+static __always_inline bool ia32_enabled(void)
{
return __ia32_enabled;
}
@@ -81,7 +81,7 @@ static inline void ia32_disable(void)
#else /* !CONFIG_IA32_EMULATION */
-static inline bool ia32_enabled(void)
+static __always_inline bool ia32_enabled(void)
{
return IS_ENABLED(CONFIG_X86_32);
}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette