Re: [RFC PATCH 4/4] iommu: make IOVA domain page size explicit

[leizhen <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

On 2014/11/26 21:31, Robin Murphy wrote:
> Hi,
> 
> On 26/11/14 07:17, leizhen wrote:
> [...]
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index a3dbba8..9e50625 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -55,12 +55,16 @@ void free_iova_mem(struct iova *iova)
>>>   }
>>>
>>>   void
>>> -init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>>> -    unsigned long pfn_32bit)
>>> +init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>> +    unsigned long start_pfn, unsigned long pfn_32bit)
>>>   {
>>> +    /* IOMMUs must support at least the CPU page size */
>>> +    BUG_ON(granule > PAGE_SIZE);
>>
>> ??? BUG_ON(granule < PAGE_SIZE);
>>
> 
> Granule represents the smallest page size that the IOMMU can map - we'd never allocate less IOVA space than a single page, so we don't need a unit smaller than that, and a bigger unit would make it
> problematic to allocate IOVA space for individual pages.
> 
> If, for instance, the CPU has 64k pages, and the IOMMU supports 1k tiny pages, granule would be less than PAGE_SIZE, but the IOMMU could happily that 64k as 64 contiguous tiny pages, even if it's a
> waste of TLB space.
> 
> However, say the IOMMU only supports 16k pages and we try to map two noncontiguous 4k CPU pages as a contiguous 8k buffer in IOVA space - this can't work, hence why the _minimum_ IOMMU page size, and
> thus granule, may never be _bigger_ than a CPU page.
> 

Oh, I just confused with the comment: /* IOMMUs must support at least the CPU page size */
But the code means at most the CPU page size. Now, I got it.

>>> +
>>>       spin_lock_init(&iovad->iova_rbtree_lock);
>>>       iovad->rbroot = RB_ROOT;
>>>       iovad->cached32_node = NULL;
>>> +    iovad->granule = granule;
>>
>> Do you need to check granule is 2^n ?
>>
> 
> Yes, that would make a lot of sense.
> 
>>>       iovad->start_pfn = start_pfn;
>>>       iovad->dma_32bit_pfn = pfn_32bit;
>>>   }
>>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>>> index 591b196..3920a19 100644
>>> --- a/include/linux/iova.h
>>> +++ b/include/linux/iova.h
>>> @@ -28,6 +28,7 @@ struct iova_domain {
>>>       spinlock_t    iova_rbtree_lock; /* Lock to protect update of rbtree */
>>>       struct rb_root    rbroot;        /* iova domain rbtree root */
>>>       struct rb_node    *cached32_node; /* Save last alloced node */
>>> +    unsigned long    granule;    /* pfn granularity for this domain */
>>>       unsigned long    start_pfn;    /* Lower limit for this domain */
>>>       unsigned long    dma_32bit_pfn;
>>>   };
>>> @@ -37,6 +38,36 @@ static inline unsigned long iova_size(struct iova *iova)
>>>       return iova->pfn_hi - iova->pfn_lo + 1;
>>>   }
>>>
>>> +static inline unsigned long iova_shift(struct iova_domain *iovad)
>>> +{
>>> +    return __ffs(iovad->granule);
>>
>> I think it's good to save the result to a struct member. If we caculate it ervertime, it's too slow.
>>
> 
> I did give some consideration at the time to which of size, shift and mask to store - given the redundancy between them storing more than one seems excessively wasteful - and this made for the
> nicest-looking code IMO. These IOVA operations are going to be mostly used in map/unmap situations leading to an IOMMU TLB flush anyway, so I'm not sure how performance-critical they really are.
> 
> Are we likely to see this code used on architectures where __ffs takes more than a few cycles and has more impact than the general memory bloat of making structures bigger?
> 
> 
> Thanks for the review,
> Robin.
> 
>>> +}
>>> +
>>> +static inline unsigned long iova_mask(struct iova_domain *iovad)
>>> +{
>>> +    return iovad->granule - 1;
>>> +}
>>> +
>>> +static inline size_t iova_offset(struct iova_domain *iovad, dma_addr_t iova)
>>> +{
>>> +    return iova & iova_mask(iovad);
>>> +}
>>> +
>>> +static inline size_t iova_align(struct iova_domain *iovad, size_t size)
>>> +{
>>> +    return ALIGN(size, iovad->granule);
>>> +}
>>> +
>>> +static inline dma_addr_t iova_dma_addr(struct iova_domain *iovad, struct iova *iova)
>>> +{
>>> +    return (dma_addr_t)iova->pfn_lo << iova_shift(iovad);
>>> +}
>>> +
>>> +static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
>>> +{
>>> +    return iova >> iova_shift(iovad);
>>> +}
>>> +
>>>   int iommu_iova_cache_init(void);
>>>   void iommu_iova_cache_destroy(void);
>>>
>>> @@ -50,8 +81,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
>>>   struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
>>>       unsigned long pfn_hi);
>>>   void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
>>> -void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>>> -    unsigned long pfn_32bit);
>>> +void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>> +    unsigned long start_pfn, unsigned long pfn_32bit);
>>>   struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>>>   void put_iova_domain(struct iova_domain *iovad);
>>>   struct iova *split_and_remove_iova(struct iova_domain *iovad,
>>>
>>
>>
>>
> 
> 
> 
> .
>