kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Chao Gao <chao.gao@intel.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kevin.tian@intel.com,
	tglx@linutronix.de, Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat
Date: Mon, 10 Jan 2022 22:59:52 +0000	[thread overview]
Message-ID: <Ydy6aIyI3jFQvF0O@google.com> (raw)
In-Reply-To: <20211227081515.2088920-6-chao.gao@intel.com>

On Mon, Dec 27, 2021, Chao Gao wrote:
> kvm_arch_check_processor_compat() needn't be called with interrupt
> disabled, as it only reads some CRs/MSRs which won't be clobbered
> by interrupt handlers or softirq.
> 
> What really needed is disabling preemption. No additional check is
> added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> (right above the WARN_ON()) can help to detect any violation.

Hrm, IIRC, the assertion that IRQs are disabled was more about detecting improper
usage with respect to KVM doing hardware enabling than it was about ensuring the
current task isn't migrated.  E.g. as exhibited by patch 06, extra protections
(disabling of hotplug in that case) are needed if this helper is called outside
of the core KVM hardware enabling flow since hardware_enable_all() does its thing
via SMP function call.

Is there CPU onlining state/metadata that we could use to handle that specific case?
It'd be nice to preserve the paranoid check, but it's not a big deal if we can't.

If we can't preserve the WARN, can you rework the changelog to explain the motivation
for removing the WARN?

Thanks!

> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
>  arch/x86/kvm/x86.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index aa09c8792134..a80e3b0c11a8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11384,8 +11384,6 @@ int kvm_arch_check_processor_compat(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
>  
> -	WARN_ON(!irqs_disabled());
> -
>  	if (__cr4_reserved_bits(cpu_has, c) !=
>  	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
>  		return -EIO;
> -- 
> 2.25.1
> 

  reply	other threads:[~2022-01-10 22:59 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-27  8:15 [PATCH 0/6] Improve KVM's interaction with CPU hotplug Chao Gao
2021-12-27  8:15 ` [PATCH 1/6] KVM: x86: Move check_processor_compatibility from init ops to runtime ops Chao Gao
2022-01-10 23:27   ` Sean Christopherson
2022-01-11  3:36     ` Chao Gao
2022-01-12 17:59       ` Sean Christopherson
2021-12-27  8:15 ` [PATCH 2/6] KVM: x86: Use kvm_x86_ops in kvm_arch_check_processor_compat Chao Gao
2022-01-10 21:10   ` Sean Christopherson
2022-01-11  3:06     ` Chao Gao
2021-12-27  8:15 ` [PATCH 3/6] KVM: Remove opaque from kvm_arch_check_processor_compat Chao Gao
2022-01-10 23:06   ` Sean Christopherson
2022-01-11  3:19     ` Chao Gao
2022-01-12 17:20       ` Sean Christopherson
2022-01-12 17:21         ` Sean Christopherson
2021-12-27  8:15 ` [PATCH 4/6] KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section Chao Gao
2021-12-27  8:15 ` [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor_compat Chao Gao
2022-01-10 22:59   ` Sean Christopherson [this message]
2022-01-11  2:15     ` Tian, Kevin
2022-01-11 19:48       ` Sean Christopherson
2022-01-12 11:00         ` Chao Gao
2022-01-12 17:35           ` Sean Christopherson
2022-01-17 13:35             ` Chao Gao
2022-01-17 13:46               ` Chao Gao
2022-01-19  0:34                 ` Sean Christopherson
2021-12-27  8:15 ` [PATCH 6/6] KVM: Do compatibility checks on hotplugged CPUs Chao Gao
2022-01-11  0:46   ` Sean Christopherson
2022-01-11  5:32     ` Chao Gao
2022-01-12 17:52       ` Sean Christopherson
2022-01-12 23:01         ` Jim Mattson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Ydy6aIyI3jFQvF0O@google.com \
    --to=seanjc@google.com \
    --cc=bp@alien8.de \
    --cc=chao.gao@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).