linux-kernel - Re: [PATCH 0/3] KVM: x86/mmu: Add macro for hugepage GFN mask

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] [day] [month] [year] [list]

Message-ID: <80038ae1-d603-dc22-1997-1ad7da0a936d@redhat.com>
Date:   Wed, 28 Oct 2020 16:01:15 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ben Gardon <bgardon@...gle.com>
Subject: Re: [PATCH 0/3] KVM: x86/mmu: Add macro for hugepage GFN mask

On 27/10/20 22:42, Sean Christopherson wrote:
> Add a macro, which is probably long overdue, to replace open coded
> variants of "~(KVM_PAGES_PER_HPAGE(level) - 1)".  The straw that broke the
> camel's back is the TDP MMU's round_gfn_for_level(), which goes straight
> for the optimized approach of using NEG instead of SUB+NOT (gcc uses NEG
> for both).  The use of '-(...)' made me do a double take (more like a
> quadrupal take) when reading the TDP MMU code as my eyes/brain have been
> heavily trained to look for the more common '~(... - 1)'.

The upside is that a "how many" macro such as KVM_PAGES_PER_HPAGE will
have only one definition that makes sense.  With a "mask" macro you
never know if the 1s are to the left or to the right.  That is, does
"gfn & KVM_HPAGE_GFN_MASK(x)" return the first gfn within the huge page
or the index of the page within the huge page?

This is actually a problem with this series; see this line in patch 3:

-	mask = KVM_PAGES_PER_HPAGE(level) - 1;
+	mask = ~KVM_HPAGE_GFN_MASK(level);
        ^^^^                  ^^^^

So it's a mask, but not _that_ mask, _another_ mask. :)  That's why I
don't really like "mask" macros, now the other question is how to
express bit masking with "how many" macros.

First of all, I think we all agree that when reading "x & how_many()" we
assume (or we check first thing of all) that the right side is a power
of two.  I like "x & -y" because---even if your eyes have trouble
distinguishing "-" from "~"---it's clearly not "x & (y-1)" and therefore
the only sensible operation that the AND can do "clear everything to the
right".

Now I realize that maybe my obsession for bit twiddling tricks is not
shared with everyone, and of course if you're debugging it you have to
look closer and check if it's really "x & -y" or "x & ~y", but at least
in normal cursory code reading that's how it works for me.

Paolo