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

Peter Xu
2019-12-03T16:16:55Z

On Tue, Dec 03, 2019 at 02:16:01PM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@redhat.com> 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