Re: [PATCH 3/4] iommu/vt-d: Support PASID DMA for in-kernel usage

[Lu Baolu <baolu.lu@linux.intel.com>]

On 12/9/21 3:16 AM, Jacob Pan wrote:
> Hi Jason,
> 
> On Wed, 8 Dec 2021 09:22:55 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
>> On Tue, Dec 07, 2021 at 05:47:13AM -0800, Jacob Pan wrote:
>>> Between DMA requests with and without PASID (legacy), DMA mapping APIs
>>> are used indiscriminately on a device. Therefore, we should always match
>>> the addressing mode of the legacy DMA when enabling kernel PASID.
>>>
>>> This patch adds support for VT-d driver where the kernel PASID is
>>> programmed to match RIDPASID. i.e. if the device is in pass-through, the
>>> kernel PASID is also in pass-through; if the device is in IOVA mode, the
>>> kernel PASID will also be using the same IOVA space.
>>>
>>> There is additional handling for IOTLB and device TLB flush w.r.t. the
>>> kernel PASID. On VT-d, PASID-selective IOTLB flush is also on a
>>> per-domain basis; whereas device TLB flush is per device. Note that
>>> IOTLBs are used even when devices are in pass-through mode. ATS is
>>> enabled device-wide, but the device drivers can choose to manage ATS at
>>> per PASID level whenever control is available.
>>>
>>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>>   drivers/iommu/intel/iommu.c | 105 +++++++++++++++++++++++++++++++++++-
>>>   drivers/iommu/intel/pasid.c |   7 +++
>>>   include/linux/intel-iommu.h |   3 +-
>>>   3 files changed, 113 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 60253bc436bb..a2ef6b9e4bfc 100644
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -1743,7 +1743,14 @@ static void domain_flush_piotlb(struct
>>> intel_iommu *iommu, if (domain->default_pasid)
>>>   		qi_flush_piotlb(iommu, did, domain->default_pasid,
>>>   				addr, npages, ih);
>>> -
>>> +	if (domain->kernel_pasid && !domain_type_is_si(domain)) {
>>> +		/*
>>> +		 * REVISIT: we only do PASID IOTLB inval for FL, we
>>> could have SL
>>> +		 * for PASID in the future such as vIOMMU PT. this
>>> doesn't get hit.
>>> +		 */
>>> +		qi_flush_piotlb(iommu, did, domain->kernel_pasid,
>>> +				addr, npages, ih);
>>> +	}
>>>   	if (!list_empty(&domain->devices))
>>>   		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr,
>>> npages, ih); }
>>> @@ -5695,6 +5702,100 @@ static void intel_iommu_iotlb_sync_map(struct
>>> iommu_domain *domain, }
>>>   }
>>>   
>>> +static int intel_enable_pasid_dma(struct device *dev, u32 pasid)
>>> +{
>>
>> This seems like completely the wrong kind of op.
>>
>> At the level of the iommu driver things should be iommu_domain centric
>>
>> The op should be
>>
>> int attach_dev_pasid(struct iommu_domain *domain, struct device *dev,
>> ioasid_t pasid)
>>
>> Where 'dev' purpose is to provide the RID
>>
>> The iommu_domain passed in should be the 'default domain' ie the table
>> used for on-demand mapping, or the passthrough page table.
>>
> Makes sense. DMA API is device centric, iommu API is domain centric. It
> should be the common IOMMU code to get the default domain then pass to
> vendor drivers. Then we can enforce default domain behavior across all
> vendor drivers.
> i.e. 	
> 	dom = iommu_get_dma_domain(dev);
> 	attach_dev_pasid(dom, dev, pasid);
> 
>>> +	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
>>> +	struct device_domain_info *info;
>>
>> I don't even want to know why an iommu driver is tracking its own
>> per-device state. That seems like completely wrong layering.
>>
> This is for IOTLB and deTLB flush. IOTLB is flushed at per domain level,
> devTLB is per device.
> 
> For multi-device groups, this is a need to track how many devices are using
> the kernel DMA PASID.
> 
> Are you suggesting we add the tracking info in the generic layer? i.e.
> iommu_group.
> 
> We could also have a generic device domain info to replace what is in VT-d
> and FSL IOMMU driver, etc.

The store place of per-device iommu driver private data has already been
standardized. The iommu core provides below interfaces for this purpose:

void dev_iommu_priv_set(struct device *dev, void *priv);
void *dev_iommu_priv_get(struct device *dev);

If we have anything generic among different vendor iommu drivers,
perhaps we could move them into dev->iommu.

Best regards,
baolu