Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Nadav Amit <namit@vmware.com>
To:     Robin Murphy <robin.murphy@arm.com>
CC:     Joerg Roedel <joro@8bytes.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        Jiajun Cao <caojiajun@vmware.com>,
        Will Deacon <will@kernel.org>,
        "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH v3 6/6] iommu/amd: Sync once for scatter-gather operations
Thread-Topic: [PATCH v3 6/6] iommu/amd: Sync once for scatter-gather
 operations
Thread-Index: AQHXYdkgKs2MbFD4qkq3dIbdm3nnb6sVaxsA
Date:   Tue, 15 Jun 2021 18:51:08 +0000
Message-ID: <DA679904-017D-477A-9284-46644D6F9858@vmware.com>
References: <20210607182541.119756-1-namit@vmware.com>
 <20210607182541.119756-7-namit@vmware.com>
 <99671205-134d-7563-63e2-b65c13d5d074@arm.com>
In-Reply-To: <99671205-134d-7563-63e2-b65c13d5d074@arm.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?h37rIZsiCAggnA1K7HymvA6P5KEvnaIp3uVyKghzby4jh7SlXF6TSNJjgAfG?=
 =?us-ascii?Q?Z1cP3p1SapWEpCf1aJ0wODT9Mjznq+346FhI45ukRKRulDHVWH7tX5hdbGtY?=
 =?us-ascii?Q?HXOamv6oG5z5+CxaUZ3FV8hbW2XKKL9xynVNRdZf4nX1+/hiZGXN6rcSuq7u?=
 =?us-ascii?Q?dpFzdTJ6EMHUP9qcTuQcFhKJn6IIFKHat7xH+KIO8SWOU2zGIci5zpHtHvTs?=
 =?us-ascii?Q?Itrbc0BAJjnjRE8daQ4YwA84BcTyxKI9TDJRe/HxgLtj+XB5U6fhWXe27GQ7?=
 =?us-ascii?Q?TbqXUEviW7Lou7k8ke08cjK5Zi8YOzcX63nOSPewwTuvwa9cuyfXqNnuY/fc?=
 =?us-ascii?Q?aQPVjl7dsJBRVzIPgZgLuCIF39JO+D/wyO6/4yg/MNQObe3TrioHeLsIItMi?=
 =?us-ascii?Q?hiiOuOC8yyDw+PxrlTGB69A9u0FAJ0Qe3IBlUwwIQ4YCmTP44FoLRUql3L2Q?=
 =?us-ascii?Q?4aBsz5yabv1Hum28OQvz4/z5YjaLLaTlrZwY1SFNj/wqIaqV28B1+Pul9UzH?=
 =?us-ascii?Q?YL1KS6NTwsL5Z3kn1vnhDrI0RYIXkIq8D65kD32Cunoo6ws96RcVq+tnpleM?=
 =?us-ascii?Q?ZiUyDgwZjnPRYUM4PAYwUrz7dK4AMzNnlz6PlQyQ9oFnSun2t9JU7gZN32uS?=
 =?us-ascii?Q?yQr9mwnKhpoNUz20x4vMtsMOdDBlELNLuEcYRp9u03Aq5Ngr/p37dIIzncBh?=
 =?us-ascii?Q?ZOzuxcbJle/JkPH0HrgLeHSjf5LiX8KC2/Y7eCQrcs+CO3XEkKJHZwz4YBoQ?=
 =?us-ascii?Q?qyjLcnbfo6FlRk3arZqNmJG68p6t0Lb106qryljdy0dBFT+o3BDrvwjCW3G/?=
 =?us-ascii?Q?oJ/qHm0WA24nnUBcWo6Qo4svzfaNehcUo5BQeiJKI8HUaOv7b4z17asL9mQt?=
 =?us-ascii?Q?i6WewFSpGThkNHg5SdebKv97uUj9YzxgFSTGvC5cWbuCEG8ZpEZ7zdneq0kJ?=
 =?us-ascii?Q?/w3fLOvpZwqakbIpkuq6jN4OHay7dkzvZfjHyzNUYN9nXRs9XP5mbXR9UI/5?=
 =?us-ascii?Q?ueKjxa8kv8AmbcsD+ZxskNpnDVeJEBljRMyWNZEfoWA/npLgXryEdZRS+jKC?=
 =?us-ascii?Q?cBnWDT0VxUik5mdPKN1qhuQoxr2XvBcPOKCyfLGkpEpNv7G7qlpgqRYQQ2JI?=
 =?us-ascii?Q?73qSFnAXzxJ9c6P9WV9V5ra9YBIXe3BlhV0R9Zhf1RTedx1lrEoGlK+uKzyP?=
 =?us-ascii?Q?U/FsfCCg7j/ogN+thKaEeTjF2ZOQXmnBdJylRwdCPSshuIKQEKX1avcNLuQv?=
 =?us-ascii?Q?Tpl4WrCT4oc4U5oVIaf1768e3PedlmBtd4ZO630hc8qtexf93T4rpXRaeEdS?=
 =?us-ascii?Q?3IMrc9N4au70deHIjvuIYZ4Z?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <335AC7F53DB4BF42A40468975BC9C531@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: vmware.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY3PR05MB8531.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c88aa4d4-d7d0-46e7-38de-08d9302e89e8
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jun 2021 18:51:08.8936
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Mvvv2HorjQNl5YZQBiUdCCq1J2xTnlhUD9Rp3ocAaru55Y6GEuFKV7obhxqFuPaTbZpah2pdXmBqjxy8OogaQw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB4887
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jun 15, 2021, at 4:25 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>=20
> On 2021-06-07 19:25, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> On virtual machines, software must flush the IOTLB after each page table
>> entry update.
>> The iommu_map_sg() code iterates through the given scatter-gather list
>> and invokes iommu_map() for each element in the scatter-gather list,
>> which calls into the vendor IOMMU driver through iommu_ops callback. As
>> the result, a single sg mapping may lead to multiple IOTLB flushes.
>> Fix this by adding amd_iotlb_sync_map() callback and flushing at this
>> point after all sg mappings we set.
>> This commit is followed and inspired by commit 933fcd01e97e2
>> ("iommu/vt-d: Add iotlb_sync_map callback").
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Jiajun Cao <caojiajun@vmware.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
>> Cc: iommu@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 128f2e889ced..dd23566f1db8 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2027,6 +2027,16 @@ static int amd_iommu_attach_device(struct iommu_d=
omain *dom,
>>  	return ret;
>>  }
>>  +static void amd_iommu_iotlb_sync_map(struct iommu_domain *dom,
>> +				     unsigned long iova, size_t size)
>> +{
>> +	struct protection_domain *domain =3D to_pdomain(dom);
>> +	struct io_pgtable_ops *ops =3D &domain->iop.iop.ops;
>> +
>> +	if (ops->map)
>=20
> Not too critical since you're only moving existing code around, but is op=
s->map ever not set? Either way the check ends up looking rather out-of-pla=
ce here :/
>=20
> It's not very clear what the original intent was - I do wonder whether it=
's supposed to be related to PAGE_MODE_NONE, but given that amd_iommu_map()=
 has an explicit check and errors out early in that case, we'd never get he=
re anyway. Possibly something to come back and clean up later?

[ +Suravee ]

According to what I see in the git log, the checks for ops->map (as well as=
 ops->unmap) were relatively recently introduced by Suravee [1] in preparat=
ion to AMD IOMMU v2 page tables [2]. Since I do not know what he plans, I p=
refer not to touch this code.

[1] https://lore.kernel.org/linux-iommu/20200923101442.73157-13-suravee.sut=
hikulpanit@amd.com/
[2] https://lore.kernel.org/linux-iommu/20200923101442.73157-1-suravee.suth=
ikulpanit@amd.com/=