Received-SPF: pass (google.com: domain of linux-kernel+bounces-11336-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <acf7dabb-d9b0-4601-868b-03cfff10a206@intel.com>
Date: Tue, 26 Dec 2023 12:00:59 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 2/9] iommufd: Add IOMMU_HWPT_INVALIDATE
Content-Language: en-US
To: "Tian, Kevin" <kevin.tian@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>, "j.granados@samsung.com" <j.granados@samsung.com>
References: <20231221153948.119007-1-yi.l.liu@intel.com>
 <20231221153948.119007-3-yi.l.liu@intel.com>
 <BN9PR11MB5276A3AFF358A0E822AD22B08C94A@BN9PR11MB5276.namprd11.prod.outlook.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <BN9PR11MB5276A3AFF358A0E822AD22B08C94A@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZHhrWGp0ZWFFci80dVozK2crMmJhc1NXaS95N0xFZWhhK29xN0JNTXZVRnhv?=
 =?utf-8?B?eGppMEJFQzk5ZDU4cGZncFk1NDg0akk3KzBzZmx6OGdoMlQ3OGVYRXJNZHdC?=
 =?utf-8?B?OXBjK0VDL01Nb1JwcENkWkZXVVhsa1FsaENDdm9EdmFqaEo1ZlJBM1FGSU0x?=
 =?utf-8?B?bGhFVGxvamsrUndJMmw2UXh6eHJVeHlYM1BURjNOd2Nmd1AyV0ZxNGFjeVZ5?=
 =?utf-8?B?QVRtVnRBUWorWWdpQnlUd0hSL2FRbmNTaUNtVGdNSDlxcXJyNEZKdW9pNWxB?=
 =?utf-8?B?MU45VjV5eFFuTGEwK21yVnZBVWtDSmNFeDJYTTZ4TmtYcGxKVWdtMkNqOUJO?=
 =?utf-8?B?azJPMHlJa204QVg5K0RodmlDd21qc2x3SGpXVjFVWFdvZ2FrYk5QaCttQXlt?=
 =?utf-8?B?dlNOblVFR3pUWUh1a1MyeW9veXBtWXJDRG9pOTFZZXVETk5Rb29YdUpRWjJs?=
 =?utf-8?B?S1J1Uk9Pd0c0VDlBM2xHSXh5YzNTSHk0UUpZYVlaS3owRzBSaGN3WXJKVWFI?=
 =?utf-8?B?REIxa3F2eFROdVNudzNwTXpVYXh1TkVuSVVHbW9OZzQ0ZTgrMndtRTI4ZnVz?=
 =?utf-8?B?aEg4QnQ2U1dKLzYyMzlvazBQNVdsSkw2VmhVb3dsQy81eUlnOE9YclJUY3pL?=
 =?utf-8?B?WStrWmJVaXFja2F6VWV6em9lS3NONHlDQ2pNejRWYjUwN0dwRmIydzRyODJz?=
 =?utf-8?B?V3FhL0xuMWVFMjRnT1gzWk82UkhLeC9NVmpCbnJEV0xJWHB1citNWEM5Mk84?=
 =?utf-8?B?aGtCRXRwMUZKeUVjWE5UT0N2Tnh4T0ZtWS81T1BIM0VXOStCcUZNTTMxYzBE?=
 =?utf-8?B?aXlDYUowU1VEZG0wQTZWckJ4dzl3bzBGWExzUm5YZ2IxbGNTUHV6aE5jUkY5?=
 =?utf-8?B?b1hxaWtoNkluMjVMdDUxRFVvUEREVXpUL2V3Rm8vWkJxZ1JmZGJWMFh3UVFC?=
 =?utf-8?B?bWxONUc2d1RGbkNDVm41MUxuN2tsMEZmWGlzaWpyaE5qSm5nZ0M0Tm4xL2t6?=
 =?utf-8?B?OU03ejdsRVhnb296bFZzTGQ0UUFyK25PcTN0VGVKSFpwcjRBWHE2aDlmaFZR?=
 =?utf-8?B?N2VVSy92ZGZ3SUZZWmw0NkM2RmVSdTdmbnpvRzRWd01hS2JWb1JRT3k0ckRW?=
 =?utf-8?B?b1YzREhCaHdYRjVibHhadGFqbzRjeDZGWSs0d09VcWNLNXYxZG5RSzk1V24r?=
 =?utf-8?B?a3huZ0RzMUdKTXh3bWdRNjI1NUV0blJ3Q0gvbjdFQWZmM0dZK2VFSXdZdTZC?=
 =?utf-8?B?YWNPWHdEWFNRUXpuQUZMSC9oYnpnVG1SZXE0cTBhRTRxSUYyQmZHUU9CY3lL?=
 =?utf-8?B?MzJQQXZ3a3pSNVZnK0lPTUhtWngxejJhblgzZ1pGT2RhcDczbklNSXgxMnhW?=
 =?utf-8?B?NXFkYmJTUGRlUmRQclJTUHRQMGdUSjlOek9zOUt2am5SVEFoTmRHUk40UmdR?=
 =?utf-8?B?S0ZiZ3o1TE1pd0pUVUliUXA0MlQ1Y1QwbEJjV2x0c0gwd0NMaTdpaHl5WEs0?=
 =?utf-8?B?dEdDUmo4dzZIeFUvWTBiYzR2d3dhS2c0aHVXQ1dxdGdjNnBHMldaMFp3cmxG?=
 =?utf-8?B?ell3ci93SXF2dWlWd0t4NmlwTUdQSG5Qc21FZ1c1aGIycjdWMGxmSTBIVkRF?=
 =?utf-8?B?eHlZa0h5OEl6N2FIWEcraE8vTE43aEpPa3psQ2orczNPSjZTdGVzcm9tWjZS?=
 =?utf-8?B?QzdZTkdjdldYUWV3Vy9oUFp4VGl4VHBNbU83bzZtcEduMnQ3NUh6VzIrMTF5?=
 =?utf-8?B?T3ZHSkJlVCtvY0IvcWo3TmNQdVQ0UE1nRGhZSWlpSVNYc0ZWUkMvendRMUdt?=
 =?utf-8?B?SlM1QXRSL05idGJXd0hOem9RQkdoWWlOMW8zK1lBTStQVWJMWnB6YWpjeDRG?=
 =?utf-8?B?cTdRaHNrUytaQ3JIOFlFT3RPT0ltdHUycHJQeURBYWZXR2RJcTRWaENNYjZ5?=
 =?utf-8?B?cFZYN1VOcGI3dVJmVEVLN3BnSDFPUEc0ekpCKzAySmFVaEx4TGg3YjZTUE5I?=
 =?utf-8?B?NUp6bWNLU09FK0xhWEZwM1lmQ0NZRWhZTmZza1FiZEYvMEFhUEE5bTk1QUJR?=
 =?utf-8?B?L2ttWm5zZmVrTjJxYkg2Zmh1aHg4OWtBU0NWVFlKaHVsUWJ1cFhYV0Q3UCtL?=
 =?utf-8?Q?PLSCfoIDg6YAZmVVVqSX4UHNU?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e9d3be7b-b0c9-4f1b-02e2-08dc05c6e153
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Dec 2023 03:58:12.9200
 (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: /IjoTlFLhm8Ep8ssnP0N/Ao7ZPdzZGI3rxMCFkZ2py9wQ60VaqTkLY8dVKhVyS3j4EjXDWNxmiPD7wkWM/ufMA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5920
X-OriginatorOrg: intel.com

On 2023/12/22 11:19, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 21, 2023 11:40 PM
>> +
>> +int iommufd_hwpt_invalidate(struct iommufd_ucmd *ucmd)
>> +{
>> +	struct iommu_hwpt_invalidate *cmd = ucmd->cmd;
>> +	struct iommu_user_data_array data_array = {
>> +		.type = cmd->req_type,
>> +		.uptr = u64_to_user_ptr(cmd->reqs_uptr),
>> +		.entry_len = cmd->req_len,
>> +		.entry_num = cmd->req_num,
>> +	};
>> +	struct iommufd_hw_pagetable *hwpt;
>> +	u32 done_num = 0;
>> +	int rc;
>> +
>> +	if (cmd->req_num && (!cmd->reqs_uptr || !cmd->req_len)) {
>> +		rc = -EINVAL;
>> +		goto out;
>> +	}
> 
> miss a check on the __reserved field.

done.

> 
>> @@ -323,6 +323,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;
> 
> this should be in alphabetic order. I gave this comment in v6 too:
> 
> https://lore.kernel.org/linux-iommu/BN9PR11MB5276D8406BF08B853329288C8CB4A@BN9PR11MB5276.namprd11.prod.outlook.com/

oops, I did miss it. sigh, I paid too much attention to the error code
discussion in the end. :( I've revisited the comments and should have
done now.

>> +/**
>> + * enum iommu_hwpt_invalidate_data_type - IOMMU HWPT Cache
>> Invalidation
>> + *                                        Data Type
>> + * @IOMMU_HWPT_INVALIDATE_DATA_VTD_S1: Invalidation data for
>> VTD_S1
>> + */
>> +enum iommu_hwpt_invalidate_data_type {
>> +	IOMMU_HWPT_INVALIDATE_DATA_VTD_S1,
>> +};
> 
> Defining DATA_VTD_S1 at this point is fine, if there is no usage on
> DATA_NONE. But following vtd specific definitions should be moved
> to the later vtd specific patches. they are not used by the common
> code anyway.

sure.

>> +
>> +/**
>> + * enum iommu_hwpt_vtd_s1_invalidate_flags - Flags for Intel VT-d
>> + *                                           stage-1 cache invalidation
>> + * @IOMMU_VTD_INV_FLAGS_LEAF: The LEAF flag indicates whether only
>> the
>> + *                            leaf PTE caching needs to be invalidated
>> + *                            and other paging structure caches can be
>> + *                            preserved.
> 
> "indicates whether the invalidation applies to all-levels page structure
> cache or just the leaf PTE cache"

done.

>> + */
>> +enum iommu_hwpt_vtd_s1_invalidate_flags {
>> +	IOMMU_VTD_INV_FLAGS_LEAF = 1 << 0,
>> +};
>> +
>> +/**
>> + * enum iommu_hwpt_vtd_s1_invalidate_error - Result of invalidation
> 
> "hardware error of invalidation"

done.

> 
>> + * @IOMMU_HWPT_INVALIDATE_VTD_S1_ICE: Invalidation Completion
>> Error, details
>> + *                                     refer to 11.4.7.1 Fault Status Register
>> + *                                     of VT-d specification.
>> + * @IOMMU_HWPT_INVALIDATE_VTD_S1_ITE: Invalidation Time-out Error,
>> details
>> + *                                     refer to 11.4.7.1 Fault Status Register
>> + *                                     of VT-d specification.
>> + */
>> +enum iommu_hwpt_vtd_s1_invalidate_error {
>> +	IOMMU_HWPT_INVALIDATE_VTD_S1_ICE = 1 << 0,
>> +	IOMMU_HWPT_INVALIDATE_VTD_S1_ITE = 1 << 1,
>> +};
>> +
>> +/**
>> + * struct iommu_hwpt_vtd_s1_invalidate - Intel VT-d cache invalidation
>> + *                                       (IOMMU_HWPT_INVALIDATE_DATA_VTD_S1)
>> + * @addr: The start address of the addresses to be invalidated. It needs
>> + *        to be 4KB aligned.
> 
> 'of the range'

done.

> 
>> + * @npages: Number of contiguous 4K pages to be invalidated.
>> + * @flags: Combination of enum iommu_hwpt_vtd_s1_invalidate_flags
>> + * @inv_error: One of enum iommu_hwpt_vtd_s1_invalidate_error
> 
> '@hw_error'

done.

>> + *
>> + * The Intel VT-d specific invalidation data for user-managed stage-1 cache
>> + * invalidation in nested translation. Userspace uses this structure to
>> + * tell the impacted cache scope after modifying the stage-1 page table.
>> + *
>> + * Invalidating all the caches related to the page table by setting @addr
>> + * to be 0 and @npages to be U64_MAX.
> 
> here should clarify that the invalidation applies to device TLB automatically
> for VT-d.

done.

>> + *
>> + * @inv_error is meaningful only if the request is handled by kernel. This
>> + * can be known by checking struct iommu_hwpt_invalidate::req_num
>> output.
>> + * @inv_error only covers the errors detected by hardware after submitting
>> the
>> + * invalidation. The software detected errors would go through the normal
>> + * ioctl errno.
>> + */
>> +struct iommu_hwpt_vtd_s1_invalidate {
>> +	__aligned_u64 addr;
>> +	__aligned_u64 npages;
>> +	__u32 flags;
>> +	__u32 inv_error;
>> +};
>> +
>> +/**
>> + * 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
>> + * @reqs_uptr: User pointer to an array having @req_num of cache
>> invalidation
>> + *             requests. The request entries in the array are of fixed width
>> + *             @req_len, and contain a user data structure for invalidation
>> + *             request specific to the given hardware page table.
> 
> Just:
> 
> 'User pointer to an array of driver-specific cache invalidation requests'

done.

>> + * @req_type: One of enum iommu_hwpt_invalidate_data_type, defining
>> the data
>> + *            type of all the entries in the invalidation request array. It
>> + *            should be a type supported by the hwpt pointed by @hwpt_id.
>> + * @req_len: Length (in bytes) of a request entry in the request array
>> + * @req_num: Input the number of cache invalidation requests in the array.

s/req_len/entry_len, s/req_num/entry_num per the comments in last version.

>> + *           Output the number of requests successfully handled by kernel.
>> + * @__reserved: Must be 0.
>> + *
>> + * 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.
>> + *
>> + * An empty invalidation request array by setting @req_num==0 is allowed,
>> and
>> + * @req_len and @reqs_uptr would be ignored in this case. This can be
>> used to
>> + * check if the given @req_type is supported or not by kernel.
>> + */
>> +struct iommu_hwpt_invalidate {
>> +	__u32 size;
>> +	__u32 hwpt_id;
>> +	__aligned_u64 reqs_uptr;
>> +	__u32 req_type;
>> +	__u32 req_len;
>> +	__u32 req_num;
>> +	__u32 __reserved;
>> +};
>> +#define IOMMU_HWPT_INVALIDATE _IO(IOMMUFD_TYPE,
>> IOMMUFD_CMD_HWPT_INVALIDATE)
>>   #endif
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu