Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Liu, Yi L" <yi.l.liu@intel.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>
CC:     "cohuck@redhat.com" <cohuck@redhat.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "nicolinc@nvidia.com" <nicolinc@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
        "chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
        "yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
        "peterx@redhat.com" <peterx@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        "lulu@redhat.com" <lulu@redhat.com>,
        "suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "Duan, Zhenzhong" <zhenzhong.duan@intel.com>,
        "joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
        "Zeng, Xin" <xin.zeng@intel.com>,
        "Zhao, Yan Y" <yan.y.zhao@intel.com>
Subject: RE: [PATCH v6 2/6] iommufd: Add IOMMU_HWPT_INVALIDATE
Thread-Topic: [PATCH v6 2/6] iommufd: Add IOMMU_HWPT_INVALIDATE
Thread-Index: AQHaGVcJ4nFLu64FV02Qq9BjVpwSvbCC3axQ
Date:   Mon, 20 Nov 2023 08:09:35 +0000
Message-ID: <BN9PR11MB5276D8406BF08B853329288C8CB4A@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20231117130717.19875-1-yi.l.liu@intel.com>
 <20231117130717.19875-3-yi.l.liu@intel.com>
In-Reply-To: <20231117130717.19875-3-yi.l.liu@intel.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?JMohBZkjcyHJ6bcoWGZs2+S2VSj42bs9xXAuLAxauWIQb6IglKBKvZQRtxo/?=
 =?us-ascii?Q?CRvOkjsKSNSxlIwVimZKe7CLU53lmr51a9+Q8DoX6AV2LETbKLNvLgQh1giq?=
 =?us-ascii?Q?Bw85rMkAfHzps4e9KocFwsKXgDKGVsbA0VZVIIMHmvMQnpMQkZ1UC6U7tmv4?=
 =?us-ascii?Q?82UAKJ6MDI5zpwREphDgoreOMTd3+1pNsDXbHkyWkBr3Biva619WBPaQkiPJ?=
 =?us-ascii?Q?p/96hqN6fZDNTzyCh8F0kPbQT8Wrwf0cz7Xnnjs0IJBsuLNrR5YEuYUiiRqq?=
 =?us-ascii?Q?VMpeQLcgl0yMgQNSD758etiJ3cJjyqzPpIEaVxKEQZYA8PTS9pXy688+wEdr?=
 =?us-ascii?Q?SuxxhG5j1pouw+LoMKk77mHoWIUsL74MYgkKoP0yfG7VG0WVykkII3oxQYqH?=
 =?us-ascii?Q?n9eH9dmsUAoB3jLiOTia1jowa0vgGa6EYkBw5BNsd+wy5c1sz2fvi7fQ+EeP?=
 =?us-ascii?Q?9pKh4Sndr1+KhAYRBZki8bOFQP4tEUZgESD0OJN6D/a9tHdzTjkDdtrte3lw?=
 =?us-ascii?Q?RtcOoBONSppOea/yToadc0hj8h6OsCXec1W3fQI9bBpO5bBNWrjZpY16CRia?=
 =?us-ascii?Q?acvHm0s4x6cBLaKyE0uEyBLx18Pb+Lzj4ZJfu6FVOkvpRp+8eK2pJkoCR6hr?=
 =?us-ascii?Q?CanllFw4W/7JGAH2rst6C34AjsD/3vA12GuZCkNB7sEB6qVWsOs1QbqfApHz?=
 =?us-ascii?Q?DR+NfOoWo+amNfDeA3ozuD9SdvtDvQvzhtw3BguSs9SBPMhrwyH2e72yrmSM?=
 =?us-ascii?Q?CXA3OmiszSlT09vCmHUQ71h8f7rQxxY2lJBCz/u5pMeXAGcbRFpiFDQZw3a3?=
 =?us-ascii?Q?tkUdVpKUX6kN41uyYYu58gnuwPQ9UL7jQwQpvuG4ip87Ll2d1N/vc8Sigjx2?=
 =?us-ascii?Q?i7bgB6Nk8SvInS+X8ItWNlQQa/KM65Hd4wb2nPH82tjAakucvS8mhJr6qU9R?=
 =?us-ascii?Q?hxEBcsbmb0TW+XeL8QVl1yEOdy2cadNeVXMFhsvyH+WUirclWe1aV+6kem9B?=
 =?us-ascii?Q?F2l+4VkG6vvywkc5kPp9Hqn9JsryxijmZ6j4403gaqihlZDsJgdIkg+oQdWC?=
 =?us-ascii?Q?5KRIYus14UoPcHDJEUCI8L6LQobhLqc7L/ezszA0cJ7eiCy2xvBtIa4OGy1I?=
 =?us-ascii?Q?SqdktUBuw6YNN8HoE1Q5znyNZg3eamFXpT68qGJmDAdHbOUXqrrbW8cfGdDG?=
 =?us-ascii?Q?TSFJnncQRTROTh+T0BJbtW8BDi+yeS/uum1x1ZVlXGlvg7RH+ZxUIxCFthRB?=
 =?us-ascii?Q?QvqcCnUkTFM4DrXm0k9PCY59hdytBs6Hs2WbQ+KQCTdqogK2G57K9haNWusM?=
 =?us-ascii?Q?6KzgceC1IvsDPLz95XD9t2CHFWZ8FoQmaHH3q3Dz9GP3U4DZkLDvKT0QdRSJ?=
 =?us-ascii?Q?QUpbO42UipGshNwz9Wp9Ru34d1BKQMG2kjogzNSeja/Ox069eJAtw/mWIRLw?=
 =?us-ascii?Q?bTn607JTX1Jf6ah6Cc/UUWKHXmWJUZHWGbx6fKYOMfmPXqIuttLTnyX5aucb?=
 =?us-ascii?Q?Bdu2/28yrAv1IkxgXsdLbHim1ITpmjtb9OAjnOw62rm2VJ5zRf2okLFJrQdT?=
 =?us-ascii?Q?9yDtRlBS7bNjRyElWZqq0+btlCTSkhw8Q1UIfUyz?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 96a05c70-4da4-4ab9-136f-08dbe9a00901
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Nov 2023 08:09:35.7874
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SyFt94AlB6Jlbtu5YaLNfRs0l7Lp1sm+rwT4xHqRZ1f+Gk5EZx7XEz8bH5V6G7fYBEmN97V2IKaaokPLbYv2Ig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8254
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 20 Nov 2023 00:09:52 -0800 (PST)

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, November 17, 2023 9:07 PM
> +
> +	hwpt =3D iommufd_hw_pagetable_get_nested(ucmd, cmd->hwpt_id);
> +	if (IS_ERR(hwpt))
> +		return PTR_ERR(hwpt);
> +
> +	rc =3D hwpt->domain->ops->cache_invalidate_user(hwpt->domain,
> &data_array,
> +						      &cmd-
> >out_driver_error_code);
> +	cmd->req_num =3D data_array.entry_num;
> +	if (iommufd_ucmd_respond(ucmd, sizeof(*cmd)))
> +		return -EFAULT;
> +	iommufd_put_object(&hwpt->obj);

the object is not put when ucmd response fails. It can be put right
after calling @cache_invalidate_user()

> @@ -309,6 +309,7 @@ union ucmd_buffer {
>  	struct iommu_hwpt_alloc hwpt;
>  	struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap;
>  	struct iommu_hwpt_set_dirty_tracking set_dirty_tracking;
> +	struct iommu_hwpt_invalidate cache;

In alphabetic order this should be put before "hwpt_set_dirty"

>  	struct iommu_ioas_alloc alloc;
>  	struct iommu_ioas_allow_iovas allow_iovas;
>  	struct iommu_ioas_copy ioas_copy;
> @@ -348,6 +349,8 @@ static const struct iommufd_ioctl_op
> iommufd_ioctl_ops[] =3D {
>  		 struct iommu_hwpt_get_dirty_bitmap, data),
>  	IOCTL_OP(IOMMU_HWPT_SET_DIRTY_TRACKING,
> iommufd_hwpt_set_dirty_tracking,
>  		 struct iommu_hwpt_set_dirty_tracking, __reserved),
> +	IOCTL_OP(IOMMU_HWPT_INVALIDATE, iommufd_hwpt_invalidate,
> +		 struct iommu_hwpt_invalidate, out_driver_error_code),

ditto

> +/**
> + * struct iommu_hwpt_invalidate - ioctl(IOMMU_HWPT_INVALIDATE)
> + * @size: sizeof(struct iommu_hwpt_invalidate)
> + * @hwpt_id: HWPT ID of a nested HWPT for cache invalidation

remove the first 'HWPT'

> + * @reqs_uptr: User pointer to an array having @req_num of cache
> invalidation
> + *             requests. The request entries in the array are of fixed w=
idth
> + *             @req_len, and contain a user data structure for invalidat=
ion
> + *             request specific to the given hardware page table.
> + * @req_type: One of enum iommu_hwpt_data_type, defining the data
> type of all
> + *            the entries in the invalidation request array. It should s=
uit
> + *            with the data_type passed per the allocation of the hwpt p=
ointed
> + *            by @hwpt_id.

"It should match the data_type given to the specified hwpt"

> + * @req_len: Length (in bytes) of a request entry in the request array
> + * @req_num: Input the number of cache invalidation requests in the arra=
y.
> + *           Output the number of requests successfully handled by kerne=
l.
> + * @out_driver_error_code: Report a driver speicifc error code upon fail=
ure.
> + *                         It's optional, driver has a choice to fill it=
 or
> + *                         not.

Being optional how does the user tell whether the code is filled or not?

> + *
> + * Invalidate the iommu cache for user-managed page table. Modifications
> on a
> + * user-managed page table should be followed by this operation to sync
> cache.
> + * Each ioctl can support one or more cache invalidation requests in the
> array
> + * that has a total size of @req_len * @req_num.
> + */
> +struct iommu_hwpt_invalidate {
> +	__u32 size;
> +	__u32 hwpt_id;
> +	__aligned_u64 reqs_uptr;
> +	__u32 req_type;
> +	__u32 req_len;
> +	__u32 req_num;
> +	__u32 out_driver_error_code;
> +};

Probably 'data_uptr', 'data_type', "entry_len' and 'entry_num" read slightl=
y
better.