On 09.07.2012, at 16:28, Scott Wood wrote: > On 07/09/2012 12:13 AM, Bhushan Bharat-R65777 wrote: >> >> >>> -----Original Message----- >>> From: Alexander Graf [mailto:agraf@xxxxxxx] >>> Sent: Saturday, July 07, 2012 1:21 PM >>> To: Wood Scott-B07421 >>> Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Bhushan >>> Bharat-R65777 >>> Subject: Re: [PATCH 2/2] KVM: PPC: booke: Add watchdog emulation >>> >>> >>> Ah, being 2 bits wide, the above code suddenly makes more sense :). How about >>> >>> /* WRC is a 2-bit field that is supposed to preserve its value once written to >>> be non-zero */ >>> spr_val &= ~TCR_WRC_MASK; >>> spr_val |= vcpu->arch.tcr & TCR_WRC_MASK; >>> kvmppc_set_tcr(vcpu, spr_val); >> >> I think you mean: >> >> if (TCR_WRC_MASK & vcpu->arch.tcr) { >> spr_val &= ~TCR_WRC_MASK; >> spr_val |= vcpu->arch.tcr & TCR_WRC_MASK; >> } >> kvmppc_set_tcr(vcpu, spr_val); > > Actually I think he means: > > if (vcpu->arch.tcr & TCR_WRC_MASK) { > spr_val &= ~TCR_WRC_MASK; > spr_val |= vcpu->arch.tcr & TCR_WRC_MASK; > } > > kvmppc_set_tcr(vcpu, spr_val); Plus the comment, yes :). I really don't like (mask & val) constructs. About as much as I dislike if (0 == x) ones. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
