| Message ID | 20230316154554.1237-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add the missing stub function for kvm_dirty_ring_check_request() | expand |
On Thu, Mar 16, 2023, Shameer Kolothum wrote:
> The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing.
No stub is needed. kvm_dirty_ring_check_request() isn't called from common code,
and should not (and isn't unless I'm missing something) be called from arch code
unless CONFIG_HAVE_KVM_DIRTY_RING=y.
x86 and arm64 are the only users, and they both select HAVE_KVM_DIRTY_RING
unconditionally when KVM is enabled.
> -----Original Message----- > From: Sean Christopherson [mailto:seanjc@google.com] > Sent: 16 March 2023 17:02 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; gshan@redhat.com; > maz@kernel.org > Subject: Re: [PATCH] KVM: Add the missing stub function for > kvm_dirty_ring_check_request() > > On Thu, Mar 16, 2023, Shameer Kolothum wrote: > > The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing. > > No stub is needed. kvm_dirty_ring_check_request() isn't called from > common code, > and should not (and isn't unless I'm missing something) be called from arch > code > unless CONFIG_HAVE_KVM_DIRTY_RING=y. > > x86 and arm64 are the only users, and they both select > HAVE_KVM_DIRTY_RING > unconditionally when KVM is enabled. Yes, it is at present not called from anywhere other than x86 and arm64. But I still think since it is a common helper, better to have a stub. Thanks, Shameer
On Thu, Mar 16, 2023, Shameerali Kolothum Thodi wrote: > > From: Sean Christopherson [mailto:seanjc@google.com] > > On Thu, Mar 16, 2023, Shameer Kolothum wrote: > > > The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing. > > > > No stub is needed. kvm_dirty_ring_check_request() isn't called from > > common code, > > and should not (and isn't unless I'm missing something) be called from arch > > code > > unless CONFIG_HAVE_KVM_DIRTY_RING=y. > > > > x86 and arm64 are the only users, and they both select > > HAVE_KVM_DIRTY_RING > > unconditionally when KVM is enabled. > > Yes, it is at present not called from anywhere other than x86 and arm64. > But I still think since it is a common helper, better to have a stub. Why? It buys us nothing other than dead code, and even worse it could let a bug that would otherwise be caught during build time escape to run time.
> -----Original Message----- > From: Sean Christopherson [mailto:seanjc@google.com] > Sent: 16 March 2023 19:57 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; gshan@redhat.com; > maz@kernel.org > Subject: Re: [PATCH] KVM: Add the missing stub function for > kvm_dirty_ring_check_request() > > On Thu, Mar 16, 2023, Shameerali Kolothum Thodi wrote: > > > From: Sean Christopherson [mailto:seanjc@google.com] On Thu, Mar 16, > > > 2023, Shameer Kolothum wrote: > > > > The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing. > > > > > > No stub is needed. kvm_dirty_ring_check_request() isn't called from > > > common code, and should not (and isn't unless I'm missing something) > > > be called from arch code unless CONFIG_HAVE_KVM_DIRTY_RING=y. > > > > > > x86 and arm64 are the only users, and they both select > > > HAVE_KVM_DIRTY_RING unconditionally when KVM is enabled. > > > > Yes, it is at present not called from anywhere other than x86 and arm64. > > But I still think since it is a common helper, better to have a stub. > > Why? It buys us nothing other than dead code, and even worse it could let > a bug that would otherwise be caught during build time escape to run time. Agree, it buys nothing now:) It just came up while I was playing with a custom build without HAVE_KVM_DIRTY_RING. Since all other functions there has a stub just thought it would make it easier for future common usage. We could very well leave it till that comes up as well. Thanks, Shameer
On Thu, Mar 16, 2023, Shameerali Kolothum Thodi wrote: > > From: Sean Christopherson [mailto:seanjc@google.com] > > On Thu, Mar 16, 2023, Shameerali Kolothum Thodi wrote: > > > > From: Sean Christopherson [mailto:seanjc@google.com] On Thu, Mar 16, > > > > 2023, Shameer Kolothum wrote: > > > > > The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing. > > > > > > > > No stub is needed. kvm_dirty_ring_check_request() isn't called from > > > > common code, and should not (and isn't unless I'm missing something) > > > > be called from arch code unless CONFIG_HAVE_KVM_DIRTY_RING=y. > > > > > > > > x86 and arm64 are the only users, and they both select > > > > HAVE_KVM_DIRTY_RING unconditionally when KVM is enabled. > > > > > > Yes, it is at present not called from anywhere other than x86 and arm64. > > > But I still think since it is a common helper, better to have a stub. > > > > Why? It buys us nothing other than dead code, and even worse it could let > > a bug that would otherwise be caught during build time escape to run time. > > Agree, it buys nothing now:) It just came up while I was playing with a custom > build without HAVE_KVM_DIRTY_RING. Since all other functions there has a stub > just thought it would make it easier for future common usage. We could very well > leave it till that comes up as well. Stubs are typically only added when they are strictly necessary. Providing a stub would make things "easier" in the sense that it could theoretically avoid a build error, but as above, in many cases we _want_ build errors when new code behaves in a way that diverges from what's expected/established.
diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h index 4862c98d80d3..a00301059da5 100644 --- a/include/linux/kvm_dirty_ring.h +++ b/include/linux/kvm_dirty_ring.h @@ -69,6 +69,11 @@ static inline void kvm_dirty_ring_free(struct kvm_dirty_ring *ring) { } +static inline bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu) +{ + return false; +} + #else /* CONFIG_HAVE_KVM_DIRTY_RING */ int kvm_cpu_dirty_log_size(void);
The stub for !CONFIG_HAVE_KVM_DIRTY_RING case is missing. Fixes: cf87ac739e48 ("KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- include/linux/kvm_dirty_ring.h | 5 +++++ 1 file changed, 5 insertions(+)