linux-kernel - Re: [PATCH v6 3/4] iommu/vt-d: Add set_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 May 2023 10:19:05 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...dia.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:     baolu.lu@...ux.intel.com, LKML <linux-kernel@...r.kernel.org>,
        iommu@...ts.linux.dev, Joerg Roedel <joro@...tes.org>,
        dmaengine@...r.kernel.org, vkoul@...nel.org,
        Robin Murphy <robin.murphy@....com>,
        Will Deacon <will@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Raj Ashok <ashok.raj@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        "Zanussi, Tom" <tom.zanussi@...el.com>,
        narayan.ranganathan@...el.com
Subject: Re: [PATCH v6 3/4] iommu/vt-d: Add set_dev_pasid callback for dma
 domain

On 5/30/23 3:48 AM, Jason Gunthorpe wrote:
> On Fri, May 19, 2023 at 01:32:22PM -0700, Jacob Pan wrote:
> 
>> @@ -4720,25 +4762,99 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
>>   static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>   {
>>   	struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
>> +	struct dev_pasid_info *curr, *dev_pasid = NULL;
>> +	struct dmar_domain *dmar_domain;
>>   	struct iommu_domain *domain;
>> +	unsigned long flags;
>>   
>> -	/* Domain type specific cleanup: */
>>   	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
>> -	if (domain) {
>> -		switch (domain->type) {
>> -		case IOMMU_DOMAIN_SVA:
>> -			intel_svm_remove_dev_pasid(dev, pasid);
>> -			break;
>> -		default:
>> -			/* should never reach here */
>> -			WARN_ON(1);
>> +	if (!domain)
>> +		goto out_tear_down;
>> +
>> +	/*
>> +	 * The SVA implementation needs to stop mm notification, drain the
>> +	 * pending page fault requests before tearing down the pasid entry.
>> +	 * The VT-d spec (section 6.2.3.1) also recommends that software
>> +	 * could use a reserved domain id for all first-only and pass-through
>> +	 * translations. Hence there's no need to call domain_detach_iommu()
>> +	 * in the sva domain case.
>> +	 */
>> +	if (domain->type == IOMMU_DOMAIN_SVA) {
>> +		intel_svm_remove_dev_pasid(dev, pasid);
>> +		goto out_tear_down;
>> +	}
> 
> But why don't you need to do all the other
> intel_pasid_tear_down_entry(), intel_svm_drain_prq() (which is
> misnamed) and other stuff from intel_svm_remove_dev_pasid() ?

Perhaps,

	if (domain->type == IOMMU_DOMAIN_SVA) {
		intel_svm_remove_dev_pasid(dev, pasid);
		return;
	}

?

> 
> There still seems to be waaay too much "SVM" in the PASID code.

This segment of code is destined to be temporary. From a long-term
perspective, I hope to move SVA specific staffs such as mm notification,
prq draining, etc. to the iommu core. They are generic rather than Intel
iommu specific.

After the code consolidation done, the code here will become simpler and
appealing. We just need to tear down the pasid entry.

> 
>> +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>> +				     struct device *dev, ioasid_t pasid)
>> +{
>> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
>> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> +	struct intel_iommu *iommu = info->iommu;
>> +	struct dev_pasid_info *dev_pasid;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
>> +		return -EOPNOTSUPP;
>> +
>> +	if (context_copied(iommu, info->bus, info->devfn))
>> +		return -EBUSY;
>> +
>> +	ret = prepare_domain_attach_device(domain, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>> +	if (!dev_pasid)
>> +		return -ENOMEM;
>> +
>> +	ret = domain_attach_iommu(dmar_domain, iommu);
>> +	if (ret)
>> +		goto out_free;
>> +
>> +	if (domain_type_is_si(dmar_domain))
>> +		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
>> +						     dev, pasid);
>> +	else if (dmar_domain->use_first_level)
>> +		ret = domain_setup_first_level(iommu, dmar_domain,
>> +					       dev, pasid);
>> +	else
>> +		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>> +						     dev, pasid);
> 
> It would be nice if the different domain types had their own ops..

Good suggestion!

We can add a domain ops in the Intel domain structure which is
responsible for how to install an Intel iommu domain onto the VT-d
hardware.

It worth a separated refactoring series. Let me do it afterward.

Best regards,
baolu