On Tue, May 09, 2023 at 06:25:05PM +0200, Jan Beulich wrote: > On 09.05.2023 13:03, Roger Pau Monne wrote: > > The 'i' iterator index stores a pdx, not a pfn, and hence the initial > > assignation of start (which stores a pfn) needs a conversion from pfn > > to pdx. > > Strictly speaking: Yes. But pdx compression skips the bottom MAX_ORDER > bits, so ...
Oh, that wasn't obvious to me. > > --- a/xen/drivers/passthrough/x86/iommu.c > > +++ b/xen/drivers/passthrough/x86/iommu.c > > @@ -406,7 +406,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain > > *d) > > */ > > start = paging_mode_translate(d) ? PFN_DOWN(MB(1)) : 0; > > ... with this, ... > > > - for ( i = start, count = 0; i < top; ) > > + for ( i = pfn_to_pdx(start), count = 0; i < top; ) > > ... this is an expensive identity transformation. Could I talk you into > adding > > ASSERT(start == pfn_to_pdx(start)); > > instead (or the corresponding BUG_ON() if you'd prefer that, albeit then > the expensive identity transformation will still be there even in release > builds; not that it matters all that much right here, but still)? So far the value of start is not influenced by hardware, so having an assert should be fine. Given that the assignation is just done once at the start of the loop I don't see it being that relevant to the performance of this piece of code TBH, the more that we do a pdx_to_pfn() for each loop iteration, so my preference would be to use the proposed change. > In any event, with no real bug fixed (unless I'm overlooking something), > I would suggest to drop the Fixes: tag. Right, I could drop that. Thanks, Roger.
