| 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
| ||
|
Date: Thu, 17 Nov 2022 01:39:23 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Christopherson,, Sean" <seanjc@...gle.com>
CC: "borntraeger@...ux.ibm.com" <borntraeger@...ux.ibm.com>,
"Yao, Yuan" <yuan.yao@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"kvm-riscv@...ts.infradead.org" <kvm-riscv@...ts.infradead.org>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>,
"suzuki.poulose@....com" <suzuki.poulose@....com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"david@...hat.com" <david@...hat.com>,
"linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
"kvmarm@...ts.linux.dev" <kvmarm@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>,
"oliver.upton@...ux.dev" <oliver.upton@...ux.dev>,
"farosas@...ux.ibm.com" <farosas@...ux.ibm.com>,
"linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
"palmer@...belt.com" <palmer@...belt.com>,
"chenhuacai@...nel.org" <chenhuacai@...nel.org>,
"mpe@...erman.id.au" <mpe@...erman.id.au>,
"aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
"alexandru.elisei@....com" <alexandru.elisei@....com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"maz@...nel.org" <maz@...nel.org>,
"anup@...infault.org" <anup@...infault.org>,
"frankja@...ux.ibm.com" <frankja@...ux.ibm.com>,
"aleksandar.qemu.devel@...il.com" <aleksandar.qemu.devel@...il.com>,
"james.morse@....com" <james.morse@....com>,
"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
"farman@...ux.ibm.com" <farman@...ux.ibm.com>,
"paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"atishp@...shpatra.org" <atishp@...shpatra.org>,
"imbrenda@...ux.ibm.com" <imbrenda@...ux.ibm.com>,
"Gao, Chao" <chao.gao@...el.com>
Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling
On Wed, 2022-11-16 at 17:11 +0000, Sean Christopherson wrote:
> On Wed, Nov 16, 2022, Huang, Kai wrote:
> > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote:
> > > On Thu, Nov 10, 2022, Huang, Kai wrote:
> > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote:
> > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also
> > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and
> > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is
> > > > false.
> > >
> > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And
> > > the reason for that is because you're wrong about the hotplug case. In this version
> > > of things, the compatibility checks are routed through hardware enabling, i.e. this
> > > flow is used only when loading KVM. This helper should only be called via SMP function
> > > call, which means that IRQs should always be disabled.
> >
> > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop
> > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"?
> >
> > /*
> > * Abort the CPU online process if hardware virtualization cannot
> > * be enabled. Otherwise running VMs would encounter unrecoverable
> > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu)
> > if (kvm_usage_count) {
> > WARN_ON_ONCE(atomic_read(&hardware_enable_failed));
> >
> > + local_irq_save(flags);
> > hardware_enable_nolock(NULL);
> > + local_irq_restore(flags);
>
> Sort of. What I was saying is that in this v1, the compatibility checks that are
> done during harware enabling are initiated from vendor code, i.e. VMX and SVM call
> {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that
> are handled in common code:
>
> if (__cr4_reserved_bits(cpu_has, c) !=
> __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> return -EIO;
>
> are skipped. And if that's fixed, then the above hardware_enable_nolock() call
> will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled
> once the KVM hotplug hook is moved to the ONLINE section.
Oh I see. So you still want the kvm_x86_ops->check_processor_compatibility(),
in order to avoid duplicating the above code in SVM and VMX.
>
> As above, the simple "fix" would be to disable IRQs, but that's not actually
> necessary. The only requirement is that preemption is disabled so that the checks
> are done on the current CPU.
>
Probably even preemption is allowed, as long as the compatibility check is not
scheduled to another cpu.
> The "IRQs disabled" check was a deliberately
> agressive WARN that was added to guard against doing compatibility checks from
> the "wrong" location.
>
> E.g. this is what I ended up with for a changelog to drop the irqs_disabled()
> check and for the end code (though it's not tested yet...)
>
> Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are
> disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't
^
enabled.
> intended to be a requirement, e.g. disabling preemption is sufficient,
> the IRQ thing was purely an aggressive sanity check since the helper was
> only ever invoked via SMP function call.
>
>
> static int kvm_x86_check_processor_compatibility(void)
> {
> int cpu = smp_processor_id();
> struct cpuinfo_x86 *c = &cpu_data(cpu);
>
> /*
> * Compatibility checks are done when loading KVM and when enabling
> * hardware, e.g. during CPU hotplug, to ensure all online CPUs are
> * compatible, i.e. KVM should never perform a compatibility check on
> * an offline CPU.
> */
> WARN_ON(!cpu_online(cpu));
Looks good to me. Perhaps this also can be removed, though.
And IMHO the removing of WARN_ON(!irq_disabled()) should be folded to the patch
"[PATCH 37/44] KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section".
Because moving from STARTING section to ONLINE section changes the IRQ status
when the compatibility check is called.
>
> if (__cr4_reserved_bits(cpu_has, c) !=
> __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> return -EIO;
>
> return static_call(kvm_x86_check_processor_compatibility)();
> }
>
>
> int kvm_arch_hardware_enable(void)
> {
> struct kvm *kvm;
> struct kvm_vcpu *vcpu;
> unsigned long i;
> int ret;
> u64 local_tsc;
> u64 max_tsc = 0;
> 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;
>
>
> ....
> }
Powered by blists - more mailing lists