Re: [Xen-devel] [PATCH v5 02/12] arm/mem_access: Move PAGE_SHIFT_* macros to lib.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 15:07, <proskurin@xxxxxxxxxxxxx> wrote:
> On 07/03/2017 11:16 AM, Jan Beulich wrote:
>>>>> On 03.07.17 at 11:03, <proskurin@xxxxxxxxxxxxx> wrote:
>>> To prevent potential type width issues with ARMv7, I would reuse the
>>> macro from xen/iommu.h at this point:
>>>
>>> PAGE_MASK_GRAN(sz)                     (~(u64)0 << PAGE_SHIFT_##sz)
>> Seems reasonable, except
>> - no new use of u64 please (use uint64_t instead)
>> - it's questionable whether a 64-bit type here is correct in the
>>   first place, especially when considering 32-bit architectures
> 
> As to prevent breaking the currently available implementation making use
> of the existing defines in xen/iommu.h, I believe the PAGE_MASK_GRAN(sz)
> macro should keep the type of uint64_t. Additonally, the introduced
> PAGE_*_GRAN(sz) macros might be applied in the context of the
> long-descriptor translation table format with 64bit PTEs for both ARMv8
> and ARMv7. Having an unsigned long at this point would limit the mask to
> 32 bit on ARMv7 and thus not be appropriate in handling the
> long-descriptor format on ARMv7. Please let me know if you still think
> that using the uint64_t at this point is still questionable.

Yes, I still view it as questionable, as much depends on what contexts
these macros are meant to be used in. But please realize, questionable
!= wrong.

> Apart from that, I discovered that there is no code that currently uses
> PAGE_{SIZE,MASK.ALIGN}_64*K. The only 64-bit related macro in use is
> PAGE_SHIFT_64K. So I wonder whether we need these defines after all (the
> same would apply to 16K as well)?

Yes, I think they should be added / removed / kept in full sets.

Jan


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