| Message ID | 2-v1-9e466539c244+47b5-secure_msi_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove IOMMU_CAP_INTR_REMAP | expand |
On Thu, 8 Dec 2022 16:26:29 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > iommu_group_for_each_dev() exits the loop at the first callback that > returns 1 - thus returning 1 fails to check the rest of the devices in the > group. > > msi_remap (aka secure MSI) requires that all the devices in the group > support it, not just any one. This is only a theoretical problem as no > current drivers will have different secure MSI properties within a group. Which is exactly how Robin justified the behavior in the referenced commit: As with domains, any capability must in practice be consistent for devices in a given group - and after all it's still the same capability which was expected to be consistent across an entire bus! - so there's no need for any complicated validation. That suggests to me that it's intentional that we break if any device supports the capability and therefore this isn't so much a "Fixes:", as it is a refactoring expressly to support msi_device_has_secure_msi(), which cannot make these sort of assumptions as a non-group API. Thanks, Alex > Make vfio_iommu_device_secure_msi() reduce AND across all the devices. > > Fixes: eed20c782aea ("vfio/type1: Simplify bus_type determination") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio_iommu_type1.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 23c24fe98c00d4..3025b4e643c135 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -2160,10 +2160,12 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu, > list_splice_tail(iova_copy, iova); > } > > -/* Redundantly walks non-present capabilities to simplify caller */ > -static int vfio_iommu_device_capable(struct device *dev, void *data) > +static int vfio_iommu_device_secure_msi(struct device *dev, void *data) > { > - return device_iommu_capable(dev, (enum iommu_cap)data); > + bool *secure_msi = data; > + > + *secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP); > + return 0; > } > > static int vfio_iommu_domain_alloc(struct device *dev, void *data) > @@ -2278,9 +2280,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, > INIT_LIST_HEAD(&domain->group_list); > list_add(&group->next, &domain->group_list); > > - msi_remap = irq_domain_check_msi_remap() || > - iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP, > - vfio_iommu_device_capable); > + msi_remap = irq_domain_check_msi_remap(); > + if (!msi_remap) { > + msi_remap = true; > + iommu_group_for_each_dev(iommu_group, &msi_remap, > + vfio_iommu_device_secure_msi); > + } > > if (!allow_unsafe_interrupts && !msi_remap) { > pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
On Thu, Dec 08, 2022 at 02:48:25PM -0700, Alex Williamson wrote: > On Thu, 8 Dec 2022 16:26:29 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > iommu_group_for_each_dev() exits the loop at the first callback that > > returns 1 - thus returning 1 fails to check the rest of the devices in the > > group. > > > > msi_remap (aka secure MSI) requires that all the devices in the group > > support it, not just any one. This is only a theoretical problem as no > > current drivers will have different secure MSI properties within a group. > > Which is exactly how Robin justified the behavior in the referenced > commit: > > As with domains, any capability must in practice be consistent for > devices in a given group - and after all it's still the same > capability which was expected to be consistent across an entire bus! > - so there's no need for any complicated validation. > > That suggests to me that it's intentional that we break if any device > supports the capability and therefore this isn't so much a "Fixes:", as > it is a refactoring expressly to support msi_device_has_secure_msi(), > which cannot make these sort of assumptions as a non-group API. Thanks, Sure, lets drop the fixes and your analysis seems correct Jason
On 2022-12-09 00:44, Jason Gunthorpe wrote: > On Thu, Dec 08, 2022 at 02:48:25PM -0700, Alex Williamson wrote: >> On Thu, 8 Dec 2022 16:26:29 -0400 >> Jason Gunthorpe <jgg@nvidia.com> wrote: >> >>> iommu_group_for_each_dev() exits the loop at the first callback that >>> returns 1 - thus returning 1 fails to check the rest of the devices in the >>> group. >>> >>> msi_remap (aka secure MSI) requires that all the devices in the group >>> support it, not just any one. This is only a theoretical problem as no >>> current drivers will have different secure MSI properties within a group. >> >> Which is exactly how Robin justified the behavior in the referenced >> commit: >> >> As with domains, any capability must in practice be consistent for >> devices in a given group - and after all it's still the same >> capability which was expected to be consistent across an entire bus! >> - so there's no need for any complicated validation. >> >> That suggests to me that it's intentional that we break if any device >> supports the capability and therefore this isn't so much a "Fixes:", as >> it is a refactoring expressly to support msi_device_has_secure_msi(), >> which cannot make these sort of assumptions as a non-group API. Thanks, > > Sure, lets drop the fixes and your analysis seems correct Yup, Alex is spot on - the fact is that all the IOMMU drivers supporting this cap have always returned global values, and continue to do so right up until we remove it later, so there's really no benefit in pretending otherwise. I'd say just fold this into patch #3 to keep it even simpler. Thanks, Robin.
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00d4..3025b4e643c135 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -2160,10 +2160,12 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu, list_splice_tail(iova_copy, iova); } -/* Redundantly walks non-present capabilities to simplify caller */ -static int vfio_iommu_device_capable(struct device *dev, void *data) +static int vfio_iommu_device_secure_msi(struct device *dev, void *data) { - return device_iommu_capable(dev, (enum iommu_cap)data); + bool *secure_msi = data; + + *secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP); + return 0; } static int vfio_iommu_domain_alloc(struct device *dev, void *data) @@ -2278,9 +2280,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, INIT_LIST_HEAD(&domain->group_list); list_add(&group->next, &domain->group_list); - msi_remap = irq_domain_check_msi_remap() || - iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP, - vfio_iommu_device_capable); + msi_remap = irq_domain_check_msi_remap(); + if (!msi_remap) { + msi_remap = true; + iommu_group_for_each_dev(iommu_group, &msi_remap, + vfio_iommu_device_secure_msi); + } if (!allow_unsafe_interrupts && !msi_remap) { pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
iommu_group_for_each_dev() exits the loop at the first callback that returns 1 - thus returning 1 fails to check the rest of the devices in the group. msi_remap (aka secure MSI) requires that all the devices in the group support it, not just any one. This is only a theoretical problem as no current drivers will have different secure MSI properties within a group. Make vfio_iommu_device_secure_msi() reduce AND across all the devices. Fixes: eed20c782aea ("vfio/type1: Simplify bus_type determination") Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio_iommu_type1.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)