On Sat, Aug 13, 2016 at 02:43:03PM +0200, Radim Krčmář wrote: > 2016-08-12 15:37-0300, Eduardo Habkost: > > Now I have another question: are features that require the > > in-kernel irqchip supposed to be present in GET_SUPPORTED_CPUID? > > I don't think so. Simply removing X2APIC and PV_UNHALT would disable > them on old userspaces, though, which would probably cause more bugs. Yes, we can't remove it now. > > (The blunder doesn't seem to be bad enough for a new capability or > interface and a deprecation protocol on these features.) Agreed. After we notice it and work around it in userspace, it's now part of the interface and not a problem anymore. > > > We have examples of both cases in KVM: > > > > * TSC_DEADLINE_TIMER is _not_ present in GET_SUPPORTED_CPUID, > > and is reported through KVM_CAP_TSC_DEADLINE_TIMER. > > * X2APIC is present in GET_SUPPORTED_CPUID, but the bit > > makes sense only if the in-kernel irqchip is used. > > * KVM_PV_UNHALT is present in GET_SUPPORTED_CPUID, but > > requires the in-kernel irqchip to work. > > Well ... no excuses for that. > > > Should userspace expect more cases like x2apic and kvm_pv_unhalt > > in the future? > > At least one userspace (QEMU) doesn't filter unknown features from > GET_SUPPORTED_CPUID, so KVM cannot plan to pass conditionally buggy > features. I will add to my TODO-list a reminder to write a Documentation update for GET_SUPPORTED_CPUID mentioning those details (unless somebody else volunteers to do it). > KVM would need to define a new interface to handle these issues, so I > think that userspace can ignore unknown KVM bugs. What can we do to avoid introducing those bugs in the future? > > I would like to return -EINVAL from KVM_SET_CPUID2 if userspace > requested a new CPUID feature that cannot work in given situation. > > Another way would be to disable buggy features in KVM_SET_CPUID2, which > would require userspace to call KVM_GET_CPUID2 afterwards to learn what > the guest is actually using. I'd prefer to get -EINVAL. QEMU needs to be 100% aware of the resulting CPUID bits, so if we end up trying to enable something that will never work, it's already too late to silently clear CPUID bits. This shouldn't make any difference for existing QEMU code, because it already filters out all CPUID feature flags based on GET_SUPPORTED_CPUID (+ the extra fixups in QEMU's kvm_arch_get_supported_cpuid()). But: note that returning -EINVAL might break very old QEMU versions. Maybe we really want them to break (because they have broken CPUID logic), but I am not sure. > > I have patches that implement the latter for X2APIC and PV_UNHALT, but > I'm not sure if it's better than leaving the bug unfixed, because QEMU > doesn't use KVM_GET_CPUID2 and migration to older KVM would change > CPUID, which is a very subtle bug. > What do you think? Implementing those checks basically mean moving the existing kvm_arch_get_supported_cpuid() logic inside KVM. If we do that, can we get an interface that will allow QEMU to just query KVM instead of duplicating that logic? Making KVM_SET_CPUID2 clear those bits and/or return -EINVAL would probably be enough as an interface, as long as QEMU has a way to know in advance if it needs to clear CPUID bits based on the legacy kvm_arch_get_supported_cpuid() code (for older kernels), or if it can just give everything to KVM_SET_CPUID2, and check for unsupported/cleared flags later (for newer kernels). -- Eduardo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
