linux-kernel - Re: [PATCH v2 40/50] KVM: x86: Do compatibility checks when onlining CPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y4oh+XsbifA2BSj9@google.com>
Date:   Fri, 2 Dec 2022 16:04:09 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     "Huang, Kai" <kai.huang@...el.com>
Cc:     "chenhuacai@...nel.org" <chenhuacai@...nel.org>,
        "maz@...nel.org" <maz@...nel.org>,
        "frankja@...ux.ibm.com" <frankja@...ux.ibm.com>,
        "borntraeger@...ux.ibm.com" <borntraeger@...ux.ibm.com>,
        "farman@...ux.ibm.com" <farman@...ux.ibm.com>,
        "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
        "palmer@...belt.com" <palmer@...belt.com>,
        "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "aleksandar.qemu.devel@...il.com" <aleksandar.qemu.devel@...il.com>,
        "imbrenda@...ux.ibm.com" <imbrenda@...ux.ibm.com>,
        "paul@....org" <paul@....org>,
        "mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>,
        "anup@...infault.org" <anup@...infault.org>,
        "oliver.upton@...ux.dev" <oliver.upton@...ux.dev>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "farosas@...ux.ibm.com" <farosas@...ux.ibm.com>,
        "david@...hat.com" <david@...hat.com>,
        "james.morse@....com" <james.morse@....com>,
        "Yao, Yuan" <yuan.yao@...el.com>,
        "alexandru.elisei@....com" <alexandru.elisei@....com>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mpe@...erman.id.au" <mpe@...erman.id.au>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "kvmarm@...ts.linux.dev" <kvmarm@...ts.linux.dev>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "suzuki.poulose@....com" <suzuki.poulose@....com>,
        "kvm-riscv@...ts.infradead.org" <kvm-riscv@...ts.infradead.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
        "philmd@...aro.org" <philmd@...aro.org>,
        "atishp@...shpatra.org" <atishp@...shpatra.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "Gao, Chao" <chao.gao@...el.com>
Subject: Re: [PATCH v2 40/50] KVM: x86: Do compatibility checks when onlining
 CPU

On Fri, Dec 02, 2022, Huang, Kai wrote:
> On Wed, 2022-11-30 at 23:09 +0000, Sean Christopherson wrote:
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11967,6 +11967,11 @@ int kvm_arch_hardware_enable(void)
> >  	bool stable, backwards_tsc = false;
> >  
> >  	kvm_user_return_msr_cpu_online();
> > +
> > +	ret = kvm_x86_check_processor_compatibility();
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = static_call(kvm_x86_hardware_enable)();
> >  	if (ret != 0)
> >  		return ret;
> 
> Thinking more, AFAICT, kvm_x86_vendor_init() so far still does the compatibility
> check on all online cpus.  Since now kvm_arch_hardware_enable() also does the
> compatibility check, IIUC the compatibility check will be done twice -- one in
> kvm_x86_vendor_init() and one in hardware_enable_all() when creating the first
> VM.
> 
> Do you think it's still worth to do compatibility check in vm_x86_vendor_init()?
> 
> The behaviour difference should be "KVM module fail to load" vs "failing to
> create the first VM" IIUC.  I don't know whether the former is better than the
> better, but it seems duplicated compatibility checking isn't needed?

It's not strictly needed, but I think it's worth keeping.  The duplicate checking
annoys me too, and I considered removing it multiple times when creating this
series.  But, if there is a hardware incompatibility for whatever reason, failing
to load and thus not instantiating /dev/kvm is friendlier to userspace, e.g.
userspace can immediately flag the platform as potentially flaky, whereas
detecting the likely hardware issue when VM creation fails would essentialy require
scraping the kernel logs.