| Message ID | 2-v3-3313bb5dd3a3+10f11-secure_msi_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove IOMMU_CAP_INTR_REMAP | expand |
On 1/6/2023 3:33 AM, Jason Gunthorpe wrote: > Compute the isolated_msi over all the devices in the IOMMU group because > iommufd and vfio both need to know that the entire group is isolated > before granting access to it. > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 26 ++++++++++++++++++++++++++ > include/linux/iommu.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index de91dd88705bd3..7f744904e02f4d 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -30,6 +30,7 @@ > #include <linux/cc_platform.h> > #include <trace/events/iommu.h> > #include <linux/sched/mm.h> > +#include <linux/msi.h> > > #include "dma-iommu.h" > > @@ -1897,6 +1898,31 @@ bool device_iommu_capable(struct device *dev, enum iommu_cap cap) > } > EXPORT_SYMBOL_GPL(device_iommu_capable); > > +/** > + * iommu_group_has_isolated_msi() - Compute msi_device_has_isolated_msi() > + * for a group > + * @group: Group to query > + * > + * IOMMU groups should not have differing values of > + * msi_device_has_isolated_msi() for devices in a group. However nothing > + * directly prevents this, so ensure mistakes don't result in isolation failures > + * by checking that all the devices are the same. > + */ > +bool iommu_group_has_isolated_msi(struct iommu_group *group) > +{ > + struct group_device *group_dev; > + bool ret = true; > + > + mutex_lock(&group->mutex); > + list_for_each_entry(group_dev, &group->devices, list) > + ret &= msi_device_has_isolated_msi(group_dev->dev) || > + device_iommu_capable(group_dev->dev, > + IOMMU_CAP_INTR_REMAP); > + mutex_unlock(&group->mutex); > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_group_has_isolated_msi); > + > /** > * iommu_set_fault_handler() - set a fault handler for an iommu domain > * @domain: iommu domain > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 46e1347bfa2286..9b7a9fa5ad28d3 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -455,6 +455,7 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) > extern int bus_iommu_probe(struct bus_type *bus); > extern bool iommu_present(struct bus_type *bus); > extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap); > +extern bool iommu_group_has_isolated_msi(struct iommu_group *group); This lacks a static inline definition when CONFIG_IOMMU_API is false? > extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus); > extern struct iommu_group *iommu_group_get_by_id(int id); > extern void iommu_domain_free(struct iommu_domain *domain); Others look good to me. With above addressed, Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> -- Best regards, baolu
On Fri, Jan 06, 2023 at 07:28:46PM +0800, Baolu Lu wrote: > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 46e1347bfa2286..9b7a9fa5ad28d3 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -455,6 +455,7 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) > > extern int bus_iommu_probe(struct bus_type *bus); > > extern bool iommu_present(struct bus_type *bus); > > extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap); > > +extern bool iommu_group_has_isolated_msi(struct iommu_group *group); > > This lacks a static inline definition when CONFIG_IOMMU_API is false? It is not needed, the call sites are all compilation protected by CONFIG_IOMMU_API already. Thanks, Jason
On 1/6/2023 9:12 PM, Jason Gunthorpe wrote: > On Fri, Jan 06, 2023 at 07:28:46PM +0800, Baolu Lu wrote: > >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >>> index 46e1347bfa2286..9b7a9fa5ad28d3 100644 >>> --- a/include/linux/iommu.h >>> +++ b/include/linux/iommu.h >>> @@ -455,6 +455,7 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) >>> extern int bus_iommu_probe(struct bus_type *bus); >>> extern bool iommu_present(struct bus_type *bus); >>> extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap); >>> +extern bool iommu_group_has_isolated_msi(struct iommu_group *group); >> This lacks a static inline definition when CONFIG_IOMMU_API is false? > It is not needed, the call sites are all compilation protected by > CONFIG_IOMMU_API already. Thanks for the explanation. It's okay to me as it has been considered. -- Best regards, baolu
On Thu, Jan 05 2023 at 15:33, Jason Gunthorpe wrote: > + > + mutex_lock(&group->mutex); > + list_for_each_entry(group_dev, &group->devices, list) > + ret &= msi_device_has_isolated_msi(group_dev->dev) || > + device_iommu_capable(group_dev->dev, > + IOMMU_CAP_INTR_REMAP); Nit. This really wants brackets even if they are not required by the language. Why? Brackets can be omitted for a single line statement in the loop/if path, but this > + ret &= msi_device_has_isolated_msi(group_dev->dev) || > + device_iommu_capable(group_dev->dev, > + IOMMU_CAP_INTR_REMAP); is visually a multi line statement. So having brackets makes visual parsing of this construct way clearer: list_for_each_entry(group_dev, &group->devices, list) { ret &= msi_device_has_isolated_msi(group_dev->dev) || device_iommu_capable(group_dev->dev, IOMMU_CAP_INTR_REMAP); } Also get rid of that extra line break. We lifted the 80 characters line length quite some while ago and made it 100. Thanks, tglx
On Wed, Jan 11, 2023 at 07:19:34PM +0100, Thomas Gleixner wrote: > On Thu, Jan 05 2023 at 15:33, Jason Gunthorpe wrote: > > + > > + mutex_lock(&group->mutex); > > + list_for_each_entry(group_dev, &group->devices, list) > > + ret &= msi_device_has_isolated_msi(group_dev->dev) || > > + device_iommu_capable(group_dev->dev, > > + IOMMU_CAP_INTR_REMAP); > > Nit. This really wants brackets even if they are not required by the > language. Why? > > Brackets can be omitted for a single line statement in the loop/if path, > but this > > > + ret &= msi_device_has_isolated_msi(group_dev->dev) || > > + device_iommu_capable(group_dev->dev, > > + IOMMU_CAP_INTR_REMAP); > > is visually a multi line statement. So having brackets makes visual > parsing of this construct way clearer: Sure, though this is all undone by the last patch which does make it visually a single statement: list_for_each_entry(group_dev, &group->devices, list) - ret &= msi_device_has_isolated_msi(group_dev->dev) || - device_iommu_capable(group_dev->dev, - IOMMU_CAP_INTR_REMAP); + ret &= msi_device_has_isolated_msi(group_dev->dev); > Also get rid of that extra line break. We lifted the 80 characters line > length quite some while ago and made it 100. Ah, there are so many different opinions on that! I got everything else, I'll stick this in linux-next via the iommufd tree so more bots can check it - if anyone else has remarks I can make a v4 Thanks, Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index de91dd88705bd3..7f744904e02f4d 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -30,6 +30,7 @@ #include <linux/cc_platform.h> #include <trace/events/iommu.h> #include <linux/sched/mm.h> +#include <linux/msi.h> #include "dma-iommu.h" @@ -1897,6 +1898,31 @@ bool device_iommu_capable(struct device *dev, enum iommu_cap cap) } EXPORT_SYMBOL_GPL(device_iommu_capable); +/** + * iommu_group_has_isolated_msi() - Compute msi_device_has_isolated_msi() + * for a group + * @group: Group to query + * + * IOMMU groups should not have differing values of + * msi_device_has_isolated_msi() for devices in a group. However nothing + * directly prevents this, so ensure mistakes don't result in isolation failures + * by checking that all the devices are the same. + */ +bool iommu_group_has_isolated_msi(struct iommu_group *group) +{ + struct group_device *group_dev; + bool ret = true; + + mutex_lock(&group->mutex); + list_for_each_entry(group_dev, &group->devices, list) + ret &= msi_device_has_isolated_msi(group_dev->dev) || + device_iommu_capable(group_dev->dev, + IOMMU_CAP_INTR_REMAP); + mutex_unlock(&group->mutex); + return ret; +} +EXPORT_SYMBOL_GPL(iommu_group_has_isolated_msi); + /** * iommu_set_fault_handler() - set a fault handler for an iommu domain * @domain: iommu domain diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 46e1347bfa2286..9b7a9fa5ad28d3 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -455,6 +455,7 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) extern int bus_iommu_probe(struct bus_type *bus); extern bool iommu_present(struct bus_type *bus); extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap); +extern bool iommu_group_has_isolated_msi(struct iommu_group *group); extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus); extern struct iommu_group *iommu_group_get_by_id(int id); extern void iommu_domain_free(struct iommu_domain *domain);