2024-02-29 08:40:47

by Petr Tesarik

Subject: [PATCH 1/1] x86/entry: Use one cmpq in NMI entry to check RIP for nested NMIs

From: Petr Tesarik <[email protected]>

Optimize the check whether a nested NMI occurred between repeat_nmi and
end_repeat_nmi. Although this is not a hot path, this is standard code to
check whether a value is within a given range; it is slightly faster, takes
up less bytes of code and saves one entry in the branch predictor.

This patch also removes the only relocation for end_repeat_nmi, removing
the need for ANNOTATE_NOENDBR.

Signed-off-by: Petr Tesarik <[email protected]>
---
arch/x86/entry/entry_64.S | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 9bb485977629..cae40076e109 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1251,13 +1251,10 @@ SYM_CODE_START(asm_exc_nmi)
* the outer NMI.
*/

- movq $repeat_nmi, %rdx
- cmpq 8(%rsp), %rdx
- ja 1f
- movq $end_repeat_nmi, %rdx
- cmpq 8(%rsp), %rdx
- ja nested_nmi_out
-1:
+ movq 8(%rsp), %rdx
+ subq $repeat_nmi, %rdx
+ cmpq $(end_repeat_nmi - repeat_nmi), %rdx
+ jb nested_nmi_out

/*
* Now check "NMI executing". If it's set, then we're nested.
@@ -1383,8 +1380,6 @@ repeat_nmi:
.endr
subq $(5*8), %rsp
end_repeat_nmi:
- ANNOTATE_NOENDBR // this code
-
/*
* Everything below this point can be preempted by a nested NMI.
* If this happens, then the inner NMI will change the "iret"
--
2.34.1

2024-03-12 15:07:05

by Petr Tesarik

Subject: Re: [PATCH 1/1] x86/entry: Use one cmpq in NMI entry to check RIP for nested NMIs

On 2/29/2024 9:37 AM, Petr Tesarik wrote:
> From: Petr Tesarik <[email protected]>
>
> Optimize the check whether a nested NMI occurred between repeat_nmi and
> end_repeat_nmi. Although this is not a hot path, this is standard code to
> check whether a value is within a given range; it is slightly faster, takes
> up less bytes of code and saves one entry in the branch predictor.
>
> This patch also removes the only relocation for end_repeat_nmi, removing
> the need for ANNOTATE_NOENDBR.

Any comment on this?

Kind regards
Petr T

> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> arch/x86/entry/entry_64.S | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 9bb485977629..cae40076e109 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1251,13 +1251,10 @@ SYM_CODE_START(asm_exc_nmi)
> * the outer NMI.
> */
>
> - movq $repeat_nmi, %rdx
> - cmpq 8(%rsp), %rdx
> - ja 1f
> - movq $end_repeat_nmi, %rdx
> - cmpq 8(%rsp), %rdx
> - ja nested_nmi_out
> -1:
> + movq 8(%rsp), %rdx
> + subq $repeat_nmi, %rdx
> + cmpq $(end_repeat_nmi - repeat_nmi), %rdx
> + jb nested_nmi_out
>
> /*
> * Now check "NMI executing". If it's set, then we're nested.
> @@ -1383,8 +1380,6 @@ repeat_nmi:
> .endr
> subq $(5*8), %rsp
> end_repeat_nmi:
> - ANNOTATE_NOENDBR // this code
> -
> /*
> * Everything below this point can be preempted by a nested NMI.
> * If this happens, then the inner NMI will change the "iret"

2024-03-25 13:54:15

by Petr Tesařík

Subject: Re: [PATCH 1/1] x86/entry: Use one cmpq in NMI entry to check RIP for nested NMIs

On Tue, 12 Mar 2024 16:03:17 +0100
Petr Tesarik <[email protected]> wrote:

> On 2/29/2024 9:37 AM, Petr Tesarik wrote:
> > From: Petr Tesarik <[email protected]>
> >
> > Optimize the check whether a nested NMI occurred between repeat_nmi and
> > end_repeat_nmi. Although this is not a hot path, this is standard code to
> > check whether a value is within a given range; it is slightly faster, takes
> > up less bytes of code and saves one entry in the branch predictor.
> >
> > This patch also removes the only relocation for end_repeat_nmi, removing
> > the need for ANNOTATE_NOENDBR.
>
> Any comment on this?

Ping. If you don't like the change, can I at least get an orderly NAK?

(Or have I made it onto an x86 maintainer blacklist?)

Petr T

> Kind regards
> Petr T
>
> > Signed-off-by: Petr Tesarik <[email protected]>
> > ---
> > arch/x86/entry/entry_64.S | 13 ++++---------
> > 1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> > index 9bb485977629..cae40076e109 100644
> > --- a/arch/x86/entry/entry_64.S
> > +++ b/arch/x86/entry/entry_64.S
> > @@ -1251,13 +1251,10 @@ SYM_CODE_START(asm_exc_nmi)
> > * the outer NMI.
> > */
> >
> > - movq $repeat_nmi, %rdx
> > - cmpq 8(%rsp), %rdx
> > - ja 1f
> > - movq $end_repeat_nmi, %rdx
> > - cmpq 8(%rsp), %rdx
> > - ja nested_nmi_out
> > -1:
> > + movq 8(%rsp), %rdx
> > + subq $repeat_nmi, %rdx
> > + cmpq $(end_repeat_nmi - repeat_nmi), %rdx
> > + jb nested_nmi_out
> >
> > /*
> > * Now check "NMI executing". If it's set, then we're nested.
> > @@ -1383,8 +1380,6 @@ repeat_nmi:
> > .endr
> > subq $(5*8), %rsp
> > end_repeat_nmi:
> > - ANNOTATE_NOENDBR // this code
> > -
> > /*
> > * Everything below this point can be preempted by a nested NMI.
> > * If this happens, then the inner NMI will change the "iret"
>