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 <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Uros Bizjak <[email protected]>
---
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))
return -EBUSY;
return 0;
@@ -111,7 +111,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
* descriptor was modified on "put" to use the wakeup vector.
*/
new.nv = POSTED_INTR_VECTOR;
- } while (pi_try_set_control(pi_desc, old.control, new.control));
+ } while (pi_try_set_control(pi_desc, &old.control, new.control));
local_irq_restore(flags);
@@ -161,7 +161,7 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
/* set 'NV' to 'wakeup vector' */
new.nv = POSTED_INTR_WAKEUP_VECTOR;
- } while (pi_try_set_control(pi_desc, old.control, new.control));
+ } while (pi_try_set_control(pi_desc, &old.control, new.control));
/*
* Send a wakeup IPI to this CPU if an interrupt may have been posted
--
2.35.1
Hi Uros,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/master]
[also build test ERROR on mst-vhost/linux-next linux/master linus/master v5.18-rc7 next-20220518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git master
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220519/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
git checkout 2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> arch/x86/kvm/vmx/posted_intr.c:45:7: error: call to undeclared function 'try_cmpxchg64'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
if (!try_cmpxchg64(&pi_desc->control, pold, new))
^
arch/x86/kvm/vmx/posted_intr.c:45:7: note: did you mean '__cmpxchg64'?
arch/x86/include/asm/cmpxchg_32.h:47:19: note: '__cmpxchg64' declared here
static inline u64 __cmpxchg64(volatile u64 *ptr, u64 old, u64 new)
^
1 error generated.
vim +/try_cmpxchg64 +45 arch/x86/kvm/vmx/posted_intr.c
36
37 static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
38 {
39 /*
40 * PID.ON can be set at any time by a different vCPU or by hardware,
41 * e.g. a device. PID.control must be written atomically, and the
42 * update must be retried with a fresh snapshot an ON change causes
43 * the cmpxchg to fail.
44 */
> 45 if (!try_cmpxchg64(&pi_desc->control, pold, new))
46 return -EBUSY;
47
48 return 0;
49 }
50
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Thu, May 19, 2022 at 3:05 AM kernel test robot <[email protected]> wrote:
>
> Hi Uros,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on kvm/master]
> [also build test ERROR on mst-vhost/linux-next linux/master linus/master v5.18-rc7 next-20220518]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
> base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git master
> config: i386-debian-10.3-kselftests (https://download.01.org/0day-ci/archive/20220519/[email protected]/config)
> compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
> reproduce (this is a W=1 build):
> # https://github.com/intel-lab-lkp/linux/commit/2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
> git checkout 2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
As stated in the patch submission, this patch needs locking
infrastructure patches that are currently in the tip git, so there is
a small patch ordering issue between tip and KVM trees.
Uros.
Hi Uros,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/master]
[also build test ERROR on mst-vhost/linux-next linux/master linus/master v5.18-rc7 next-20220518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git master
config: i386-debian-10.3-kselftests (https://download.01.org/0day-ci/archive/20220519/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Uros-Bizjak/KVM-VMX-Use-try_cmpxchg64-in-pi_try_set_control/20220518-214709
git checkout 2af4f7c4ecfcaedf9b98ba30ee508dc0d9002955
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
arch/x86/kvm/vmx/posted_intr.c: In function 'pi_try_set_control':
>> arch/x86/kvm/vmx/posted_intr.c:45:14: error: implicit declaration of function 'try_cmpxchg64'; did you mean 'try_cmpxchg'? [-Werror=implicit-function-declaration]
45 | if (!try_cmpxchg64(&pi_desc->control, pold, new))
| ^~~~~~~~~~~~~
| try_cmpxchg
cc1: some warnings being treated as errors
vim +45 arch/x86/kvm/vmx/posted_intr.c
36
37 static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
38 {
39 /*
40 * PID.ON can be set at any time by a different vCPU or by hardware,
41 * e.g. a device. PID.control must be written atomically, and the
42 * update must be retried with a fresh snapshot an ON change causes
43 * the cmpxchg to fail.
44 */
> 45 if (!try_cmpxchg64(&pi_desc->control, pold, new))
46 return -EBUSY;
47
48 return 0;
49 }
50
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, May 18, 2022 at 3:46 PM Uros Bizjak <[email protected]> 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 <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Uros Bizjak <[email protected]>
> ---
> 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.