Re: [PATCH] x86/iommu: fix wrong iterator type in arch_iommu_hwdom_init()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.05.2023 10:32, Roger Pau Monné wrote:
> 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.

Well, okay, but then please with the description also "softened" a
little (it isn't really "needs", but e.g. "better would undergo"),
alongside ...

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

... this.

Jan