Date:   Mon, 10 Jan 2022 22:59:52 +0000
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
Message-ID: <Ydy6aIyI3jFQvF0O@google.com>
References: <20211227081515.2088920-1-chao.gao@intel.com>
 <20211227081515.2088920-6-chao.gao@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211227081515.2088920-6-chao.gao@intel.com>
Precedence: bulk

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
>