Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220518134550.2358-1-ubizjak@gmail.com>
In-Reply-To: <20220518134550.2358-1-ubizjak@gmail.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Thu, 19 May 2022 23:34:00 +0200
Message-ID: <CAFULd4Y3ZSfBtx3ZbLcT3DtorGxPqyWeOmrMxHS4b3AKH2cxVw@mail.gmail.com>
Subject: Re: [PATCH] KVM: VMX: Use try_cmpxchg64 in pi_try_set_control
To:     kvm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 18, 2022 at 3:46 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Use try_cmpxchg64 instead of cmpxchg64 (*ptr, old, new) != old
> in pi_try_set_control.  cmpxchg returns success in ZF flag, so this
> change saves a compare after cmpxchg (and related move instruction
> in front of cmpxchg):
>
>   b9:   88 44 24 60             mov    %al,0x60(%rsp)
>   bd:   48 89 c8                mov    %rcx,%rax
>   c0:   c6 44 24 62 f2          movb   $0xf2,0x62(%rsp)
>   c5:   48 8b 74 24 60          mov    0x60(%rsp),%rsi
>   ca:   f0 49 0f b1 34 24       lock cmpxchg %rsi,(%r12)
>   d0:   48 39 c1                cmp    %rax,%rcx
>   d3:   75 cf                   jne    a4 <vmx_vcpu_pi_load+0xa4>
>
> patched:
>
>   c1:   88 54 24 60             mov    %dl,0x60(%rsp)
>   c5:   c6 44 24 62 f2          movb   $0xf2,0x62(%rsp)
>   ca:   48 8b 54 24 60          mov    0x60(%rsp),%rdx
>   cf:   f0 48 0f b1 13          lock cmpxchg %rdx,(%rbx)
>   d4:   75 d5                   jne    ab <vmx_vcpu_pi_load+0xab>
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
> Patch requires commits 0aa7be05d83cc584da0782405e8007e351dfb6cc
> and c2df0a6af177b6c06a859806a876f92b072dc624 from tip.git
> ---
>  arch/x86/kvm/vmx/posted_intr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
> index 3834bb30ce54..4d41d5994a26 100644
> --- a/arch/x86/kvm/vmx/posted_intr.c
> +++ b/arch/x86/kvm/vmx/posted_intr.c
> @@ -34,7 +34,7 @@ static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu)
>         return &(to_vmx(vcpu)->pi_desc);
>  }
>
> -static int pi_try_set_control(struct pi_desc *pi_desc, u64 old, u64 new)
> +static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
>  {
>         /*
>          * PID.ON can be set at any time by a different vCPU or by hardware,
> @@ -42,7 +42,7 @@ static int pi_try_set_control(struct pi_desc *pi_desc, u64 old, u64 new)
>          * update must be retried with a fresh snapshot an ON change causes
>          * the cmpxchg to fail.
>          */
> -       if (cmpxchg64(&pi_desc->control, old, new) != old)
> +       if (!try_cmpxchg64(&pi_desc->control, pold, new))

Actually, we can just use the address of local variable &old here, no
need to change function arguments.

Patch v2 incoming...

Uros.