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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f7d3pbe4s52twxaddjwlpca3mlb6htxi3ozze7n2sv4d3cafn@o3cyq3tmjhbx>
Date: Wed, 18 Jun 2025 20:09:57 +0530
From: Naveen N Rao <naveen.rao@....com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>,
	Paolo Bonzini <pbonzini@...hat.com>, Joerg Roedel <joro@...tes.org>, "David
 Woodhouse" <dwmw2@...radead.org>, Lu Baolu <baolu.lu@...ux.intel.com>,
	<linux-arm-kernel@...ts.infradead.org>, <kvmarm@...ts.linux.dev>,
	<kvm@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, Sairaj Kodilkar <sarunkod@....com>, "Vasant
 Hegde" <vasant.hegde@....com>, Maxim Levitsky <mlevitsk@...hat.com>, "Joao
 Martins" <joao.m.martins@...cle.com>, Francesco Lavra
	<francescolavra.fl@...il.com>, David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH v3 13/62] KVM: SVM: Drop redundant check in AVIC code on
 ID during vCPU creation

On Tue, Jun 17, 2025 at 09:33:20AM -0700, Sean Christopherson wrote:
> 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?

Not at all, please feel free to add a patch (or not, given that this 
will be a trivial change).

> 
> > Otherwise, for this patch:
> > Reviewed-by: Naveen N Rao (AMD) <naveen@...nel.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