From mboxrd@z Thu Jan  1 00:00:00 1970
From: andre.przywara@arm.com (Andre Przywara)
Date: Wed, 18 May 2016 17:46:55 +0100
Subject: [PATCH v4 22/56] KVM: arm/arm64: vgic-new: Add MMIO handling
 framework
In-Reply-To: <20160518122521.GD3827@cbox>
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>
Message-ID: <573C9C7F.9020209@arm.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

Hi,

>>> +
>>> +/* generate a mask that covers 1024 interrupts with <b> 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 <b> bits. This assumes
>>  * that <b> 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> bits. This assumes that <bits> is a power
>>  * of two, that <addr> both part of a memory region aligned on a
> 
> did you mean '<addr> *is* both part of' ?
> 
>>  * <b> 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.
>