[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] x86/iommu: fix wrong iterator type in arch_iommu_hwdom_init()
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Date: Wed, 10 May 2023 10:32:13 +0200
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=R4JoY2KIceGClWxi+pH3a+dZJx6oAHtkD+56sKaBiaM=; b=dHgicEyQ7RuH9bR9vZR50Iwq75ehp9rbH9jcSbNz6o9RJu36SC036Fst3ClvCyCYSpRftVBUBk0+Kix/bTZfI00mdteruwZx2HVkTsBAa5Nz5cA53dFnm6xcQMSjv6W07J3ERdpjjT9OijK5WxXh9QPKCgXY5+Wvi5Mp1dY+NxSsO1X/W47r5gHuQNCrk5Ch6v4rBJP/g8ylvQO1kPwrP8poIKIO8t1dKd0F8VnFFkQZ7xn1J3Oofjq7dJTikdH4KuRagdlGkOhvgJsB1eELPTF3oA16tWR4nt+t34Ees779VuTKdHk9yas66LsQsNxQR40GYfChh0WB8eg4p7Eetw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dqhTLHl9xWHiF+4ESuGxK7eJAeCm1ObqYBdwancwH71Gt9GI8vomoc9WLKLtsXRUvVO6oyPGLbz3+a7Z/dUpuO5ldO9hOsLN5uW9OY45Z9z+LxnOvNZ1pzwveSdaYstNRstXRB9WiASV9ooWeuu5u+Sfkk6mXI6iar+DWIh+tE/YZCS+ksD4c+G9ZU9PNbwMcSGXbbhZA+2kjHmJ6bcYtAWgVgRALVoqaSKXLKCmvJkyrbAQNCztU5cIcoA2YUY4+W6ppz458nBKqLE4k/u0fi/W/Z1PPjr0VVkSLoEFJqwR59smxeG5yo5yNXowrGQ/dv94FjQbI9xw/fXeUk4nuA==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
- Cc: Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Wed, 10 May 2023 08:32:43 +0000
- Ironport-data: A9a23:o6EQ1KhAOdNPEMiaKFN9ujS+X161QBEKZh0ujC45NGQN5FlHY01je htvD2CCbvuIamL3KNgka4Sw8UwAsJbTm4RkQFRvrHxhQXsb9cadCdqndUqhZCn6wu8v7q5Ex 55HNoSfdpBcolv0/ErF3m3J9CEkvU2wbuOgTrWCYmYpHlUMpB4J0XpLg/Q+jpNjne+3CgaMv cKai8DEMRqu1iUc3lg8sspvkzsy+qWj0N8klgZmP6sT4QaGzyB94K83fsldEVOpGuG4IcbiL wrz5OnR1n/U+R4rFuSknt7TGqHdauePVeQmoiM+t5mK2nCulARrukoIHKN0hXNsoyeIh7hMJ OBl7vRcf+uL0prkw4zxWzEAe8130DYvFLXveRBTuuTLp6HKnueFL1yDwyjaMKVBktubD12i+ tRJLWEISzmFptuPwbSERtlJv+oyFdLSadZ3VnFIlVk1DN4AaLWbGeDgw4Yd2z09wMdTAfzZe swVLyJ1awjNaAFOPVFRD48imOCvhT/0dDgwRFC9/PJrpTSMilEoluS1WDbWUoXiqcF9hEGXq 3iA523kKhobKMae2XyO9XfEaurnxHukAt9PT+zlnhJsqEGY4GoXCA8vb0OApcKV2mmdR8Nkd lNBr0LCqoB3riRHVOLVXRe1vXqFtR40QMdLHqsx7wTl4rLd/gKxFmUCCDlbZ7QOpMIwADAny FKNt9foHiB09q2YT2qH8bWZpi/0PjIaRVLufgcBRAoBptPl+Yc6i0qVSs45SPLtyNroBTv33 jaG6jAkgKkehtIK0KP9+k3bhzWrpd7CSQtdChjrY19JJzhRPOaND7FEI3CCvZ6s8K7xooG9g UU5
- Ironport-hdrordr: A9a23:GGtYzqyLI8KEYw6r8TccKrPxiOgkLtp133Aq2lEZdPULSK2lfp GV8sjziyWatN9IYgBepTiBUJPwJk80hqQFn7X5XI3SEDUO3VHJEGgM1/qY/9SNIVyaygcZ79 YdT0EcMqy+MbEZt7eB3ODQKb9Jq7X3k9HLuQ6d9QYRcegAUdAH0+4NMHfiLqQAfng+OXNWLu v52uN34x6bPVgHZMWyAXcIG8DFut3wjZrjJTIWGhI97wGKrDWwrJr3CQKR0BsyWy5Ghe5Kyx mFryXJooGY992rwB7V0GHeq7xQhdva09NGQOCcl8QPLT3oqwCwIKBsQaeLsjwZqPymrHwqjN 7PiRE9ONkb0QKeQkiF5T/WnyXw2jcn7HHvjXWCh2H4nMD/TDUmT+JcmINwaHLimgkdleA59J gO83OStpJRAx+Ftj/6/cL0WxZjkVfxiWY+kNQUk2dUXeIlGf1sRM0kjQZo+aU7bWXHAbMcYa 9T5Qbnla9rmGahHjTkV69UsYSRtzoIb0y7qwM5y72oOnBt7QBEJg0jtYwidhppzuNmd7B0o9 nhdoxkmbFICucLcKMVPpZQfeKHTlHoBTrAPWKUZW39EqwaMW/mrZP6iY9Ft92CSdg06N8elJ HAT19C8VQzdUXnFNGU0PRwg0HwaVT4YBCo7ul/wtxDlpfRZIXGHGm/aHQD+vHLn9wvRvD+H9 KaGLcTP8PCAALVdLqgGmXFKsZvwb13arxIhj79M2j+//7jG8nWksTgXLLjCpbLOxMDZk6XOA pcYNG7HrQy0mm7HnD/mxTfQHXrZwj2+o9xCrHT+6wJxJEKLZAkiHlftb2V3LDDFdR5iN1/QG JuZLf81q+rr2i/+mjFq21vJxpGF05QpLHtSWlDqwMGO179Ne9rgaTTRUlCmH+cYhNvRcLfFw BS41xx5KKsNpSVgSQvEciuPG6Wh2Ya4HiKU5AfkKue4tqNQOJzMr82HKhqUQnbHR18nghn7G 9FdQ8fX0faUijjjK205aZkct03t+MM9ztDDfQk3U4373/s1/3HbkFrKgKTbQ==
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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.
|
