Re: [PATCH v3 12/16] iommu/amd: Add support for different types of PPR handler

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Jason,


On 8/4/2023 6:58 PM, Jason Gunthorpe wrote:
> On Fri, Aug 04, 2023 at 06:42:12AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> For AMD IOMMU, the PPR feature is needed to support IO page fault (IOPF).
>> PPR is enabled per PCI end-point device, and is configured by the PPR bit
>> in the IOMMU device table entry (i.e DTE[PPR]).
>>
>> Currently, PPR is enabled for a device when it is initialized for AMD
>> IOMMU v2API. To support paging for IOPF, the DTE[PPR] bit needs to be
>> updated when enable/disable IOPF feature.
>>
>> Introducing struct iommu_dev_data.ppr and enum ppr_handlers to track PPR
>> setting for each device.
>>
>> Finally iommu_dev_data.ppr is set only when IOMMU supports PPR. Hence
>> remove redundant feature support check in set_dte_entry().
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h |  6 ++++++
>>  drivers/iommu/amd/iommu.c           | 10 ++++------
>>  drivers/iommu/amd/iommu_v2.c        | 14 +++++++++++++-
>>  3 files changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index b79c6e10684d..443b13b56235 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -803,6 +803,11 @@ struct devid_map {
>>  	bool cmd_line;
>>  };
>>  
>> +enum ppr_handlers {
>> +	PPR_HANDLER_NONE,	/* No handler specified */
>> +	PPR_HANDLER_V2API,	/* IOMMU v2 API ppr handler */
>> +};
>> +
>>  /*
>>   * This struct contains device specific data for the IOMMU
>>   */
>> @@ -820,6 +825,7 @@ struct iommu_dev_data {
>>  	u8 ats_enabled  :1;		  /* ATS state */
>>  	u8 pri_tlp      :1;		  /* PASID TLB required for
>>  					     PPR completions */
>> +	enum ppr_handlers ppr;
>>  	bool use_vapic;			  /* Enable device to use vapic mode */
>>  	bool defer_attach;
>>  
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 5968eb8d265c..888bce5abe56 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1634,10 +1634,8 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
>>  	if (ats)
>>  		flags |= DTE_FLAG_IOTLB;
>>  
>> -	if (ppr) {
>> -		if (iommu_feature(iommu, FEATURE_EPHSUP))
>> -			pte_root |= 1ULL << DEV_ENTRY_PPR;
>> -	}
>> +	if (ppr)
>> +		pte_root |= 1ULL << DEV_ENTRY_PPR;
>>  
>>  	if (domain->flags & PD_IOMMUV2_MASK) {
>>  		u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
>> @@ -1730,7 +1728,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
>>  
>>  	/* Update device table */
>>  	set_dte_entry(iommu, dev_data->devid, domain,
>> -		      ats, dev_data->iommu_v2);
>> +		      ats, dev_data->ppr);
> 
> This is casting an enum to a bool, 
> 
> dev_data->ppr != PPR_HANDLER_NONE
> 
> ?
> 
> And in other places
> 
> Or maybe just pass in the enum.

My bad. I missed it. I will pass enum in next iteration.

-Vasant