Re: [PATCH v3 13/62] KVM: SVM: Drop redundant check in AVIC code on ID during vCPU creation
From: Naveen N Rao <hidden>
Date: 2025-06-18 14:46:47
Also in:
kvm, kvmarm, linux-iommu, lkml
On Tue, Jun 17, 2025 at 09:33:20AM -0700, Sean Christopherson wrote:
On Tue, Jun 17, 2025, Naveen N Rao wrote:quoted
On Wed, Jun 11, 2025 at 03:45:16PM -0700, Sean Christopherson wrote:quoted
static int avic_init_backing_page(struct kvm_vcpu *vcpu) { - u64 *entry, new_entry; - int id = vcpu->vcpu_id; + struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm); struct vcpu_svm *svm = to_svm(vcpu); + u32 id = vcpu->vcpu_id; + u64 *table, new_entry; /* * Inhibit AVIC if the vCPU ID is bigger than what is supported by AVIC@@ -291,6 +277,9 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu) return 0; } + BUILD_BUG_ON((AVIC_MAX_PHYSICAL_ID + 1) * sizeof(*table) > PAGE_SIZE || + (X2AVIC_MAX_PHYSICAL_ID + 1) * sizeof(*table) > PAGE_SIZE);^^^^^^^^^^^^^^ Renaming new_entry to just 'entry' and using sizeof(entry) makes this more readable for me.Good call, though I think it makes sense to do that on top so as to minimize the churn in this patch. I'll post a patch, unless you want the honors?
Not at all, please feel free to add a patch (or not, given that this will be a trivial change).
quoted
Otherwise, for this patch: Reviewed-by: Naveen N Rao (AMD) <naveen@kernel.org> As an aside, there are a few static asserts to validate some of the related macros. Can this also be a static_assert(), or is there is reason to prefer BUILD_BUG_ON()?For this particular assertion, static_assert() would be fine. That said, BUILD_BUG_ON() is slightly preferred in this context. The advantage of BUILD_BUG_ON() is that it works so long as the condition is compile-time constant, whereas static_assert() requires the condition to an integer constant expression. E.g. BUILD_BUG_ON() can be used so long as the condition is eventually resolved to a constant, whereas static_assert() has stricter requirements. E.g. the fls64() assert below is fully resolved at compile time, but isn't a purely constant expression, i.e. that one *needs* to be BUILD_BUG_ON(). -- arch/x86/kvm/svm/avic.c: In function ‘avic_init_backing_page’: arch/x86/kvm/svm/avic.c:293:45: error: expression in static assertion is not constant 293 | static_assert(__PHYSICAL_MASK_SHIFT <= include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ arch/x86/kvm/svm/avic.c:293:9: note: in expansion of macro ‘static_assert’ 293 | static_assert(__PHYSICAL_MASK_SHIFT <= | ^~~~~~~~~~~~~ make[5]: *** [scripts/Makefile.build:203: arch/x86/kvm/svm/avic.o] Error 1 -- The downside of BUILD_BUG_ON() is that it can't be used at global scope, i.e. needs to be called from a function. As a result, when adding an assertion in a function, using BUILD_BUG_ON() is slightly preferred, because it's less likely to break in the future. E.g. if X2AVIC_MAX_PHYSICAL_ID were changed to something that is a compile-time constant, but for whatever reason isn't a pure integer constant.
Understood, thanks for the explanation. - Naveen