From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH v4 22/56] KVM: arm/arm64: vgic-new: Add MMIO handling framework Date: Wed, 18 May 2016 17:46:55 +0100 Message-ID: <573C9C7F.9020209@arm.com> References: <1463392481-26583-1-git-send-email-andre.przywara@arm.com> <1463392481-26583-23-git-send-email-andre.przywara@arm.com> <573B1DB0.7030901@arm.com> <20160518122521.GD3827@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org To: Christoffer Dall , Marc Zyngier Return-path: In-Reply-To: <20160518122521.GD3827@cbox> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org Hi, >>> + >>> +/* generate a mask that covers 1024 interrupts with bits per IRQ */ >> >> Hmmm. I'd appreciate some additional comments, specially when it comes >> to the various restrictions. May I'd suggest something like: >> >> /* >> * Generate a mask that covers the number of bytes required to address >> * up to 1024 interrupts, each represented by bits. This assumes >> * that is a power of two. >> * >> * ilog2(b) + ilog2(1024) is the number of bits required to bit-address >> * 1024 interrupts, each represented by b bits. Minus ilog2(8) converts >> * this to a byte address. > > So I'm guessting this is a rewrite of ilog2( (b * 1024) / 8), but I'm > stupid enough to not understand our use of logarithms here. Can someone > remind me whatever I forgot at CS 101? I guess it was more me not seeing the wood for the trees here: Indeed doing the multiplication first and then calling ilog2 seems to make more sense. Also I was thinking: Isn't "GENMASK_ULL(ilog2(n) - 1, 0)" the same as "n - 1"? So can't we just write: #define VGIC_ADDR_IRQ_MASK(bpi) (((bpi) * 1024 / 8) - 1) Proven by enumeration - over the values we use ;-) I'd keep the first paragraph of Marc's comment above then, but we can avoid mentioning Advanced Maths textbooks about binary logarithmic ;-) Cheers, Andre. >> */ >> >>> +#define VGIC_ADDR_IRQ_MASK(b) GENMASK_ULL(ilog2(b) + ilog2(1024) - \ >>> + ilog2(BITS_PER_BYTE) - 1, 0) >> >> /* >> * Convert a base address into a base interrupt (each interrupt >> * represented by bits. This assumes that is a power >> * of two, that both part of a memory region aligned on a > > did you mean ' *is* both part of' ? > >> * bits boundary, and itself aligned on that same boundary >> * (for regions that describe an interrupt with more than a single >> * byte of data). >> */ >> > > In any case, thanks for the commentary, I was faily lost here. >