[PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[Suravee Suthikulpanit]

The commit 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window
while running L2 with V_INTR_MASKING=1") introduced a WARN_ON,
which checks if AVIC is enabled when trying to set V_IRQ
in the VMCB for enabling irq window.

The following warning is triggered because the requesting vcpu
(to deactivate AVIC) does not get to process APICv update request
for itself until the next #vmexit.

WARNING: CPU: 0 PID: 118232 at arch/x86/kvm/svm/svm.c:1372 enable_irq_window+0x6a/0xa0 [kvm_amd]
 RIP: 0010:enable_irq_window+0x6a/0xa0 [kvm_amd]
 Call Trace:
  kvm_arch_vcpu_ioctl_run+0x6e3/0x1b50 [kvm]
  ? kvm_vm_ioctl_irq_line+0x27/0x40 [kvm]
  ? _copy_to_user+0x26/0x30
  ? kvm_vm_ioctl+0xb3e/0xd90 [kvm]
  ? set_next_entity+0x78/0xc0
  kvm_vcpu_ioctl+0x236/0x610 [kvm]
  ksys_ioctl+0x8a/0xc0
  __x64_sys_ioctl+0x1a/0x20
  do_syscall_64+0x58/0x210
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes by sending APICV update request to all other vcpus, and
immediately update APIC for itself.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Link: https://lkml.org/lkml/2020/5/2/167
Fixes: 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1")
---
 arch/x86/kvm/x86.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index df473f9..69a01ea 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8085,6 +8085,7 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
  */
 void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
 {
+	struct kvm_vcpu *except;
 	unsigned long old, new, expected;
 
 	if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
@@ -8110,7 +8111,17 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
 	trace_kvm_apicv_update_request(activate, bit);
 	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
 		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
-	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
+
+	/*
+	 * Sending request to update APICV for all other vcpus,
+	 * while update the calling vcpu immediately instead of
+	 * waiting for another #VMEXIT to handle the request.
+	 */
+	except = kvm_get_running_vcpu();
+	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
+					 except);
+	if (except)
+		kvm_vcpu_update_apicv(except);
 }
 EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
 
-- 
1.8.3.1

Re: [PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[Paolo Bonzini]

On 07/05/20 04:35, Suravee Suthikulpanit wrote:
> The commit 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window
> while running L2 with V_INTR_MASKING=1") introduced a WARN_ON,
> which checks if AVIC is enabled when trying to set V_IRQ
> in the VMCB for enabling irq window.
> 
> The following warning is triggered because the requesting vcpu
> (to deactivate AVIC) does not get to process APICv update request
> for itself until the next #vmexit.
> 
> WARNING: CPU: 0 PID: 118232 at arch/x86/kvm/svm/svm.c:1372 enable_irq_window+0x6a/0xa0 [kvm_amd]
>  RIP: 0010:enable_irq_window+0x6a/0xa0 [kvm_amd]
>  Call Trace:
>   kvm_arch_vcpu_ioctl_run+0x6e3/0x1b50 [kvm]
>   ? kvm_vm_ioctl_irq_line+0x27/0x40 [kvm]
>   ? _copy_to_user+0x26/0x30
>   ? kvm_vm_ioctl+0xb3e/0xd90 [kvm]
>   ? set_next_entity+0x78/0xc0
>   kvm_vcpu_ioctl+0x236/0x610 [kvm]
>   ksys_ioctl+0x8a/0xc0
>   __x64_sys_ioctl+0x1a/0x20
>   do_syscall_64+0x58/0x210
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Fixes by sending APICV update request to all other vcpus, and
> immediately update APIC for itself.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Link: https://lkml.org/lkml/2020/5/2/167
> Fixes: 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1")
> ---
>  arch/x86/kvm/x86.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index df473f9..69a01ea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8085,6 +8085,7 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>   */
>  void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>  {
> +	struct kvm_vcpu *except;
>  	unsigned long old, new, expected;
>  
>  	if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
> @@ -8110,7 +8111,17 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>  	trace_kvm_apicv_update_request(activate, bit);
>  	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
>  		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
> -	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
> +
> +	/*
> +	 * Sending request to update APICV for all other vcpus,
> +	 * while update the calling vcpu immediately instead of
> +	 * waiting for another #VMEXIT to handle the request.
> +	 */
> +	except = kvm_get_running_vcpu();
> +	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
> +					 except);
> +	if (except)
> +		kvm_vcpu_update_apicv(except);
>  }
>  EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
>  
> 

Queued, thanks.

Paolo

Re: [PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]

Hi Suravee,

I love your patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next linus/master v5.7-rc4 next-20200507]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Suravee-Suthikulpanit/KVM-SVM-Disable-AVIC-before-setting-V_IRQ/20200507-111704
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 54b35c066417d4856e9d53313f7e98b354274584)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> arch/x86/kvm/x86.c:8107:2: error: implicit declaration of function 'kvm_make_all_cpus_request_except' [-Werror,-Wimplicit-function-declaration]
           kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
           ^
   arch/x86/kvm/x86.c:8107:2: note: did you mean 'kvm_make_all_cpus_request'?
   include/linux/kvm_host.h:818:6: note: 'kvm_make_all_cpus_request' declared here
   bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req);
        ^
   1 error generated.

vim +/kvm_make_all_cpus_request_except +8107 arch/x86/kvm/x86.c

  8065	
  8066	/*
  8067	 * NOTE: Do not hold any lock prior to calling this.
  8068	 *
  8069	 * In particular, kvm_request_apicv_update() expects kvm->srcu not to be
  8070	 * locked, because it calls __x86_set_memory_region() which does
  8071	 * synchronize_srcu(&kvm->srcu).
  8072	 */
  8073	void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
  8074	{
  8075		struct kvm_vcpu *except;
  8076		unsigned long old, new, expected;
  8077	
  8078		if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
  8079		    !kvm_x86_ops.check_apicv_inhibit_reasons(bit))
  8080			return;
  8081	
  8082		old = READ_ONCE(kvm->arch.apicv_inhibit_reasons);
  8083		do {
  8084			expected = new = old;
  8085			if (activate)
  8086				__clear_bit(bit, &new);
  8087			else
  8088				__set_bit(bit, &new);
  8089			if (new == old)
  8090				break;
  8091			old = cmpxchg(&kvm->arch.apicv_inhibit_reasons, expected, new);
  8092		} while (old != expected);
  8093	
  8094		if (!!old == !!new)
  8095			return;
  8096	
  8097		trace_kvm_apicv_update_request(activate, bit);
  8098		if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
  8099			kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
  8100	
  8101		/*
  8102		 * Sending request to update APICV for all other vcpus,
  8103		 * while update the calling vcpu immediately instead of
  8104		 * waiting for another #VMEXIT to handle the request.
  8105		 */
  8106		except = kvm_get_running_vcpu();
> 8107		kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
  8108						 except);
  8109		if (except)
  8110			kvm_vcpu_update_apicv(except);
  8111	}
  8112	EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
  8113	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72774 bytes --]

Re: [PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3224 bytes --]

Hi Suravee,

I love your patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next linus/master v5.7-rc4 next-20200507]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Suravee-Suthikulpanit/KVM-SVM-Disable-AVIC-before-setting-V_IRQ/20200507-111704
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/x86/kvm/x86.c: In function 'kvm_request_apicv_update':
>> arch/x86/kvm/x86.c:8107:2: error: implicit declaration of function 'kvm_make_all_cpus_request_except'; did you mean 'kvm_make_all_cpus_request'? [-Werror=implicit-function-declaration]
     kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     kvm_make_all_cpus_request
   cc1: all warnings being treated as errors

vim +8107 arch/x86/kvm/x86.c

  8065	
  8066	/*
  8067	 * NOTE: Do not hold any lock prior to calling this.
  8068	 *
  8069	 * In particular, kvm_request_apicv_update() expects kvm->srcu not to be
  8070	 * locked, because it calls __x86_set_memory_region() which does
  8071	 * synchronize_srcu(&kvm->srcu).
  8072	 */
  8073	void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
  8074	{
  8075		struct kvm_vcpu *except;
  8076		unsigned long old, new, expected;
  8077	
  8078		if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
  8079		    !kvm_x86_ops.check_apicv_inhibit_reasons(bit))
  8080			return;
  8081	
  8082		old = READ_ONCE(kvm->arch.apicv_inhibit_reasons);
  8083		do {
  8084			expected = new = old;
  8085			if (activate)
  8086				__clear_bit(bit, &new);
  8087			else
  8088				__set_bit(bit, &new);
  8089			if (new == old)
  8090				break;
  8091			old = cmpxchg(&kvm->arch.apicv_inhibit_reasons, expected, new);
  8092		} while (old != expected);
  8093	
  8094		if (!!old == !!new)
  8095			return;
  8096	
  8097		trace_kvm_apicv_update_request(activate, bit);
  8098		if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
  8099			kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
  8100	
  8101		/*
  8102		 * Sending request to update APICV for all other vcpus,
  8103		 * while update the calling vcpu immediately instead of
  8104		 * waiting for another #VMEXIT to handle the request.
  8105		 */
  8106		except = kvm_get_running_vcpu();
> 8107		kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
  8108						 except);
  8109		if (except)
  8110			kvm_vcpu_update_apicv(except);
  8111	}
  8112	EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
  8113	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44840 bytes --]

Re: [PATCH v2] KVM: SVM: Disable AVIC before setting V_IRQ

[Maxim Levitsky]

On Thu, 2020-05-07 at 10:27 +0200, Paolo Bonzini wrote:
> On 07/05/20 04:35, Suravee Suthikulpanit wrote:
> > The commit 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window
> > while running L2 with V_INTR_MASKING=1") introduced a WARN_ON,
> > which checks if AVIC is enabled when trying to set V_IRQ
> > in the VMCB for enabling irq window.
> > 
> > The following warning is triggered because the requesting vcpu
> > (to deactivate AVIC) does not get to process APICv update request
> > for itself until the next #vmexit.
> > 
> > WARNING: CPU: 0 PID: 118232 at arch/x86/kvm/svm/svm.c:1372 enable_irq_window+0x6a/0xa0 [kvm_amd]
> >  RIP: 0010:enable_irq_window+0x6a/0xa0 [kvm_amd]
> >  Call Trace:
> >   kvm_arch_vcpu_ioctl_run+0x6e3/0x1b50 [kvm]
> >   ? kvm_vm_ioctl_irq_line+0x27/0x40 [kvm]
> >   ? _copy_to_user+0x26/0x30
> >   ? kvm_vm_ioctl+0xb3e/0xd90 [kvm]
> >   ? set_next_entity+0x78/0xc0
> >   kvm_vcpu_ioctl+0x236/0x610 [kvm]
> >   ksys_ioctl+0x8a/0xc0
> >   __x64_sys_ioctl+0x1a/0x20
> >   do_syscall_64+0x58/0x210
> >   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > 
> > Fixes by sending APICV update request to all other vcpus, and
> > immediately update APIC for itself.
> > 
> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > Link: https://lkml.org/lkml/2020/5/2/167
> > Fixes: 64b5bd270426 ("KVM: nSVM: ignore L1 interrupt window while running L2 with V_INTR_MASKING=1")
> > ---
> >  arch/x86/kvm/x86.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index df473f9..69a01ea 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8085,6 +8085,7 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> >   */
> >  void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> >  {
> > +	struct kvm_vcpu *except;
> >  	unsigned long old, new, expected;
> >  
> >  	if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
> > @@ -8110,7 +8111,17 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> >  	trace_kvm_apicv_update_request(activate, bit);
> >  	if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
> >  		kvm_x86_ops.pre_update_apicv_exec_ctrl(kvm, activate);
> > -	kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
> > +
> > +	/*
> > +	 * Sending request to update APICV for all other vcpus,
> > +	 * while update the calling vcpu immediately instead of
> > +	 * waiting for another #VMEXIT to handle the request.
> > +	 */
> > +	except = kvm_get_running_vcpu();
> > +	kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
> > +					 except);
> > +	if (except)
> > +		kvm_vcpu_update_apicv(except);
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
> >  
> > 
> 
> Queued, thanks.
> 
> Paolo
> 
I tested this patch today on top of kvm/queue,
the patch that add kvm_make_all_cpus_request_except and this patch
(the former patch needs slight adjustment to apply).

Best regards,
	Maxim Levitsky