On 07.06.24 02:12, Sean Christopherson wrote: > On Thu, Jun 06, 2024, Mathias Krause wrote: >> On 06.06.24 16:55, Sean Christopherson wrote: >>> On Thu, Jun 06, 2024, Mathias Krause wrote: >>>> [snip] >>> If @id is checked as a 32-bit value, and we somehow screw up and >>> define KVM_MAX_VCPU_IDS to be a 64-bit value, clang will rightly complain that >>> the check is useless, e.g. given "#define KVM_MAX_VCPU_ID_TEST BIT(32)" >>> >>> arch/x86/kvm/x86.c:12171:9: error: result of comparison of constant 4294967296 with >>> expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare] >>> if (id > KVM_MAX_VCPU_ID_TEST) >>> ~~ ^ ~~~~~~~~~~~~~~~~~~~~ >>> 1 error generated. >> ^^^^^^^^^^^^^^^^^^ >> Perfect! So this breaks the build. How much better can we prevent this >> bug from going unnoticed? > > Yes, but iff @id is a 32-bit value, i.e. this trick doesn't work on 64-bit kernels > if the comparison is done with @id is an unsigned long (and I'm hoping that we > can kill off 32-bit KVM support in the not too distant future). Fortunately, the BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX) will still catch it. > >> ¹ IMHO, using 'int' for vcpu_id is actually *very* *wrong*, as it's used >> as an index in certain constructs and having a signed type doesn't feel >> right at all. But that's just a side matter, as, according to the checks >> on the ioctl() path, the actual value of vcpu_id can never be negative. >> So lets not distract. > > Hmm, I 100% agree that it's horrific, but I disagree that it's a distraction. > I think we should fix that at the same time as we harden the trunction stuff, so > that it's (hopefully) clear what KVM _intends_ to support, as opposed to what the > code happens to allow. I looked into it a little closer and it's even a bigger mess than what I initially thought. 'vcpu_id' values get passed around as int, unsigned int, u32, u16 even (S390) and unsigned long in the various architectures implementing KVM support. A change touching all of these clearly needs quite some coordination among multiple maintainers and testing to rule out issues caused by changing the sign of the underlying type -- even if it'll be just new warnings popping up. I don't even have cross-compilers for all of these, less so setups to actually do some tests. So, unfortunately, I'm not up to do that change. > > In the end, I'm ok relying on the KVM_MAX_VCPU_IDS check, so long as there's > a BUILD_BUG_ON() and a comment. Ok, will send a v2 series soon, covering KVM_SET_BOOT_CPU_ID, too. Thanks, Mathias
