| Message ID | 20210919063848.1476776-5-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce /dev/iommu for userspace I/O address space management | expand |
On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: > From: Lu Baolu <baolu.lu@linux.intel.com> > > This provides an interface for upper layers to get the per-device iommu > attributes. > > int iommu_device_get_info(struct device *dev, > enum iommu_devattr attr, void *data); Can't we use properly typed ops and functions here instead of a void *data? get_snoop() get_page_size() get_addr_width() ? Jason
Hi Jason, On 9/22/21 12:19 AM, Jason Gunthorpe wrote: > On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: >> From: Lu Baolu <baolu.lu@linux.intel.com> >> >> This provides an interface for upper layers to get the per-device iommu >> attributes. >> >> int iommu_device_get_info(struct device *dev, >> enum iommu_devattr attr, void *data); > > Can't we use properly typed ops and functions here instead of a void > *data? > > get_snoop() > get_page_size() > get_addr_width() Yeah! Above are more friendly to the upper layer callers. > > ? > > Jason > Best regards, baolu
On Wed, Sep 22, 2021 at 10:31:47AM +0800, Lu Baolu wrote: > Hi Jason, > > On 9/22/21 12:19 AM, Jason Gunthorpe wrote: >> On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: >>> From: Lu Baolu <baolu.lu@linux.intel.com> >>> >>> This provides an interface for upper layers to get the per-device iommu >>> attributes. >>> >>> int iommu_device_get_info(struct device *dev, >>> enum iommu_devattr attr, void *data); >> >> Can't we use properly typed ops and functions here instead of a void >> *data? >> >> get_snoop() >> get_page_size() >> get_addr_width() > > Yeah! Above are more friendly to the upper layer callers. The other option would be a struct with all the attributes. Still type safe, but not as many methods. It'll require a little boilerplate in the callers, though.
On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: > From: Lu Baolu <baolu.lu@linux.intel.com> > > This provides an interface for upper layers to get the per-device iommu > attributes. > > int iommu_device_get_info(struct device *dev, > enum iommu_devattr attr, void *data); That fact that this interface doesn't let you know how to size the data buffer, other than by just knowing the right size for each attr concerns me. > > The first attribute (IOMMU_DEV_INFO_FORCE_SNOOP) is added. It tells if > the iommu can force DMA to snoop cache. At this stage, only PCI devices > which have this attribute set could use the iommufd, this is due to > supporting no-snoop DMA requires additional refactoring work on the > current kvm-vfio contract. The following patch will have vfio check this > attribute to decide whether a pci device can be exposed through > /dev/vfio/devices. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/iommu/iommu.c | 16 ++++++++++++++++ > include/linux/iommu.h | 19 +++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 63f0af10c403..5ea3a007fd7c 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -3260,3 +3260,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, > > return ret; > } > + > +/* Expose per-device iommu attributes. */ > +int iommu_device_get_info(struct device *dev, enum iommu_devattr attr, void *data) > +{ > + const struct iommu_ops *ops; > + > + if (!dev->bus || !dev->bus->iommu_ops) > + return -EINVAL; > + > + ops = dev->bus->iommu_ops; > + if (unlikely(!ops->device_info)) > + return -ENODEV; > + > + return ops->device_info(dev, attr, data); > +} > +EXPORT_SYMBOL_GPL(iommu_device_get_info); > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 32d448050bf7..52a6d33c82dc 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -150,6 +150,14 @@ enum iommu_dev_features { > IOMMU_DEV_FEAT_IOPF, > }; > > +/** > + * enum iommu_devattr - Per device IOMMU attributes > + * @IOMMU_DEV_INFO_FORCE_SNOOP [bool]: IOMMU can force DMA to be snooped. > + */ > +enum iommu_devattr { > + IOMMU_DEV_INFO_FORCE_SNOOP, > +}; > + > #define IOMMU_PASID_INVALID (-1U) > > #ifdef CONFIG_IOMMU_API > @@ -215,6 +223,7 @@ struct iommu_iotlb_gather { > * - IOMMU_DOMAIN_IDENTITY: must use an identity domain > * - IOMMU_DOMAIN_DMA: must use a dma domain > * - 0: use the default setting > + * @device_info: query per-device iommu attributes > * @pgsize_bitmap: bitmap of all possible supported page sizes > * @owner: Driver module providing these ops > */ > @@ -283,6 +292,8 @@ struct iommu_ops { > > int (*def_domain_type)(struct device *dev); > > + int (*device_info)(struct device *dev, enum iommu_devattr attr, void *data); > + > unsigned long pgsize_bitmap; > struct module *owner; > }; > @@ -604,6 +615,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, > void iommu_sva_unbind_device(struct iommu_sva *handle); > u32 iommu_sva_get_pasid(struct iommu_sva *handle); > > +int iommu_device_get_info(struct device *dev, enum iommu_devattr attr, void *data); > + > #else /* CONFIG_IOMMU_API */ > > struct iommu_ops {}; > @@ -999,6 +1012,12 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev) > { > return NULL; > } > + > +static inline int iommu_device_get_info(struct device *dev, > + enum iommu_devattr type, void *data) > +{ > + return -ENODEV; > +} > #endif /* CONFIG_IOMMU_API */ > > /**
Hi David, On 2021/9/29 10:52, David Gibson wrote: > On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: >> From: Lu Baolu<baolu.lu@linux.intel.com> >> >> This provides an interface for upper layers to get the per-device iommu >> attributes. >> >> int iommu_device_get_info(struct device *dev, >> enum iommu_devattr attr, void *data); > That fact that this interface doesn't let you know how to size the > data buffer, other than by just knowing the right size for each attr > concerns me. > We plan to address this by following the comments here. https://lore.kernel.org/linux-iommu/20210921161930.GP327412@nvidia.com/ Best regards, baolu
On 2021/9/29 17:25, Lu Baolu wrote: > Hi David, > > On 2021/9/29 10:52, David Gibson wrote: >> On Sun, Sep 19, 2021 at 02:38:32PM +0800, Liu Yi L wrote: >>> From: Lu Baolu<baolu.lu@linux.intel.com> >>> >>> This provides an interface for upper layers to get the per-device iommu >>> attributes. >>> >>> int iommu_device_get_info(struct device *dev, >>> enum iommu_devattr attr, void *data); >> That fact that this interface doesn't let you know how to size the >> data buffer, other than by just knowing the right size for each attr >> concerns me. >> > > We plan to address this by following the comments here. > > https://lore.kernel.org/linux-iommu/20210921161930.GP327412@nvidia.com/ And Christoph gave another option as well. https://lore.kernel.org/linux-iommu/20210922050746.GA12921@lst.de/ Best regards, baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 63f0af10c403..5ea3a007fd7c 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3260,3 +3260,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, return ret; } + +/* Expose per-device iommu attributes. */ +int iommu_device_get_info(struct device *dev, enum iommu_devattr attr, void *data) +{ + const struct iommu_ops *ops; + + if (!dev->bus || !dev->bus->iommu_ops) + return -EINVAL; + + ops = dev->bus->iommu_ops; + if (unlikely(!ops->device_info)) + return -ENODEV; + + return ops->device_info(dev, attr, data); +} +EXPORT_SYMBOL_GPL(iommu_device_get_info); diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 32d448050bf7..52a6d33c82dc 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -150,6 +150,14 @@ enum iommu_dev_features { IOMMU_DEV_FEAT_IOPF, }; +/** + * enum iommu_devattr - Per device IOMMU attributes + * @IOMMU_DEV_INFO_FORCE_SNOOP [bool]: IOMMU can force DMA to be snooped. + */ +enum iommu_devattr { + IOMMU_DEV_INFO_FORCE_SNOOP, +}; + #define IOMMU_PASID_INVALID (-1U) #ifdef CONFIG_IOMMU_API @@ -215,6 +223,7 @@ struct iommu_iotlb_gather { * - IOMMU_DOMAIN_IDENTITY: must use an identity domain * - IOMMU_DOMAIN_DMA: must use a dma domain * - 0: use the default setting + * @device_info: query per-device iommu attributes * @pgsize_bitmap: bitmap of all possible supported page sizes * @owner: Driver module providing these ops */ @@ -283,6 +292,8 @@ struct iommu_ops { int (*def_domain_type)(struct device *dev); + int (*device_info)(struct device *dev, enum iommu_devattr attr, void *data); + unsigned long pgsize_bitmap; struct module *owner; }; @@ -604,6 +615,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, void iommu_sva_unbind_device(struct iommu_sva *handle); u32 iommu_sva_get_pasid(struct iommu_sva *handle); +int iommu_device_get_info(struct device *dev, enum iommu_devattr attr, void *data); + #else /* CONFIG_IOMMU_API */ struct iommu_ops {}; @@ -999,6 +1012,12 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev) { return NULL; } + +static inline int iommu_device_get_info(struct device *dev, + enum iommu_devattr type, void *data) +{ + return -ENODEV; +} #endif /* CONFIG_IOMMU_API */ /**