| Message ID | 20211227081515.2088920-6-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve KVM's interaction with CPU hotplug | expand |
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 >
> From: Sean Christopherson <seanjc@google.com> > Sent: Tuesday, January 11, 2022 7:00 AM > > 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. Looks the WARN_ON() was added by you.
On Tue, Jan 11, 2022, Tian, Kevin wrote: > > From: Sean Christopherson <seanjc@google.com> > > Sent: Tuesday, January 11, 2022 7:00 AM > > > > 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. > > Looks the WARN_ON() was added by you.
On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote: >On Tue, Jan 11, 2022, Tian, Kevin wrote: >> > From: Sean Christopherson <seanjc@google.com> >> > Sent: Tuesday, January 11, 2022 7:00 AM >> > >> > 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. >> >> Looks the WARN_ON() was added by you.
On Wed, Jan 12, 2022, Chao Gao wrote: > On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote: > >On Tue, Jan 11, 2022, Tian, Kevin wrote: > >> > From: Sean Christopherson <seanjc@google.com> > >> > Sent: Tuesday, January 11, 2022 7:00 AM > >> > > >> > 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. > >> > >> Looks the WARN_ON() was added by you.
On Wed, Jan 12, 2022 at 05:35:12PM +0000, Sean Christopherson wrote: >On Wed, Jan 12, 2022, Chao Gao wrote: >> On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote: >> >On Tue, Jan 11, 2022, Tian, Kevin wrote: >> >> > From: Sean Christopherson <seanjc@google.com> >> >> > Sent: Tuesday, January 11, 2022 7:00 AM >> >> > >> >> > 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. >> >> >> >> Looks the WARN_ON() was added by you.
On Mon, Jan 17, 2022 at 09:35:04PM +0800, Chao Gao wrote: >On Wed, Jan 12, 2022 at 05:35:12PM +0000, Sean Christopherson wrote: >>On Wed, Jan 12, 2022, Chao Gao wrote: >>> On Tue, Jan 11, 2022 at 07:48:39PM +0000, Sean Christopherson wrote: >>> >On Tue, Jan 11, 2022, Tian, Kevin wrote: >>> >> > From: Sean Christopherson <seanjc@google.com> >>> >> > Sent: Tuesday, January 11, 2022 7:00 AM >>> >> > >>> >> > 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. >>> >> >>> >> Looks the WARN_ON() was added by you.
On Mon, Jan 17, 2022, Chao Gao wrote: > On Mon, Jan 17, 2022 at 09:35:04PM +0800, Chao Gao wrote: > >OK. How about: > > > > /* > > * Compatibility checks are done when loading KVM or in KVM's CPU > > * hotplug callback. It ensures all online CPUs are compatible before > > * running any vCPUs. For other cases, compatibility checks are > > * unnecessary or even problematic. Try to detect improper usages here. > > */ > > WARN_ON(!irqs_disabled() && !cpu_active(smp_processor_id())); > > Sorry. It should be: > WARN_ON(!irqs_disabled() && cpu_active(smp_processor_id())); Nice! That's exactly what I was hoping we could do.
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;
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. Signed-off-by: Chao Gao <chao.gao@intel.com> --- arch/x86/kvm/x86.c | 2 -- 1 file changed, 2 deletions(-)