On Tue, Dec 03, 2019 at 02:16:01PM +0100, Vitaly Kuznetsov wrote: > Peter Xu <peterx@xxxxxxxxxx> writes: > > > We were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL or 0|1 to > > fill in kvm_lapic_irq.dest_mode. It's fine only because in most cases > > when we check against dest_mode it's against APIC_DEST_PHYSICAL (which > > equals to 0). However, that's not consistent. We'll have problem > > when we want to start checking against APIC_DEST_PHYSICAL > > APIC_DEST_LOGICAL Fixed. > > + irq->dest_mode = kvm_lapic_irq_dest_mode( > > + (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo); > > This usage is a bit fishy (I understand that it works, but), > kvm_lapic_irq_dest_mode()'s input is bool (0/1) but here we're passing > something different. > > I'm not sure kvm_lapic_irq_dest_mode() is even needed here, but in case > it is I'd suggest to add '!!': > > kvm_lapic_irq_dest_mode(!!((1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo)) > > to make things explicit. I don't like how it looks though. IMHO it's the same (converting uint to _Bool will be the same as "!!", also A ? B : C will be another, so we probably wrote this three times, each of them will translate to a similar pattern of "cmpl + setne" asm code). But sure I can add them if you prefer. Thanks, -- Peter Xu
