KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

[PATCH-next] KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

Posted by Advait Dhamorikar 1 week, 4 days ago

An unsigned value can never be negative,
so this test will always evaluate the same way.
`_as_id` a u16 is compared to 0.

Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 4508d868f1cd..b4e7b6a264d6 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -153,7 +153,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
 	for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid);		\
 	     ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;		\
 	     _root = tdp_mmu_next_root(_kvm, _root, _only_valid))		\
-		if (_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) {	\
+		if (kvm_mmu_page_as_id(_root) != _as_id) {	\
 		} else
 
 #define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id)	\
-- 
2.34.1

Re: [PATCH-next] KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

Posted by Sean Christopherson 1 week, 4 days ago

On Tue, Nov 12, 2024, Advait Dhamorikar wrote:
> An unsigned value can never be negative,
> so this test will always evaluate the same way.
> `_as_id` a u16 is compared to 0.

Please wrap changelogs at ~75 characters.

> Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 4508d868f1cd..b4e7b6a264d6 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -153,7 +153,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
>  	for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid);		\
>  	     ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;		\
>  	     _root = tdp_mmu_next_root(_kvm, _root, _only_valid))		\
> -		if (_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) {	\
> +		if (kvm_mmu_page_as_id(_root) != _as_id) {	\

NAK, the comparison is necessary as kvm_tdp_mmu_zap_leafs() deliberately invokes
for_each_valid_tdp_mmu_root_yield_safe() => __for_each_tdp_mmu_root_yield_safe()
with -1 to iterate over all address spaces.

And I don't want to drop the check for __for_each_tdp_mmu_root(), even though
there aren't any _current_ users that pass -1, as I want to keep the iterators
symmetrical.

Re: [PATCH-next] KVM: x86/tdp_mmu: Fix redundant u16 compared to 0

Posted by Advait Dhamorikar 1 week, 4 days ago

Hello Sean,

> NAK, the comparison is necessary as kvm_tdp_mmu_zap_leafs() deliberately invokes
> for_each_valid_tdp_mmu_root_yi
> eld_safe() => __for_each_tdp_mmu_root_yield_safe()
>with -1 to iterate over all address spaces.

> And I don't want to drop the check for __for_each_tdp_mmu_root(), even though
> there aren't any _current_ users that pass -1, as I want to keep the iterators
> symmetrical.

Understood, thanks for the feedback.

Best regards,
Advait

On Tue, 12 Nov 2024 at 00:26, Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 12, 2024, Advait Dhamorikar wrote:
> > An unsigned value can never be negative,
> > so this test will always evaluate the same way.
> > `_as_id` a u16 is compared to 0.
>
> Please wrap changelogs at ~75 characters.
>
> > Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
> > ---
> >  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 4508d868f1cd..b4e7b6a264d6 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -153,7 +153,7 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
> >       for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid);                \
> >            ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;              \
> >            _root = tdp_mmu_next_root(_kvm, _root, _only_valid))               \
> > -             if (_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) {       \
> > +             if (kvm_mmu_page_as_id(_root) != _as_id) {      \
>
> NAK, the comparison is necessary as kvm_tdp_mmu_zap_leafs() deliberately invokes
> for_each_valid_tdp_mmu_root_yield_safe() => __for_each_tdp_mmu_root_yield_safe()
> with -1 to iterate over all address spaces.
>
> And I don't want to drop the check for __for_each_tdp_mmu_root(), even though
> there aren't any _current_ users that pass -1, as I want to keep the iterators
> symmetrical.