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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.05.2023 12:03, Jan Beulich wrote:
> 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"),

And in the title then perhaps s/fix wrong/adjust/.

Jan

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