Re: [Qemu-devel] [PATCH RFC v4 15/20] intel_iommu: provide its own replay() callback

[Jason Wang]

On 2017年01月22日 17:36, Peter Xu wrote:
> Besides this one, I tried to fix the comments in this function as
> below, hope this is better (I removed 1-3 thing since I think that's
> clearer from below code):
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index e958f53..f3fe8c4 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -735,15 +735,6 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t 
> start,
>           offset = vtd_iova_level_offset(iova, level);
>           slpte = vtd_get_slpte(addr, offset);
>
> -        /*
> -         * When one of the following case happens, we assume the whole
> -         * range is invalid:
> -         *
> -         * 1. read block failed
> -         * 3. reserved area non-zero
> -         * 2. both read & write flag are not set
> -         */
> -
>           if (slpte == (uint64_t)-1) {
>               trace_vtd_page_walk_skip_read(iova, iova_next);
>               skipped_local++;
> @@ -761,20 +752,16 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t 
> start,
>           write_cur = write && (slpte & VTD_SL_W);
>
>           /*
> -         * As long as we have either read/write permission, this is
> -         * a valid entry. The rule works for both page or page tables.
> +         * As long as we have either read/write permission, this is a
> +         * valid entry. The rule works for both page entries and page
> +         * table entries.
>            */
>           entry_valid = read_cur | write_cur;
>
>           if (vtd_is_last_slpte(slpte, level)) {
>               entry.target_as = &address_space_memory;
>               entry.iova = iova & subpage_mask;
> -            /*
> -             * This might be meaningless addr if (!read_cur &&
> -             * !write_cur), but after all this field will be
> -             * meaningless in that case, so let's share the code to
> -             * generate the IOTLBs no matter it's an MAP or UNMAP
> -             */
> +            /* NOTE: this is only meaningful if entry_valid == true */
>               entry.translated_addr = vtd_get_slpte_addr(slpte);
>               entry.addr_mask = ~subpage_mask;
>               entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
>
> Thanks,
>
> -- peterx

I still prefer to do this on patch 18 probably.

Thanks