Re: [Xen-devel] [PATCH] dump_p2m_table: For IOMMU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.12 at 12:50, Wei Wang <wei.wang2@xxxxxxx> 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.

>> (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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel