Re: [Xen-devel] [PATCH v5 01/30] bitops: add GENMASK_ULL

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 06/04/17 10:15, Jan Beulich wrote:
>>>> On 06.04.17 at 10:45, <julien.grall@xxxxxxx> wrote:
>> (CC 'The REST' maintainers)
>>
>> Hi,
>>
>> Andre, as I mentioned already a couple of times. You should CC all the 
>> appropriate maintainers for the code you are modifying.
>>
>> On 06/04/17 00:25, Stefano Stabellini wrote:
>>> On Thu, 6 Apr 2017, Andre Przywara wrote:
>>>> To safely handle 64-bit registers even on 32-bit systems, introduce
>>>> a GENMASK_ULL variant (lifted from Linux).
>>>> This adds a BITS_PER_LONG_LONG define as well.
>>>> Also fix a bug in the comment for the existing GENMASK variant.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>>
>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>
>> I'd like some input from Andrew here. I suggested a similar patch a year 
>> ago (see [1]) and the final decision was to drop GENMASK_ULL.
> 
> Well, to be honest, rather than asking Andrew (who would likely
> just re-state his opinion, as I would re-state mine), it should be
> Andre to make clear why things are different now than they
> were when the introduction of the macro was first rejected.

So first for the case of GENMASK in general:
ARM64 system registers or ARM IP registers (like in the GIC interrupt
controller) are often 64-bit, and often have several fields encoded at
bit locations *not* nibble aligned, for instance here:
GITS_BASER[0-7]:
Valid: bit[63], Indirect: bit[62], InnerCache: bits[61:59],
Type: bits[58:56], OuterCache: bits[55:53], ...
(from section 8.19.1 of ARM IHI 0069C [1])
So generating bitmasks for those fields is both tedious and error-prone,
also hard to read because of the required 16 nibbles, compare:
0x3800000000000000 vs. GENMASK(61, 59).
I think this might be an ARM specialty, which would explain why nobody
else missed it before.

For this special GENMASK_ULL version: The GIC interrupt controller is
usable from 32-bit ARM (software) as well, so the ~0UL in the normal
GENMASK is not sufficient to cover 64 bits here.

Now although this particular series is for ARM64 only, Julien wanted to
prepare for the case we ever need to port this to ARM32 as well, see:
https://lists.xen.org/archives/html/xen-devel/2016-11/msg00080.html

So long story short: Technically we don't need GENMASK_ULL (and BIT_ULL)
at the moment, but Julien asked for it. So personally I am happy to drop
them, if Julien agrees.
On the other hand this is just a tiny patch, also lifted from Linux, so
I don't see any real danger in including it into Xen (which would also
save me to fix up all the _ULL versions in my patches).

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel