Received-SPF: pass (google.com: domain of linux-kernel+bounces-183132-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <805f3ae2-341e-4255-add8-3f6dd296a556@linux.intel.com>
Date: Sun, 19 May 2024 22:37:38 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, "iommu@lists.linux.dev"
 <iommu@lists.linux.dev>,
 "virtualization@lists.linux-foundation.org"
 <virtualization@lists.linux-foundation.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 4/9] iommufd: Add fault and response message
 definitions
To: "Tian, Kevin" <kevin.tian@intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
 Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
 Robin Murphy <robin.murphy@arm.com>,
 Jean-Philippe Brucker <jean-philippe@linaro.org>,
 Nicolin Chen <nicolinc@nvidia.com>, "Liu, Yi L" <yi.l.liu@intel.com>,
 Jacob Pan <jacob.jun.pan@linux.intel.com>,
 Joel Granados <j.granados@samsung.com>
References: <20240430145710.68112-1-baolu.lu@linux.intel.com>
 <20240430145710.68112-5-baolu.lu@linux.intel.com>
 <BN9PR11MB52762F2AF16AA5833D61AFF68CEC2@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <BN9PR11MB52762F2AF16AA5833D61AFF68CEC2@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024/5/15 15:43, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, April 30, 2024 10:57 PM
>>
>> iommu_hwpt_pgfaults represent fault messages that the userspace can
>> retrieve. Multiple iommu_hwpt_pgfaults might be put in an iopf group,
>> with the IOMMU_PGFAULT_FLAGS_LAST_PAGE flag set only for the last
>> iommu_hwpt_pgfault.
> 
> Do you envision extending the same structure to report unrecoverable
> fault in the future?

I am not envisioning extending this to report unrecoverable faults in
the future. The unrecoverable faults are not always related to a hwpt,
and therefore it's more suitable to route them through a viommu object
which is under discussion in Nicolin's series.

> 
> If yes this could be named more neutral e.g. iommu_hwpt_faults with
> flags to indicate it's a recoverable PRI request.
> 
> If it's only for PRI probably iommu_hwpt_pgreqs is clearer.
> 
>> +
>> +/**
>> + * struct iommu_hwpt_pgfault - iommu page fault data
>> + * @size: sizeof(struct iommu_hwpt_pgfault)
>> + * @flags: Combination of enum iommu_hwpt_pgfault_flags
>> + * @dev_id: id of the originated device
>> + * @pasid: Process Address Space ID
>> + * @grpid: Page Request Group Index
>> + * @perm: Combination of enum iommu_hwpt_pgfault_perm
>> + * @addr: Page address
> 
> 'Fault address'

Okay.

> 
>> + * @length: a hint of how much data the requestor is expecting to fetch. For
>> + *          example, if the PRI initiator knows it is going to do a 10MB
>> + *          transfer, it could fill in 10MB and the OS could pre-fault in
>> + *          10MB of IOVA. It's default to 0 if there's no such hint.
> 
> This is not clear to me and I don't remember PCIe spec defines such
> mechanism.

This came up in a previous discussion. While it's not currently part of
the PCI specification and may not be in the future, we'd like to add
this mechanism for potential future advanced device features as it
offers significant optimization benefits.

> 
>> +/**
>> + * enum iommufd_page_response_code - Return status of fault handlers
>> + * @IOMMUFD_PAGE_RESP_SUCCESS: Fault has been handled and the page
>> tables
>> + *                             populated, retry the access. This is the
>> + *                             "Success" defined in PCI 10.4.2.1.
>> + * @IOMMUFD_PAGE_RESP_INVALID: General error. Drop all subsequent
>> faults
>> + *                             from this device if possible. This is the
>> + *                             "Response Failure" in PCI 10.4.2.1.
>> + * @IOMMUFD_PAGE_RESP_FAILURE: Could not handle this fault, don't
>> retry the
>> + *                             access. This is the "Invalid Request" in PCI
>> + *                             10.4.2.1.
> 
> the comment for 'INVALID' and 'FAILURE' are misplaced. Also I'd more
> use the spec words to be accurate.

Yes. Fixed.

> 
>> + */
>> +enum iommufd_page_response_code {
>> +	IOMMUFD_PAGE_RESP_SUCCESS = 0,
>> +	IOMMUFD_PAGE_RESP_INVALID,
>> +	IOMMUFD_PAGE_RESP_FAILURE,
>> +};
>> +

Best regards,
baolu