Re: [PATCH v5 14/14] drivers/passthrough: arm: Add support for SMMU drivers

[Julien Grall <julien.grall@linaro.org>]

On 05/14/2014 08:29 AM, Jan Beulich wrote:
>>>> On 13.05.14 at 17:50, <julien.grall@linaro.org> wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -40,6 +40,9 @@ extern bool_t amd_iommu_perdev_intremap;
>>  #define PAGE_MASK_4K        (((u64)-1) << PAGE_SHIFT_4K)
>>  #define PAGE_ALIGN_4K(addr) (((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
>>  
>> +#define PAGE_SHIFT_64K      (16)
>> +#define PAGE_SIZE_64K       (1UL << PAGE_SHIFT_64K)
> 
> I think for consistency you should also define PAGE_MASK_64K and
> PAGE_ALIGN_64K, perhaps via helper macros then also to be used
> to implement the 4k variants. That's not the least because you can't
> safely use PAGE_SIZE_64K to implement address masking on 32-bit
> ARM (which iirc supports physical addresses wider than 32 bits), due
> to the use of 1UL.
> 
> E.g.
> 
> #define IOMMU_PAGE_SIZE(sz)        (1UL << PAGE_SHIFT_##sz)
> #define IOMMU_PAGE_MASK(sz)        (~(u64)0 << PAGE_SHIFT_##sz)
> #define IOMMU_PAGE_ALIGN(sz, addr) (((addr) + ~PAGE_MASK_##sz) & PAGE_MASK_##sz)
> 
> #define PAGE_SIZE_4K        IOMMU_PAGE_SIZE(4K)
> etc.

I will use this solution.

> Anyway,
> Acked-by: Jan Beulich <jbeulich@suse.com>
> for this small piece of non-ARM code, no matter whether you follow
> the advice.

Thanks!

-- 
Julien Grall