From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH] use bitmap ops on a bitmap instead of bit ops Date: Fri, 27 Feb 2009 11:19:09 +0800 Message-ID: <200902271119.12359.sheng@linux.intel.com> References: <20090226130339.GG8810@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: avi@redhat.com, kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mga06.intel.com ([134.134.136.21]:19076 "EHLO orsmga101.jf.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753599AbZB0DTT (ORCPT ); Thu, 26 Feb 2009 22:19:19 -0500 In-Reply-To: <20090226130339.GG8810@redhat.com> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On Thursday 26 February 2009 21:03:39 Gleb Natapov wrote: > And don't zero bitmap twice. Assume that caller zeros it. > > Signed-off-by: Gleb Natapov > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > index 7c2cb2b..21627b9 100644 > --- a/virt/kvm/ioapic.c > +++ b/virt/kvm/ioapic.c > @@ -170,12 +170,11 @@ void kvm_ioapic_get_delivery_bitmask(struct > kvm_ioapic *ioapic, u8 dest, > > ioapic_debug("dest %d dest_mode %d\n", dest, dest_mode); > Hi Gleb > - *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). > 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)? -- regards Yang, Sheng > return; > } > 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.