Message ID | 20220518134550.2358-1-ubizjak@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: VMX: Use try_cmpxchg64 in pi_try_set_control | expand |
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/202205190852.VUijQkwc-lkp@intel.com/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 <lkp@intel.com> 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
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/202205191028.PcCtw7Xz-lkp@intel.com/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 <lkp@intel.com> 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
On Thu, May 19, 2022 at 3:05 AM kernel test robot <lkp@intel.com> 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/202205190852.VUijQkwc-lkp@intel.com/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 <lkp@intel.com> > > 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.
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.
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
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(-)