Re: [PATCH] dump_p2m_table: For IOMMU

[Wei Wang <wei.wang2@amd.com>]

On 08/10/2012 02:52 PM, Jan Beulich wrote:
>>>> On 10.08.12 at 12:50, Wei Wang<wei.wang2@amd.com>  wrote:
>> On 08/09/2012 09:26 AM, Jan Beulich wrote:
>>
>>> Wei - here I'm particularly worried about the use of "level - 1"
>>> instead of "next_level", which would similarly apply to the
>>> original function. If the way this is currently done is okay, then
>>> why is next_level being computed in the first place?
>>
>> I think that recalculation is to guarantee that this recursive function
>> returns. It should run at most "paging_mode" times no matter what
>> "next_level" says. But if we could assume that next level field in every
>> pde is correct, then using next level is fine to me.
>
> Especially in the dumping function we shouldn't assume too
> much. However, wasn't it that one can skip levels in your
> IOMMU implementation? That can't be handled correctly if
> always subtracting 1.

We have no skip levels yet. But since it checks (next_level != 0) before 
calling itself, it should not deallocate pages unexpectedly. But it will 
also waste some time in the loop. if next_level == 0 but level > 1 (e.g. 
we have only l4, l2, l1 tables). So, yes, now using next_level with 
ASSERT also looks better to me.

>
>>> (And similar
>>> to the issue Santosh has already fixed here - the original
>>> function pointlessly maps/unmaps the page when "level<= 1".
>>> Furthermore, iommu_map.c has nice helper functions
>>> iommu_next_level() and amd_iommu_is_pte_present() - why
>>> aren't they in a header instead, so they could be used here,
>>> avoiding the open coding of them?)
>>
>> Maybe those helps appears after the original function. I could sent a
>> patch to clean up these:
>> * do not map/unmap if level<= 1
>> * move amd_iommu_is_pte_present() and iommu_next_level() to a header
>> file. and use them in deallocate_next_page_table.
>> * Using next_level instead of recalculation (if requested)
>
> Yes, please. As to using next_level - it depends, besides the above,
>   on how bad it is if this is really wrong; an ASSERT() or BUG_ON()
> might be on order here.

How about ASSERT(next_level < = (level -1) )?

> Jan
>
>