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

[Wei Wang <wei.wang2@xxxxxxx>]

On 08/10/2012 02:52 PM, Jan Beulich wrote:
> > > > 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.
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


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