linux-kernel - Re: [PATCH v8 02/11] iommu: Add max_pasids field in struct 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]

Message-ID: <f92629ef-8c9a-b89a-ef0b-32e71617bd51@linux.intel.com>
Date:   Fri, 10 Jun 2022 14:46:52 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     "Raj, Ashok" <ashok.raj@...el.com>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 02/11] iommu: Add max_pasids field in struct dev_iommu

On 2022/6/10 03:01, Raj, Ashok wrote:
> On Tue, Jun 07, 2022 at 09:49:33AM +0800, Lu Baolu wrote:
>> Use this field to save the number of PASIDs that a device is able to
>> consume. It is a generic attribute of a device and lifting it into the
>> per-device dev_iommu struct could help to avoid the boilerplate code
>> in various IOMMU drivers.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>>   include/linux/iommu.h |  2 ++
>>   drivers/iommu/iommu.c | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 03fbb1b71536..d50afb2c9a09 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -364,6 +364,7 @@ struct iommu_fault_param {
>>    * @fwspec:	 IOMMU fwspec data
>>    * @iommu_dev:	 IOMMU device this device is linked to
>>    * @priv:	 IOMMU Driver private data
>> + * @max_pasids:  number of PASIDs device can consume
>>    *
>>    * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
>>    *	struct iommu_group	*iommu_group;
>> @@ -375,6 +376,7 @@ struct dev_iommu {
>>   	struct iommu_fwspec		*fwspec;
>>   	struct iommu_device		*iommu_dev;
>>   	void				*priv;
>> +	u32				max_pasids;
>>   };
>>   
>>   int iommu_device_register(struct iommu_device *iommu,
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 847ad47a2dfd..adac85ccde73 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/idr.h>
>>   #include <linux/err.h>
>>   #include <linux/pci.h>
>> +#include <linux/pci-ats.h>
> 
> Is this needed for this patch?

Yes. It's for pci_max_pasids().

> 
>>   #include <linux/bitops.h>
>>   #include <linux/property.h>
>>   #include <linux/fsl/mc.h>
>> @@ -218,6 +219,30 @@ static void dev_iommu_free(struct device *dev)
>>   	kfree(param);
>>   }
>>   
>> +static u32 dev_iommu_get_max_pasids(struct device *dev)
>> +{
>> +	u32 max_pasids = dev->iommu->iommu_dev->max_pasids;
>> +	u32 num_bits;
>> +	int ret;
>> +
>> +	if (!max_pasids)
>> +		return 0;
>> +
>> +	if (dev_is_pci(dev)) {
>> +		ret = pci_max_pasids(to_pci_dev(dev));
>> +		if (ret < 0)
>> +			return 0;
>> +
>> +		return min_t(u32, max_pasids, ret);
> 
> Ah.. that answers my other question to consider device pasid-max. I guess
> if we need any enforcement of restricting devices that aren't supporting
> the full PASID, that will be done by some higher layer?

The mm->pasid style of SVA is explicitly enabled through
iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA). The IOMMU driver specific
restriction might be put there?

> 
> too many returns in this function, maybe setup all returns to the end of
> the function might be elegant?

I didn't find cleanup room after a quick scan of the code. But sure, let
me go through code again offline.

> 
>> +	}
>> +
>> +	ret = device_property_read_u32(dev, "pasid-num-bits", &num_bits);
>> +	if (ret)
>> +		return 0;
>> +
>> +	return min_t(u32, max_pasids, 1UL << num_bits);
>> +}
>> +
>>   static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
>>   {
>>   	const struct iommu_ops *ops = dev->bus->iommu_ops;
>> @@ -243,6 +268,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>>   	}
>>   
>>   	dev->iommu->iommu_dev = iommu_dev;
>> +	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
>>   
>>   	group = iommu_group_get_for_dev(dev);
>>   	if (IS_ERR(group)) {
>> -- 
>> 2.25.1
>>

Best regards,
Baolu