linux-kernel - Re: [PATCH v3 3/5] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191203161644.GB17275@xz-x1>
Date:   Tue, 3 Dec 2019 11:16:44 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Nitesh Narayan Lal <nitesh@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Subject: Re: [PATCH v3 3/5] KVM: X86: Use APIC_DEST_* macros properly in
 kvm_lapic_irq.dest_mode

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