Re: [PATCH v3 3/5] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode

[Peter Xu]

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