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.

Fixes: 6b4f6a31ace1 ('x86/PVH: de-duplicate mappings for first Mb of Dom0 memory')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index cb0788960a08..6bc79e7ec843 100644
--- 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;
 
-    for ( i = start, count = 0; i < top; )
+    for ( i = pfn_to_pdx(start), count = 0; i < top; )
     {
         unsigned long pfn = pdx_to_pfn(i);
         unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
-- 
2.40.0

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

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

In any event, with no real bug fixed (unless I'm overlooking something),
I would suggest to drop the Fixes: tag.

Jan

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.

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

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
>