Received-SPF: pass (google.com: domain of linux-kernel+bounces-57470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <de8ad0b0-1fa3-8e32-57ac-5e3e8841d884@amd.com>
Date: Thu, 8 Feb 2024 10:36:42 +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>
 <f856519f-419c-1901-b8bc-3e338873157f@amd.com>
 <9577ec59-fa05-4eea-b0ae-312d9531ce61@linux.intel.com>
From: Vasant Hegde <vasant.hegde@amd.com>
In-Reply-To: <9577ec59-fa05-4eea-b0ae-312d9531ce61@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?c0h4SmpMRVIxQ3NXTE9JS0FZS2NqTUpJTHlkWStqbkJ3cmk4UnBzOE8wbnQ1?=
 =?utf-8?B?dHVWdEFPRWlTajZIM2Ewa2svMzFyYldKTG45STIzY1c0eXBFMGhXV1oxc1kx?=
 =?utf-8?B?VUh6Y2cxazNvbmwvMHR1WmNhV2dwSzJOVUFuMjNnUzVmOElzalpOWU5kUUlD?=
 =?utf-8?B?T3VtTk1Hc0JWOHVVbVFxK2hBNm4xamR1a0hVZnpValoyRTZtTUdkbTQ5djFN?=
 =?utf-8?B?b2R0a1c3U0M3R081TUM0R2VObjlWMHZZc1hkUnVXa0ovL0JaZnk1dEFSM1NZ?=
 =?utf-8?B?YWd0Qjk3M2JhaGtOeVRNd2QxUWZXQWhUb1pNcXVHZXlzYmhPTHZwNkdDaElX?=
 =?utf-8?B?eFJzazBWMGlPdzRWd2prbjJNaTFmWVJkS1ZPNVJZdGpVczI4YXRQN21OeEN6?=
 =?utf-8?B?RG1hZ0JxUm1zMU13d0g4V3J1cDhBcUJRdm82Wi8ybWdGRGxQOUFJcGpNVEk0?=
 =?utf-8?B?TlNqMGtKVDQ0WVN1ZWFSMW9VN0xHRkdFMjFET3FpZ014MUdid3lqakhnYzJk?=
 =?utf-8?B?ekRuSWRma3FMdG5HUlhnT01BcGV1QnpzT1o4NHNyRnkyUWZQWUp3MGtkUHZ5?=
 =?utf-8?B?c1h1dk9jYWprOHFqRWlVNUF0SUxUd3FoTjlTVFh4a1kxTk84SFdkaWZVNEsz?=
 =?utf-8?B?N1lKZXFlSktiK0NyM2g4ZDh0TExXTEp5TzZ6MHpyc0E0azVCTXdvdkwxL0J6?=
 =?utf-8?B?T1oxTXdoeFdSbFNwcWp5bmZzcm8wVTlyQ2VPclpqZFV0SEVkZ2tWTmRXd1JW?=
 =?utf-8?B?aXRUK3NwMUtRK1dLMitIVjRZK1paZUhPUTZvdXduY0FrbUVURVN3ZmJhc0xO?=
 =?utf-8?B?eS81T0MzQ0NqU0J3ZUpEYm9ybEhPVHkyV2RvSTF3ZnZob3NocW5sSDZYRlpJ?=
 =?utf-8?B?SmhKdGVBZ1lYeURMVmFvTmpWc2tFdFNrQjF3NHdqV2hhczEzL1BPYjUyVE1w?=
 =?utf-8?B?UWJQR3U2eTRkUEQvVUVNeWxXYkpCMERGVVh6ejNuUjVhVFlPdFBtNDk4ajl3?=
 =?utf-8?B?K2FNcEZXclV4UFVrZmgvc1pEMiszUlpjV09DZGQ0REpoRGNqWExvT01jN3g5?=
 =?utf-8?B?Z3EzSkV2ZWQ3UWc3NTJoVWN4WlJUdGxFeTU1SFRXblpMcnpHR093OTM4dmd0?=
 =?utf-8?B?bzNtMFc0VXZLZjZZck5DNzJmOGt3b1htMm5lemtQaTBRUUtQTW15MmNjVlJC?=
 =?utf-8?B?QlM0cHNEdHZKeTJ4Z0p4YnR0bTk0ZFpoWVp3dGE4eWtSVEhTT3BOQWs4eFBh?=
 =?utf-8?B?QWFDYlEyaGYyTDM3Z2hoRzVjRHl6WG9aSENjTzRkdXY2TzFXVjQ1YUJWcG9q?=
 =?utf-8?B?VUR2U0ZFM25na2tGQ0VabWMzWE9SaXFCR2wzMWNlZUI5V1lWWHNSMHc0TUZT?=
 =?utf-8?B?a0xneWgrWkZIeUZJQnVhakZsenY1d053SEZHU3RxaUR5UGNJbmQ3V21qTnFH?=
 =?utf-8?B?a01sdDNpMTVJUGdJQlBlb0FnMTJiL2FXZVRJNFZvWFlRK1JTWTJGSkNRVXM0?=
 =?utf-8?B?bU9PYlBPbWJHZWZKSitvZHFSWExDZUxHd3FIWDBoOHRmR2Z1ZzVFWVlnNjU1?=
 =?utf-8?B?cjZpSmpVV04rb092bmx4UnRFTmlpaXgwTFZpUENIYldlWU9nS0YxYUJ6cURQ?=
 =?utf-8?B?UzM2YkI1SmZQeXNDNTFjVjF4NnRkRldEdit6d3VIUStHWFl6MGZXNEZkMFhZ?=
 =?utf-8?B?Um1EcWxXbmpBSDhzNW5DRk1YNFFGV2tIaFJLMUM5Y09BUFVCbkt1V2RiYWdW?=
 =?utf-8?B?R1QzeDlod3pVVWdBMS9ObDh3RUNNMWkrY3VQSFJxWEE5bjJib0lZZFQ1dlZi?=
 =?utf-8?B?TklNZUxZRGdZOEo1UUticlMwbm5nWjU2RFBNNnhwQkNrYWdDOUM1YXpQTHpD?=
 =?utf-8?B?U2Vud3RXS0Q5OC9EeVEzMDBMU2d2V2hYbzZ6cW50RFNDQ2w1K1RwUEMzSU5V?=
 =?utf-8?B?d3FjQzdYZUtmZVV4MXZ1ZzEvcDZnWFNRUURlelFKdS9oaTFnc0xydXhyb3V4?=
 =?utf-8?B?SHh0YnA3ZmpZKzE3OHMvVDAyN0w4Q21CMkdwY2VTaUxJZVEzNHh1akd3Vloy?=
 =?utf-8?B?dlhweUhkd3pZbFFYbWQ0M1ZSNGNHdkNHVnNRcXYvNXhncEowRUFHVVVhYjg1?=
 =?utf-8?Q?M7dnIpxgwF0SYEuYBnvzGxD7R?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5420289d-3179-4f80-b678-08dc2863c3d6
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6048.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2024 05:06:53.5662
 (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: eqPBpPWUzpTAL69F0jIDZxZTz4vQMGlHlBMEyogRhrwqBJ49ZQ+X1SiCfaEfBFsw0OoFgG5g5ZFVlT8QRUtSdg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR12MB7529


On 2/8/2024 7:02 AM, Baolu Lu wrote:
> On 2024/2/8 1:59, Vasant Hegde wrote:
>> 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.
> 
> Sure. I will make it look like below.
> 
> /**
>  * iopf_queue_remove_device - Remove producer from fault queue
>  * @queue: IOPF queue
>  * @dev: device to remove
>  *
>  * Removing a device from an iopf_queue. It's recommended to follow these
>  * steps when removing a device:
>  *
>  * - 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.
>  * - Disable PRI on the device: After calling this helper, the caller could
>  *   then disable PRI on the 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().
>  */

Looks good. Thank you.

-Vasant