Re: [Qemu-devel] [RFC v4 1/5] hw/arm/smmu-common: smmu base class

[Auger Eric]

Hi Peter,

On 30/05/2017 17:56, Peter Maydell wrote:
> On 13 May 2017 at 18:43, Eric Auger <address@hidden> wrote:
>> Introduces the base device and class for the ARM smmu.
>> Implements VMSAv8-64 table lookup and translation. VMSAv8-32
>> is not yet implemented.
>>
>> For VFIO integration we will need to notify mapping changes
>> of an input range and skipped unmapped regions. table walk
>> helper allows.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>> Signed-off-by: Prem Mallappa <address@hidden>
>>
>> +/**
>> + * smmu_page_walk_level_64 - Walk an IOVA range from a specific level
>> + * @baseaddr: table base address corresponding to @level
>> + * @level: level
>> + * @cfg: translation config
>> + * @start: end of the IOVA range
>> + * @end: end of the IOVA range
>> + * @hook_fn: the hook that to be called for each detected area
>> + * @private: private data for the hook function
>> + * @read: whether parent level has read permission
>> + * @write: whether parent level has write permission
>> + * @must_translate: indicates whether each iova of the range
>> + *  must be translated or whether failure is allowed
>> + * @notify_unmap: whether we should notify invalid entries
>> + *
>> + * Return 0 on success, < 0 on errors not related to translation
>> + * process, > 1 on errors related to translation process (only
>> + * if must_translate is set)
>> + */
>> +static int
>> +smmu_page_walk_level_64(dma_addr_t baseaddr, int level,
>> +                        SMMUTransCfg *cfg, uint64_t start, uint64_t end,
>> +                        smmu_page_walk_hook hook_fn, void *private,
>> +                        bool read, bool write, bool must_translate,
>> +                        bool notify_unmap)
>> +{
> 
>> +        /* table pte */
>> +        next_table_baseaddr = get_table_pte_address(pte, granule_sz);
>> +        trace_smmu_page_walk_level_table_pte(pte, next_table_baseaddr);
>> +        ret = smmu_page_walk_level_64(next_table_baseaddr, level + 1, cfg,
>> +                                      iova, MIN(iova_next, end),
>> +                                      hook_fn, private, read_cur, write_cur,
>> +                                      must_translate, notify_unmap);
>> +        if (!ret) {
>> +            return ret;
>> +        }
>> +
>> +next:
>> +        iova = iova_next;
>> +    }
>> +
>> +    return SMMU_TRANS_ERR_NONE;
>> +}
> 
> It seems a bit odd that this function is recursive -- I was
> expecting it just to loop through for each successive level
> until it hits a block/page entry, as the LPAE table walk
> code in target/arm/helper.c does...

welcome back ;-)

Thanks for the pointer to get_phys_addr_lpae(). Actually I got largely
inspired of vtd_page_walk and vtd_page_walk_level of
hw/i386/intel_iommu.c which also use the same recursive form.

The peculiarity compared to get_phys_addr_lpae() is that we scan a range
of iovas and do not translate a single iova. In case a level entry is
not implemented we want to jump to the next one at the same level and in
case the entry matches a table entry we want to scan the next level
entries matching that iova range.

Does that make more sense?

Thanks

Eric
> 
> thanks
> -- PMM
>