Received-SPF: pass (google.com: domain of linux-kernel+bounces-56881-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f856519f-419c-1901-b8bc-3e338873157f@amd.com>
Date: Wed, 7 Feb 2024 23:29:46 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH v12 13/16] iommu: Improve iopf_queue_remove_device()
Content-Language: en-US
To: Baolu Lu <baolu.lu@linux.intel.com>, "Tian, Kevin"
 <kevin.tian@intel.com>, Joerg Roedel <joro@8bytes.org>,
 Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
 Jason Gunthorpe <jgg@ziepe.ca>,
 Jean-Philippe Brucker <jean-philippe@linaro.org>,
 Nicolin Chen <nicolinc@nvidia.com>
Cc: "Liu, Yi L" <yi.l.liu@intel.com>,
 Jacob Pan <jacob.jun.pan@linux.intel.com>,
 Longfang Liu <liulongfang@huawei.com>, "Zhao, Yan Y" <yan.y.zhao@intel.com>,
 Joel Granados <j.granados@samsung.com>,
 "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 Jason Gunthorpe <jgg@nvidia.com>
References: <20240207013325.95182-1-baolu.lu@linux.intel.com>
 <20240207013325.95182-14-baolu.lu@linux.intel.com>
 <BN9PR11MB527603AB5685FF3ED21647958C452@BN9PR11MB5276.namprd11.prod.outlook.com>
 <693ee23d-30c6-4824-9bb2-1cfbf2eccfef@linux.intel.com>
From: Vasant Hegde <vasant.hegde@amd.com>
In-Reply-To: <693ee23d-30c6-4824-9bb2-1cfbf2eccfef@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TTVjSDIwSytLaEhhVFhlMEU0RnVWRWxXKzVEOGk4bERmMVM2dU9VbFdITXpF?=
 =?utf-8?B?VHJWQ2J4RzlOcHFuOFcwUXZJYTZMbStNQXFYYWt3dXdqUTFqRHFCQUMzYTJt?=
 =?utf-8?B?eDdYODRMcDNGclIxaEMwZWM1cUVhMjh2ci9MTjI4S3NFZXB4bTFYbVFNOUky?=
 =?utf-8?B?RTRWOGgzUUpsbVRpY1lCZ2dBaytvSi9LN1c4T0FjTW1nUHBZMDc3d3VSNjQv?=
 =?utf-8?B?ME5DbGJsR2ViV3Z6VkFCZzRqK1I4ZjJCY1krWW9oTlFkdnJHOVJWWGovcUFX?=
 =?utf-8?B?eE96dXljVXgrUHZoaStKUU9wcGs0Q2doOEdOK3loMWRBcktPcTBEOEVMcWEv?=
 =?utf-8?B?TStzNWN5aGZpZzFLSGVlTThrUUlyTzZSSmN3TWZkN0JrKzdEcDZGL2I1aTJw?=
 =?utf-8?B?VDVxK29CZXJvSUsvMTJGc1d2SFN0bUdXTWNZQWw3Z3pRYU0wRFJWOXh1MXBw?=
 =?utf-8?B?QUVRWmVUbXh3VW1Bdks0OUdZMXFiSHBvZkg0enRvZllnOVlNS0dQU2F2WHFG?=
 =?utf-8?B?OXZzaXd2VjBKa0RzY0VKLzhoRGdRMGZDY3d4d3FoMDRUQ0p4eEtWUmZaVnVT?=
 =?utf-8?B?VUdSRzduRmNHdnlFbEZCMWpGaFh0TGFHOXlqeENXZk1yNFg4eE5HVDdjT3h5?=
 =?utf-8?B?TG00RjJOVmttbEZpalRhNFplQ3JyVHlYdnVZR0Z4V1h6Tk1ia2ZRTDNIbHFr?=
 =?utf-8?B?NGZHVEJIYjk3MnlKRG9SSW85elpLZTdnSko4a1hzbWpoMlpLSWxnREc3UGRT?=
 =?utf-8?B?WkhicWx6WDhlS1hyNjdmaVlPY2Z4VmtRZE5KRE84dVFKZWYwdUZ5b3ZuajJH?=
 =?utf-8?B?dEVqdHh2RDgwWnRsTmZXcURFMlFIcGFYRmVicUg2K1BGRDZJbDVmNzNpZE95?=
 =?utf-8?B?UTVFTE1DdEJzZktUdTFob2VReDBlci9xU1N5Y3V5Uy8zUUFneExzYUYvRU12?=
 =?utf-8?B?VlZ3Rm1GNFhOa09nRHNhZndjWllZbTBKMjN1SG0rdFJWckVtaHA4V2YwY0Zr?=
 =?utf-8?B?OGVodktGVm1GSStYTEE4Ykhza2tFZWN5VUdJdDhUU0xxOUJ2cCtZaURZN0o0?=
 =?utf-8?B?cWV1aHFHRGl4Sno1TjZ4QW54Tkw3TjVFUU1sTUVRZ0QveVByMkllS053bUxl?=
 =?utf-8?B?dVc1d0ttR0xRVWdmOVZodzd1RGMwc1MyT0ZuUWdFRmM4QW5QT3FDamdYQlZy?=
 =?utf-8?B?UkZ4RkpFLzVnenpDZnZqMHVuc2RDMWxINWQ2OVNPQmxnOGhMTXBGTlAvb1ln?=
 =?utf-8?B?YlF6K2hSMVBweTdrNlVSZTFwR3hzcGN3M21TWWZDdlZRS2tiSzNBcWhSZERD?=
 =?utf-8?B?c1YyUUErczhITjk0R2NRZGZTczFTbUx6Wk1mZFd3dFdncjFZRUJQQjVJZzFx?=
 =?utf-8?B?b0dBV0RPRUJoQjVZbjVJaWNya1RZdWJrRHlHRlJRUXdIcC9tYnk0eVNtbmhH?=
 =?utf-8?B?Y2pCb09KNVVialUzWG10Nk9sV0E1aDFDcVJaY3FmU04raXoxWVN4YlNSZ2Q0?=
 =?utf-8?B?bktqOEFKaldlVm1HRkVnWHFvRHVHVnlVZVhGV244SGhzb2llUlp2ME1oYXFk?=
 =?utf-8?B?a2VsRFM4em44bCtsN2pOM3YvZVhHRWZqSXU4OGQzVmdOU1NQTmhSejNtVnY5?=
 =?utf-8?B?R005aW95dTlnZStjVm1lRkVWSGZtclk5K2dkaFdHNU5Hd082ZXk0cWRENEJp?=
 =?utf-8?B?Qm1OOHBIQWs3dmdib0ZDdEdCM2JpeVhvTUNEaWdJMzlxZk5qNE95L3pXOUJq?=
 =?utf-8?B?bGRpSEZ5bllSUmVJMTJoWWxjRE9vRUVWR1lZRDJBeWwvS2k4dHgyY0drbnNY?=
 =?utf-8?B?cm5lVjM1MDZDcmR6TXdIbkh4SkptTldoSlNNTmFPVTdUbTFNdHVjUmFCak9o?=
 =?utf-8?B?NGNIVXh0OVYzOTlMYmNqV2E0Z1IrVlp4SDRxNlJLUzJjU0ZyeWVkTXh5VmZL?=
 =?utf-8?B?VU9YSS93a0xNb2NEUnkxczJiL1RJa2hSN21qU1pvNlpLSUVoZXJvWm5ucnAr?=
 =?utf-8?B?VGQ2SGxVdFlYUWlhdnFpRk5vcjhPbDZHUk82eXR6ekgvZ0FzZERkU2dWNXJk?=
 =?utf-8?B?eUlQSStxbnBXT3pOUTF3a3d1dmdzTmsxcmxWUk9hb2ttQVk5TWdreElNeE4x?=
 =?utf-8?Q?ccODzAMAuM4JGY0NqHX3MeV8K?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5bc3b249-834e-4ea0-72b9-08dc280697d6
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6048.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Feb 2024 17:59:56.5717
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SHthocRSebv26qI3414ozSrwBJ1tjRM1w7DUj1ZC3boJoUHLXWnlbtM6T6LNMLVQ3deCLgpG5+YLexzmczPxsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5123

Hi Baolu,

On 2/7/2024 5:59 PM, Baolu Lu wrote:
> On 2024/2/7 10:50, Tian, Kevin wrote:
>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>> Sent: Wednesday, February 7, 2024 9:33 AM
>>>
>>> Convert iopf_queue_remove_device() to return void instead of an error code,
>>> as the return value is never used. This removal helper is designed to be
>>> never-failed, so there's no need for error handling.
>>>
>>> Ack all outstanding page requests from the device with the response code of
>>> IOMMU_PAGE_RESP_INVALID, indicating device should not attempt any retry.
>>>
>>> Add comments to this helper explaining the steps involved in removing a
>>> device from the iopf queue and disabling its PRI. The individual drivers
>>> are expected to be adjusted accordingly. Here we just define the expected
>>> behaviors of the individual iommu driver from the core's perspective.
>>>
>>> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
>>> Tested-by: Yan Zhao<yan.y.zhao@intel.com>
>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>, with one nit:
>>
>>> + * Removing a device from an iopf_queue. It's recommended to follow
>>> these
>>> + * steps when removing a device:
>>>    *
>>> - * Return: 0 on success and <0 on error.
>>> + * - Disable new PRI reception: Turn off PRI generation in the IOMMU
>>> hardware
>>> + *   and flush any hardware page request queues. This should be done
>>> before
>>> + *   calling into this helper.
>>> + * - Acknowledge all outstanding PRQs to the device: Respond to all
>>> outstanding
>>> + *   page requests with IOMMU_PAGE_RESP_INVALID, indicating the device
>>> should
>>> + *   not retry. This helper function handles this.
>> this implies calling iopf_queue_remove_device() here.
>>
>>> + * - Disable PRI on the device: After calling this helper, the caller could
>>> + *   then disable PRI on the device.
>>> + * - Call iopf_queue_remove_device(): Calling iopf_queue_remove_device()
>>> + *   essentially disassociates the device. The fault_param might still exist,
>>> + *   but iommu_page_response() will do nothing. The device fault parameter
>>> + *   reference count has been properly passed from
>>> iommu_report_device_fault()
>>> + *   to the fault handling work, and will eventually be released after
>>> + *   iommu_page_response().
>>>    */
>> but here it suggests calling iopf_queue_remove_device() again. If the comment
>> is just about to detail the behavior with that invocation shouldn't it be merged
>> with the previous one instead of pretending to be the final step for driver
>> to call?
> 
> Above just explains the behavior of calling iopf_queue_remove_device().

Can you please leave a line -OR- move this to previous para? Otherwise we will
get confused.

-Vasant