[1/4] iommu/vt-d: Introduce dev_to_iommu()
Commit Message
The device_to_iommu() helper was originally designed to look up the DMAR
ACPI table to retrieve the iommu device and the request ID for a given
device. However, it was also being used in other places where there was
no need to lookup the ACPI table at all.
Introduce a new helper dev_to_iommu() which returns the iommu device saved
in the per-device iommu driver data. This function can be used after the
iommu_probe_device() returns success.
Rename the original device_to_iommu() function to a more meaningful name,
device_lookup_iommu(), to avoid mis-using it.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.h | 11 ++++++++++-
drivers/iommu/intel/iommu.c | 28 ++++++++--------------------
drivers/iommu/intel/svm.c | 20 +++-----------------
3 files changed, 21 insertions(+), 38 deletions(-)
Comments
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, November 16, 2023 9:51 AM
>
> +static inline struct intel_iommu *dev_to_iommu(struct device *dev)
> +{
> + /*
> + * Assume that valid per-device iommu structure must be installed
> + * if iommu_probe_device() has succeeded. This helper could only
> + * be used after device is probed.
> + */
> + return ((struct device_domain_info *)dev_iommu_priv_get(dev))-
> >iommu;
> +}
Not sure whether this helper is useful. This is only used by 2 out of 5
post-probe users. Probably just open-coding in all 5 places is clearer.
On 11/16/23 11:19 AM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, November 16, 2023 9:51 AM
>>
>> +static inline struct intel_iommu *dev_to_iommu(struct device *dev)
>> +{
>> + /*
>> + * Assume that valid per-device iommu structure must be installed
>> + * if iommu_probe_device() has succeeded. This helper could only
>> + * be used after device is probed.
>> + */
>> + return ((struct device_domain_info *)dev_iommu_priv_get(dev))-
>>> iommu;
>> +}
>
> Not sure whether this helper is useful. This is only used by 2 out of 5
> post-probe users. Probably just open-coding in all 5 places is clearer.
I thought it should get more users in the future development.
Best regards,
baolu
On Thu, Nov 16, 2023 at 11:23:56AM +0800, Baolu Lu wrote:
> On 11/16/23 11:19 AM, Tian, Kevin wrote:
> > > From: Lu Baolu <baolu.lu@linux.intel.com>
> > > Sent: Thursday, November 16, 2023 9:51 AM
> > >
> > > +static inline struct intel_iommu *dev_to_iommu(struct device *dev)
> > > +{
> > > + /*
> > > + * Assume that valid per-device iommu structure must be installed
> > > + * if iommu_probe_device() has succeeded. This helper could only
> > > + * be used after device is probed.
> > > + */
> > > + return ((struct device_domain_info *)dev_iommu_priv_get(dev))-
> > > > iommu;
> > > +}
> >
> > Not sure whether this helper is useful. This is only used by 2 out of 5
> > post-probe users. Probably just open-coding in all 5 places is clearer.
>
> I thought it should get more users in the future development.
The pattern in the SMMUv2 driver is like
struct device_domain_info *info = dev_iommu_priv_get(dev);
struct intel_iommu *iommu = info->iommu;
Which really isn't worth the helper, unless you have lots of caes
where info isn't needed at all?
Jason
On 11/17/23 9:07 PM, Jason Gunthorpe wrote:
> On Thu, Nov 16, 2023 at 11:23:56AM +0800, Baolu Lu wrote:
>> On 11/16/23 11:19 AM, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Thursday, November 16, 2023 9:51 AM
>>>>
>>>> +static inline struct intel_iommu *dev_to_iommu(struct device *dev)
>>>> +{
>>>> + /*
>>>> + * Assume that valid per-device iommu structure must be installed
>>>> + * if iommu_probe_device() has succeeded. This helper could only
>>>> + * be used after device is probed.
>>>> + */
>>>> + return ((struct device_domain_info *)dev_iommu_priv_get(dev))-
>>>>> iommu;
>>>> +}
>>> Not sure whether this helper is useful. This is only used by 2 out of 5
>>> post-probe users. Probably just open-coding in all 5 places is clearer.
>> I thought it should get more users in the future development.
> The pattern in the SMMUv2 driver is like
>
> struct device_domain_info *info = dev_iommu_priv_get(dev);
> struct intel_iommu *iommu = info->iommu;
>
> Which really isn't worth the helper, unless you have lots of caes
> where info isn't needed at all?
No. As Kevin pointed out, there are only 2 places.
I will drop this helper.
Best regards,
baolu
@@ -851,6 +851,16 @@ static inline bool context_present(struct context_entry *context)
return (context->lo & 1);
}
+static inline struct intel_iommu *dev_to_iommu(struct device *dev)
+{
+ /*
+ * Assume that valid per-device iommu structure must be installed
+ * if iommu_probe_device() has succeeded. This helper could only
+ * be used after device is probed.
+ */
+ return ((struct device_domain_info *)dev_iommu_priv_get(dev))->iommu;
+}
+
struct dmar_drhd_unit *dmar_find_matched_drhd_unit(struct pci_dev *dev);
int dmar_enable_qi(struct intel_iommu *iommu);
@@ -897,7 +907,6 @@ int dmar_ir_support(void);
void *alloc_pgtable_page(int node, gfp_t gfp);
void free_pgtable_page(void *vaddr);
void iommu_flush_write_buffer(struct intel_iommu *iommu);
-struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
const struct iommu_user_data *user_data);
@@ -703,7 +703,7 @@ static bool iommu_is_dummy(struct intel_iommu *iommu, struct device *dev)
return false;
}
-struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn)
+static struct intel_iommu *device_lookup_iommu(struct device *dev, u8 *bus, u8 *devfn)
{
struct dmar_drhd_unit *drhd = NULL;
struct pci_dev *pdev = NULL;
@@ -2081,14 +2081,11 @@ static int domain_context_mapping_cb(struct pci_dev *pdev,
static int
domain_context_mapping(struct dmar_domain *domain, struct device *dev)
{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
struct domain_context_mapping_data data;
+ struct intel_iommu *iommu = info->iommu;
+ u8 bus = info->bus, devfn = info->devfn;
struct pasid_table *table;
- struct intel_iommu *iommu;
- u8 bus, devfn;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return -ENODEV;
table = intel_pasid_get_table(dev);
@@ -2447,15 +2444,10 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
struct device *dev)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
- struct intel_iommu *iommu;
+ struct intel_iommu *iommu = info->iommu;
unsigned long flags;
- u8 bus, devfn;
int ret;
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return -ENODEV;
-
ret = domain_attach_iommu(domain, iommu);
if (ret)
return ret;
@@ -4117,13 +4109,9 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
struct device *dev)
{
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
- struct intel_iommu *iommu;
+ struct intel_iommu *iommu = dev_to_iommu(dev);
int addr_width;
- iommu = device_to_iommu(dev, NULL, NULL);
- if (!iommu)
- return -ENODEV;
-
if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
return -EINVAL;
@@ -4399,7 +4387,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
u8 bus, devfn;
int ret;
- iommu = device_to_iommu(dev, &bus, &devfn);
+ iommu = device_lookup_iommu(dev, &bus, &devfn);
if (!iommu || !iommu->iommu.ops)
return ERR_PTR(-ENODEV);
@@ -4735,8 +4723,8 @@ static int 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 intel_iommu *iommu = dev_to_iommu(dev);
struct dmar_domain *dmar_domain;
struct iommu_domain *domain;
unsigned long flags;
@@ -366,14 +366,9 @@ static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
{
struct intel_svm_dev *sdev;
- struct intel_iommu *iommu;
struct intel_svm *svm;
struct mm_struct *mm;
- iommu = device_to_iommu(dev, NULL, NULL);
- if (!iommu)
- return;
-
if (pasid_to_svm_sdev(dev, pasid, &svm, &sdev))
return;
mm = svm->mm;
@@ -724,25 +719,16 @@ int intel_svm_page_response(struct device *dev,
struct iommu_fault_event *evt,
struct iommu_page_response *msg)
{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+ u8 bus = info->bus, devfn = info->devfn;
struct iommu_fault_page_request *prm;
- struct intel_iommu *iommu;
bool private_present;
bool pasid_present;
bool last_page;
- u8 bus, devfn;
int ret = 0;
u16 sid;
- if (!dev || !dev_is_pci(dev))
- return -ENODEV;
-
- iommu = device_to_iommu(dev, &bus, &devfn);
- if (!iommu)
- return -ENODEV;
-
- if (!msg || !evt)
- return -EINVAL;
-
prm = &evt->fault.prm;
sid = PCI_DEVID(bus, devfn);
pasid_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;