[PATCH] KVM: x86: use assign_bit() where applicable

[PATCH] KVM: x86: use assign_bit() where applicable

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Convert open-coded if/else with set_bit/clear_bit and their
non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
API.

Done with Coccinelle semantic patch:
    // set_bit -> clear_bit => assign_bit

    @@
    expression cond, bit, addr;
    @@

    -if (cond)
    -        set_bit(bit, addr);
    -else
    -        clear_bit(bit, addr);
    +assign_bit(bit, addr, cond);

    // clear_bit -> set_bit => assign_bit

    @@
    expression cond, bit, addr;
    @@

    -if (cond)
    -        clear_bit(bit, addr);
    -else
    -        set_bit(bit, addr);
    +assign_bit(bit, addr, !cond);

    // __set_bit -> __clear_bit => __assign_bit

    @@
    expression cond, bit, addr;
    @@

    -if (cond)
    -        __set_bit(bit, addr);
    -else
    -        __clear_bit(bit, addr);
    +__assign_bit(bit, addr, cond);

    // __clear_bit -> __set_bit => __assign_bit

    @@
    expression cond, bit, addr;
    @@

    -if (cond)
    -        __clear_bit(bit, addr);
    -else
    -        __set_bit(bit, addr);
    +__assign_bit(bit, addr, !cond);

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/x86/kvm/hyperv.c  | 12 ++++--------
 arch/x86/kvm/svm/pmu.c |  6 ++----
 arch/x86/kvm/x86.c     | 11 +++--------
 3 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 8d2669d8ef34..c131d9a3c550 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -114,17 +114,13 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
 		return;
 
-	if (synic_has_vector_connected(synic, vector))
-		__set_bit(vector, synic->vec_bitmap);
-	else
-		__clear_bit(vector, synic->vec_bitmap);
+	__assign_bit(vector, synic->vec_bitmap,
+		     synic_has_vector_connected(synic, vector));
 
 	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
 
-	if (synic_has_vector_auto_eoi(synic, vector))
-		__set_bit(vector, synic->auto_eoi_bitmap);
-	else
-		__clear_bit(vector, synic->auto_eoi_bitmap);
+	__assign_bit(vector, synic->auto_eoi_bitmap,
+		     synic_has_vector_auto_eoi(synic, vector));
 
 	auto_eoi_new = !bitmap_empty(synic->auto_eoi_bitmap, 256);
 
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index c18286545a7a..4c13a6345277 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -169,10 +169,8 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			pmc->eventsel_hw = (data & ~AMD64_EVENTSEL_HOSTONLY) |
 					   AMD64_EVENTSEL_GUESTONLY;
 
-			if (data & AMD64_EVENTSEL_HOST_GUEST_MASK)
-				__set_bit(pmc->idx, pmu->pmc_has_mode_specific_enables);
-			else
-				__clear_bit(pmc->idx, pmu->pmc_has_mode_specific_enables);
+			__assign_bit(pmc->idx, pmu->pmc_has_mode_specific_enables,
+				     data & AMD64_EVENTSEL_HOST_GUEST_MASK);
 
 			kvm_pmu_request_counter_reprogram(pmc);
 		}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index af3ceee714c9..a33ef4ab4e60 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3074,10 +3074,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 #endif
 
 		if (lapic_in_kernel(vcpu)) {
-			if (events->smi.latched_init)
-				set_bit(KVM_APIC_INIT, &vcpu->arch.apic->pending_events);
-			else
-				clear_bit(KVM_APIC_INIT, &vcpu->arch.apic->pending_events);
+			assign_bit(KVM_APIC_INIT, &vcpu->arch.apic->pending_events,
+				   events->smi.latched_init);
 		}
 	}
 
@@ -7224,10 +7222,7 @@ static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
 
 	BUILD_BUG_ON(ARRAY_SIZE(apicv_inhibits) != NR_APICV_INHIBIT_REASONS);
 
-	if (set)
-		__set_bit(reason, inhibits);
-	else
-		__clear_bit(reason, inhibits);
+	__assign_bit(reason, inhibits, set);
 
 	trace_kvm_apicv_inhibit_changed(reason, set, *inhibits);
 }
-- 
2.51.0

Re: [PATCH] KVM: x86: use assign_bit() where applicable

[Sean Christopherson]

On Sun, Sep 20, 2026, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Convert open-coded if/else with set_bit/clear_bit and their
> non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
> API.

...

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/x86/kvm/hyperv.c  | 12 ++++--------
>  arch/x86/kvm/svm/pmu.c |  6 ++----
>  arch/x86/kvm/x86.c     | 11 +++--------
>  3 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 8d2669d8ef34..c131d9a3c550 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -114,17 +114,13 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
>  		return;
>  
> -	if (synic_has_vector_connected(synic, vector))
> -		__set_bit(vector, synic->vec_bitmap);
> -	else
> -		__clear_bit(vector, synic->vec_bitmap);
> +	__assign_bit(vector, synic->vec_bitmap,
> +		     synic_has_vector_connected(synic, vector));
>  
>  	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
>  
> -	if (synic_has_vector_auto_eoi(synic, vector))
> -		__set_bit(vector, synic->auto_eoi_bitmap);
> -	else
> -		__clear_bit(vector, synic->auto_eoi_bitmap);
> +	__assign_bit(vector, synic->auto_eoi_bitmap,
> +		     synic_has_vector_auto_eoi(synic, vector));

Am I the only one that finds the assign_bit() code signficantly harder to follow?
Maybe it's just that I haven't seen assign_bit() much, but I've come back to this
patch several times, and I've had the same reaction every time.  IMO, this is a
solution looking for a problem.

Re: [PATCH] KVM: x86: use assign_bit() where applicable

[Peng Fan]

Hi Sean,

On Mon, Sep 28, 2026 at 05:58:57PM -0700, Sean Christopherson wrote:
>On Sun, Sep 20, 2026, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> Convert open-coded if/else with set_bit/clear_bit and their
>> non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
>> API.
>
>...
>
...
>>  
>> -	if (synic_has_vector_auto_eoi(synic, vector))
>> -		__set_bit(vector, synic->auto_eoi_bitmap);
>> -	else
>> -		__clear_bit(vector, synic->auto_eoi_bitmap);
>> +	__assign_bit(vector, synic->auto_eoi_bitmap,
>> +		     synic_has_vector_auto_eoi(synic, vector));
>
>Am I the only one that finds the assign_bit() code signficantly harder to follow?

No :) S390 maintainers also not like this.

>Maybe it's just that I haven't seen assign_bit() much, but I've come back to this
>patch several times, and I've had the same reaction every time.  IMO, this is a
>solution looking for a problem.
>

It might be easy to read if the test condition is just a simple value, not
a function call.

Free to drop this patch.

Thanks
Peng

>

Re: [PATCH] KVM: x86: use assign_bit() where applicable

[David Laight]

On Mon, 28 Sep 2026 17:58:57 -0700
Sean Christopherson <seanjc@google.com> wrote:

> On Sun, Sep 20, 2026, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > Convert open-coded if/else with set_bit/clear_bit and their
> > non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
> > API.  
> 
> ...
> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/x86/kvm/hyperv.c  | 12 ++++--------
> >  arch/x86/kvm/svm/pmu.c |  6 ++----
> >  arch/x86/kvm/x86.c     | 11 +++--------
> >  3 files changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 8d2669d8ef34..c131d9a3c550 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -114,17 +114,13 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >  		return;
> >  
> > -	if (synic_has_vector_connected(synic, vector))
> > -		__set_bit(vector, synic->vec_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->vec_bitmap);
> > +	__assign_bit(vector, synic->vec_bitmap,
> > +		     synic_has_vector_connected(synic, vector));
> >  
> >  	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
> >  
> > -	if (synic_has_vector_auto_eoi(synic, vector))
> > -		__set_bit(vector, synic->auto_eoi_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->auto_eoi_bitmap);
> > +	__assign_bit(vector, synic->auto_eoi_bitmap,
> > +		     synic_has_vector_auto_eoi(synic, vector));  
> 
> Am I the only one that finds the assign_bit() code signficantly harder to follow?
> Maybe it's just that I haven't seen assign_bit() much, but I've come back to this
> patch several times, and I've had the same reaction every time.  IMO, this is a
> solution looking for a problem.
> 

The same is pretty much true of __set_bit() and even BIT().
What is wrong with:
	if (synic_has_vector_auto_eoi(synic, vector))
		vector |= 1u << synic->auto_eoi_bitmap;
	else
		vector &= ~(1u << synic->auto_eoi_bitmap);
'Does what is sway on the tin'.

David

Re: [PATCH] KVM: x86: use assign_bit() where applicable

[Alison Schofield]

On Mon, Sep 28, 2026 at 05:58:57PM -0700, Sean Christopherson wrote:
> On Sun, Sep 20, 2026, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > Convert open-coded if/else with set_bit/clear_bit and their
> > non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
> > API.
> 
> ...
> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  arch/x86/kvm/hyperv.c  | 12 ++++--------
> >  arch/x86/kvm/svm/pmu.c |  6 ++----
> >  arch/x86/kvm/x86.c     | 11 +++--------
> >  3 files changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 8d2669d8ef34..c131d9a3c550 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -114,17 +114,13 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >  		return;
> >  
> > -	if (synic_has_vector_connected(synic, vector))
> > -		__set_bit(vector, synic->vec_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->vec_bitmap);
> > +	__assign_bit(vector, synic->vec_bitmap,
> > +		     synic_has_vector_connected(synic, vector));
> >  
> >  	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
> >  
> > -	if (synic_has_vector_auto_eoi(synic, vector))
> > -		__set_bit(vector, synic->auto_eoi_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->auto_eoi_bitmap);
> > +	__assign_bit(vector, synic->auto_eoi_bitmap,
> > +		     synic_has_vector_auto_eoi(synic, vector));
> 
> Am I the only one that finds the assign_bit() code signficantly harder to follow?
> Maybe it's just that I haven't seen assign_bit() much, but I've come back to this
> patch several times, and I've had the same reaction every time.  IMO, this is a
> solution looking for a problem.


A place for me to pile on, hopefully constructively. :)

I looked at the DAX patch doing same initially and set it aside. After a few
review tags came in, I took a closer look and decided to NAK it.

A few things contributed to that decision.

These patches were sent individually, all with "where applicable" in the subject,
but without explaining why the conversion was appropriate in each case. That leaves
reviewers to establish the justification for the change, rather than evaluate the
justification provided by the author.

I would have preferred to see these as a series, so reviewers could see the scope of
the proposed conversions and discuss the approach as a whole.

Looking through the history of assign_bit(), I found that it was introduced for a
specific use case, then later moved into bitops.h when someone else had a need for it.
I didn't find any indication that the existing if/else pattern was considered
problematic or that there was an intent to replace it more broadly.

In the DAX case, the conversion /drivers/dax/super.c` didn't appear to simplify the code
or make the intent any clearer.

I'm not opposed to using `assign_bit()` where it improves the code, but I don't think the
existence of a helper is, by itself, sufficient justification for converting existing code.
I'd rather see these conversions motivated by a concrete improvement than by the
opportunity to replace an open-coded pattern.

-- Alison