On Fri, Nov 4, 2016 at 5:09 PM, David Matlack <dmatl...@google.com> wrote: > On Fri, Nov 4, 2016 at 2:57 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: >> >> On 04/11/2016 21:34, David Matlack wrote: >>> On Mon, Oct 31, 2016 at 6:37 PM, Kyle Huey <m...@kylehuey.com> wrote: >>>> + case MSR_PLATFORM_INFO: >>>> + /* cpuid faulting is supported */ >>>> + msr_info->data = PLATINFO_CPUID_FAULT; >>>> + break; >>> >>> This could break save/restore, if for example, a VM is migrated to a >>> version of KVM without MSR_PLATFORM_INFO support. I think the way to >>> handle this is to make MSR_PLATFORM_INFO writeable (but only from >>> userspace) so that hypervisors can defend themselves (by setting this >>> MSR to 0). >> >> Right---and with my QEMU hat on, this feature will have to be enabled >> manually on the command line because of the way QEMU supports running >> with old kernels. :( This however does not impact the KVM patch. >> >> We may decide that, because CPUID faulting doesn't have a CPUID bit and >> is relatively a "fringe" feature, we are okay if the kernel enables this >> unconditionally and then userspace can arrange to block migration (in >> QEMU this would use a subsection). David, Eduardo, opinions? > > Sounds reasonable. Accurate CPU virtualization might be another reason > to disable this feature from userspace. > > My worry is a kernel rollback, where migrating to an older kernel > version is unavoidable. > >> >>> >>>> + case MSR_MISC_FEATURES_ENABLES: >>>> + msr_info->data = 0; >>>> + if (vcpu->arch.cpuid_fault) >>>> + msr_info->data |= CPUID_FAULT_ENABLE; >>>> + break; >>> >>> MSR_MISC_FEATURES_ENABLES should be added to emulated_msrs[] so that >>> the hypervisor will maintain the value of CPUID_FAULT_ENABLE across a >>> save/restore. >> >> This is definitely necessary. Thanks David. >> >> Paolo >> >>>> default: >>>> if (kvm_pmu_is_valid_msr(vcpu, msr_info->index)) >>>> return kvm_pmu_get_msr(vcpu, msr_info->index, >>>> &msr_info->data); >>>> if (!ignore_msrs) { >>>> vcpu_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", >>>> msr_info->index); >>>> return 1; >>>> } else { >>>> vcpu_unimpl(vcpu, "ignored rdmsr: 0x%x\n", >>>> msr_info->index); >>>> @@ -7493,16 +7507,18 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool >>>> init_event) >>>> kvm_update_dr0123(vcpu); >>>> vcpu->arch.dr6 = DR6_INIT; >>>> kvm_update_dr6(vcpu); >>>> vcpu->arch.dr7 = DR7_FIXED_1; >>>> kvm_update_dr7(vcpu); >>>> >>>> vcpu->arch.cr2 = 0; >>>> >>>> + vcpu->arch.cpuid_fault = false; >>>> + >>>> kvm_make_request(KVM_REQ_EVENT, vcpu); >>>> vcpu->arch.apf.msr_val = 0; >>>> vcpu->arch.st.msr_val = 0; >>>> >>>> kvmclock_reset(vcpu); >>>> >>>> kvm_clear_async_pf_completion_queue(vcpu); >>>> kvm_async_pf_hash_reset(vcpu); >>>> -- >>>> 2.10.2 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe kvm" in >>>> the body of a message to majord...@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>
Thanks for the feedback. A new patch series is coming up. - Kyle