Re: [PATCH v2 17/17] kvm: Drop dependencies on very old capabilities

Jan Kiszka
2011-01-03T17:24:24Z

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

Am 03.01.2011 18:01, Avi Kivity wrote:
> On 01/03/2011 06:54 PM, Jan Kiszka wrote:
>> Am 03.01.2011 17:08, Avi Kivity wrote:
>> > On 01/03/2011 10:33 AM, Jan Kiszka wrote:
>> >> From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >> COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY,
>> PV_MMU -
>> >> all these caps predate features on which we already depend at build
>> >> time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
>> >> already test& fail is a more recent feature is missing.
>> >
>> > No. Each test documents a dependency of qemu on a kvm feature. Even
>> > though something like SYNC_MMU is unlikely to go away, as long as we
>> > depend on it, we require the feature.
>> >
>>
>> Then at least move all those KVM_CAPs we need at build time into
>> configure.
>
> Need a run time check as well (build on new kernel, run on old kernel,
> or run on even newer kernel that lost a feature).
>
>> I really see no value in keeping ugly conditional code
>> around, A) because those paths won't be tested and B) none of the CAPs
>> touched here are to pass away without a replacement that will require
>> user space adaption anyway.
>
> I'm fine with a series of checks during init time with no fallback. I'm
> not fine with just dropping those away. Reducing code size is great,
> but not at the cost of undiagnosed runtime failures.

My worry was not code size but untested code. And looking at the
EXT_CPUID case again, this is what would happen:
kvm_x86_get_supported_cpuid will return -1 if we lack EXT_CPUID, but
that value is interpreted by the callers as "all requested features
available" - likely not that helpful in all cases. This also affects
qemu-kvm.

I will add generic and per-arch check sections at build and runtime for
CAPs we don't want to miss.

Jan

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]