Re: [PATCH v3 13/62] KVM: SVM: Drop redundant check in AVIC code on ID during vCPU creation

Sean Christopherson
2025-06-17T16:33:22Z

On Tue, Jun 17, 2025, Naveen N Rao wrote:
> On Wed, Jun 11, 2025 at 03:45:16PM -0700, Sean Christopherson wrote:
> > 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?

> 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.