diff mbox

use bitmap ops on a bitmap instead of bit ops

Message ID 20090227132658.GA10118@redhat.com (mailing list archive)
State Accepted
Headers show

Commit Message

Gleb Natapov Feb. 27, 2009, 1:26 p.m. UTC 
On Fri, Feb 27, 2009 at 11:19:09AM +0800, Sheng Yang wrote:
> > -	*mask = 0;
> 
> I think the caller should not assume anything about the output, so let 
> function itself do the clean is better. (however, not in this way for "mask" 
> should be KVM_MAX_VCPUS long.)
> 
Ok. I'll leave it although all callers of the function zeroes bitmask anyway.
 
> And, I found some other things are not proper after looking back code, I would 
> send a patch to fix it (deliver_bitmask in kvm_get_intr_delivery_bitmask).
> 
> >  	if (dest_mode == 0) {	/* Physical mode. */
> >  		if (dest == 0xFF) {	/* Broadcast. */
> >  			for (i = 0; i < KVM_MAX_VCPUS; ++i)
> >  				if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
> > -					*mask |= 1 << i;
> > +					__set_bit(i, *mask);
> 
> Should be __set_bit(i, mask)?
> 
Oh. Here is updated patch:

Signed-off-by: Gleb Natapov <gleb@redhat.com>
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sheng Yang March 2, 2009, 7:12 a.m. UTC | #1 
On Friday 27 February 2009 21:26:58 Gleb Natapov wrote:
> On Fri, Feb 27, 2009 at 11:19:09AM +0800, Sheng Yang wrote:
> > > -	*mask = 0;
> >
> > I think the caller should not assume anything about the output, so let
> > function itself do the clean is better. (however, not in this way for
> > "mask" should be KVM_MAX_VCPUS long.)
>
> Ok. I'll leave it although all callers of the function zeroes bitmask
> anyway.
>
> > And, I found some other things are not proper after looking back code, I
> > would send a patch to fix it (deliver_bitmask in
> > kvm_get_intr_delivery_bitmask).
> >
> > >  	if (dest_mode == 0) {	/* Physical mode. */
> > >  		if (dest == 0xFF) {	/* Broadcast. */
> > >  			for (i = 0; i < KVM_MAX_VCPUS; ++i)
> > >  				if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
> > > -					*mask |= 1 << i;
> > > +					__set_bit(i, *mask);
> >
> > Should be __set_bit(i, mask)?
>
> Oh. Here is updated patch:

Thanks. :)

Acked-by: Sheng Yang <sheng@linux.intel.com>
Sheng Yang March 3, 2009, 1:28 a.m. UTC | #2 
On Monday 02 March 2009 15:12:00 Sheng Yang wrote:
> On Friday 27 February 2009 21:26:58 Gleb Natapov wrote:
> > On Fri, Feb 27, 2009 at 11:19:09AM +0800, Sheng Yang wrote:
> > > > -	*mask = 0;
> > >
> > > I think the caller should not assume anything about the output, so let
> > > function itself do the clean is better. (however, not in this way for
> > > "mask" should be KVM_MAX_VCPUS long.)
> >
> > Ok. I'll leave it although all callers of the function zeroes bitmask
> > anyway.
> >
> > > And, I found some other things are not proper after looking back code,
> > > I would send a patch to fix it (deliver_bitmask in
> > > kvm_get_intr_delivery_bitmask).
> > >
> > > >  	if (dest_mode == 0) {	/* Physical mode. */
> > > >  		if (dest == 0xFF) {	/* Broadcast. */
> > > >  			for (i = 0; i < KVM_MAX_VCPUS; ++i)
> > > >  				if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
> > > > -					*mask |= 1 << i;
> > > > +					__set_bit(i, *mask);
> > >
> > > Should be __set_bit(i, mask)?
> >
> > Oh. Here is updated patch:
>
> Thanks. :)
>
> Acked-by: Sheng Yang <sheng@linux.intel.com>

Just a reminder that I have merge this patch into the patch title"[PATCH] KVM: 
Merge kvm_ioapic_get_delivery_bitmask into kvm_get_intr_delivery_bitmask" with 
Gleb's agreement. 

Thanks. :)
diff mbox

Patch

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 7c2cb2b..7bd27aa 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -170,12 +170,13 @@  void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
 
 	ioapic_debug("dest %d dest_mode %d\n", dest, dest_mode);
 
-	*mask = 0;
+	bitmap_zero(mask, KVM_MAX_VCPUS);
+
 	if (dest_mode == 0) {	/* Physical mode. */
 		if (dest == 0xFF) {	/* Broadcast. */
 			for (i = 0; i < KVM_MAX_VCPUS; ++i)
 				if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
-					*mask |= 1 << i;
+					__set_bit(i, mask);
 			return;
 		}
 		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
@@ -184,7 +185,7 @@  void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
 				continue;
 			if (kvm_apic_match_physical_addr(vcpu->arch.apic, dest)) {
 				if (vcpu->arch.apic)
-					*mask = 1 << i;
+					__set_bit(i, mask);
 				break;
 			}
 		}
@@ -195,7 +196,7 @@  void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
 				continue;
 			if (vcpu->arch.apic &&
 			    kvm_apic_match_logical_addr(vcpu->arch.apic, dest))
-				*mask |= 1 << vcpu->vcpu_id;
+				__set_bit(i, mask);
 		}
 	ioapic_debug("mask %x\n", *mask);
 }