use bitmap ops on a bitmap instead of bit ops

virt/kvm/ioapic.c +0 -0

 			return;
 		}

virt/kvm/ioapic.c +2 -3

 		for (i = 0; i < KVM_MAX_VCPUS; ++i) {

@@ -184,7 +183,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 +194,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);
 }
--

Gleb.

Review

Quoted 1 line

(quoted diff) +0 -1

-	*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.)

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).

Review

Quoted 6 lines

(quoted diff) +1 -1

 	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)?

Quoted 5 lines · Sheng Yang

virt/kvm/ioapic.c void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest, · line 173

Quoted 1 line

virt/kvm/ioapic.c +0 -1

-	*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.

Quoted 4 lines

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).

Quoted 8 lines

virt/kvm/ioapic.c void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,

Quoted 6 lines

virt/kvm/ioapic.c +1 -1

 	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 <[email protected]>

virt/kvm/ioapic.c +3 -2

index 7c2cb2b..7bd27aa 100644
@@ -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;
 		}

virt/kvm/ioapic.c +2 -3

 		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);
 }
--

Gleb.

Quoted 23 lines · On Friday 27 February 2009 21:26:58 Gleb Natapov wrote:

Quoted 5 lines · Sheng Yang

virt/kvm/ioapic.c void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest, · line 173

Quoted 1 line

virt/kvm/ioapic.c +0 -1

-	*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.

Quoted 12 lines

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).

virt/kvm/ioapic.c void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,

Quoted 6 lines

virt/kvm/ioapic.c +1 -1

 	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)?

Quoted 1 line

Oh. Here is updated patch:

Thanks. :)

Acked-by Sheng Yang <[email protected]>