Re: [PATCH 0/3] KVM: x86/mmu: Add macro for hugepage GFN mask
On Thu, Oct 29, 2020 at 08:08:48AM +0100, Paolo Bonzini wrote: > On 28/10/20 16:29, Sean Christopherson wrote: > > The naming and usage also aligns with the kernel, which defines PAGE, PMD > > and > > PUD masks, and has near identical usage patterns. > > > > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > > #define PAGE_MASK (~(PAGE_SIZE-1)) > > > > #define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT) > > #define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1)) > > > > #define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT) > > #define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1)) > > Well, PAGE_MASK is also one of my pet peeves for Linux. At least I am > consistent. :) > > >> 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. > > > > IMO, "x & -y" has a higher barrier to entry, especially when the kernel's > > page > > masks uses "x & ~(y - 1))". But, my opinion is definitely colored by my > > inability to read two's-complement on the fly. > > Fair enough. What about having instead > > #define KVM_HPAGE_GFN_BASE(gfn, level) \ >(x & ~(KVM_PAGES_PER_HPAGE(gfn) - 1)) > #define KVM_HPAGE_GFN_INDEX(gfn, level) \ >(x & (KVM_PAGES_PER_HPAGE(gfn) - 1)) > > ? Hmm, not awful? What about OFFSET instead of INDEX, to pair with page offset? I don't particularly love either one, but I can't think of anything better.
Re: [PATCH 0/3] KVM: x86/mmu: Add macro for hugepage GFN mask
On 28/10/20 16:29, Sean Christopherson wrote: > The naming and usage also aligns with the kernel, which defines PAGE, PMD and > PUD masks, and has near identical usage patterns. > > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > #define PAGE_MASK (~(PAGE_SIZE-1)) > > #define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT) > #define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1)) > > #define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT) > #define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1)) Well, PAGE_MASK is also one of my pet peeves for Linux. At least I am consistent. :) >> 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. > > IMO, "x & -y" has a higher barrier to entry, especially when the kernel's page > masks uses "x & ~(y - 1))". But, my opinion is definitely colored by my > inability to read two's-complement on the fly. Fair enough. What about having instead #define KVM_HPAGE_GFN_BASE(gfn, level) \ (x & ~(KVM_PAGES_PER_HPAGE(gfn) - 1)) #define KVM_HPAGE_GFN_INDEX(gfn, level) \ (x & (KVM_PAGES_PER_HPAGE(gfn) - 1)) ? Paolo
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
