linux-kernel - Re: [PATCH] x86/ACPI/boot: Improve __acpi_acquire_global

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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gCk2WtL6+zE9q8KMB+p=bywT=2uZM8VCtmTzWo8MoGdQ@mail.gmail.com>
Date:   Mon, 20 Mar 2023 18:12:43 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Uros Bizjak <ubizjak@...il.com>
Cc:     x86@...nel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] x86/ACPI/boot: Improve __acpi_acquire_global_lock

On Mon, Feb 27, 2023 at 5:48 PM Uros Bizjak <ubizjak@...il.com> wrote:
>
> Improve __acpi_acquire_global_lock by using a temporary variable.
> This enables compiler to perform if-conversion and improves generated
> code from:
>
>  ...
>  72a:   d1 ea                   shr    %edx
>  72c:   83 e1 fc                and    $0xfffffffc,%ecx
>  72f:   83 e2 01                and    $0x1,%edx
>  732:   09 ca                   or     %ecx,%edx
>  734:   83 c2 02                add    $0x2,%edx
>  737:   f0 0f b1 17             lock cmpxchg %edx,(%rdi)
>  73b:   75 e9                   jne    726 <__acpi_acquire_global_lock+0x6>
>  73d:   83 e2 03                and    $0x3,%edx
>  740:   31 c0                   xor    %eax,%eax
>  742:   83 fa 03                cmp    $0x3,%edx
>  745:   0f 95 c0                setne  %al
>  748:   f7 d8                   neg    %eax
>
> to:
>
>  ...
>  72a:   d1 e9                   shr    %ecx
>  72c:   83 e2 fc                and    $0xfffffffc,%edx
>  72f:   83 e1 01                and    $0x1,%ecx
>  732:   09 ca                   or     %ecx,%edx
>  734:   83 c2 02                add    $0x2,%edx
>  737:   f0 0f b1 17             lock cmpxchg %edx,(%rdi)
>  73b:   75 e9                   jne    726 <__acpi_acquire_global_lock+0x6>
>  73d:   8d 41 ff                lea    -0x1(%rcx),%eax
>
> BTW: the compiler could generate:
>
>         lea 0x2(%rcx,%rdx,1),%edx
>
> instead of:
>
>         or     %ecx,%edx
>         add    $0x2,%edx
>
> but unwated conversion from add to or when bits are known to be zero
> prevents this improvement. This is GCC PR108477.
>
> No functional change intended.
>
> Signed-off-by: Uros Bizjak <ubizjak@...il.com>
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> Cc: Len Brown <lenb@...nel.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> ---
>  arch/x86/kernel/acpi/boot.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 1c38174b5f01..577403c69983 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1853,13 +1853,14 @@ early_param("acpi_sci", setup_acpi_sci);
>
>  int __acpi_acquire_global_lock(unsigned int *lock)
>  {
> -       unsigned int old, new;
> +       unsigned int old, new, val;
>
>         old = READ_ONCE(*lock);
>         do {
> -               new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
> +               val = (old >> 1) & 0x1;
> +               new = (old & ~0x3) + 2 + val;
>         } while (!try_cmpxchg(lock, &old, new));
> -       return ((new & 0x3) < 3) ? -1 : 0;
> +       return val ? 0 : -1;

I would prefer

if (val)
        return -1;

return 0;

>  }
>
>  int __acpi_release_global_lock(unsigned int *lock)
> --

Otherwise I agree that this change would be an improvement.

>