| Message ID | 20211128025051.355578-5-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
On Sun, Nov 28, 2021 at 10:50:38AM +0800, Lu Baolu wrote: > Multiple platform devices may be placed in the same IOMMU group because > they cannot be isolated from each other. These devices must either be > entirely under kernel control or userspace control, never a mixture. This > checks and sets DMA ownership during driver binding, and release the > ownership during driver unbinding. > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, > the userspace framework drivers (vfio etc.) which need to manually claim > DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. Why would any vfio driver be a platform driver? That should never be the case as they obviously are not platform drivers, they are virtual ones. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/platform_device.h | 1 + > drivers/base/platform.c | 30 +++++++++++++++++++++++++++++- > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..779bcf2a851c 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -210,6 +210,7 @@ struct platform_driver { > struct device_driver driver; > const struct platform_device_id *id_table; > bool prevent_deferred_probe; > + bool suppress_auto_claim_dma_owner; What platform driver needs this change? > }; > > #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 598acf93a360..df4b385c8a52 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -30,6 +30,7 @@ > #include <linux/property.h> > #include <linux/kmemleak.h> > #include <linux/types.h> > +#include <linux/iommu.h> > > #include "base.h" > #include "power/power.h" > @@ -1465,6 +1466,32 @@ int platform_dma_configure(struct device *dev) > return ret; > } > > +static int _platform_dma_configure(struct device *dev) > +{ > + struct platform_driver *drv = to_platform_driver(dev->driver); > + int ret; > + > + if (!drv->suppress_auto_claim_dma_owner) { > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); > + if (ret) > + return ret; > + } > + > + ret = platform_dma_configure(dev); > + if (ret && !drv->suppress_auto_claim_dma_owner) > + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); > + > + return ret; > +} > + > +static void _platform_dma_unconfigure(struct device *dev) > +{ > + struct platform_driver *drv = to_platform_driver(dev->driver); > + > + if (!drv->suppress_auto_claim_dma_owner) > + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); > +} > + > static const struct dev_pm_ops platform_dev_pm_ops = { > SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL) > USE_PLATFORM_PM_SLEEP_OPS > @@ -1478,7 +1505,8 @@ struct bus_type platform_bus_type = { > .probe = platform_probe, > .remove = platform_remove, > .shutdown = platform_shutdown, > - .dma_configure = platform_dma_configure, > + .dma_configure = _platform_dma_configure, What happened to the original platform_dma_configure() function? And single "_" prefixes are odd, please just spell out what the difference is in the function name, "_" gives us no hint at all. thnaks, greg k-h
On Sun, Nov 28, 2021 at 09:10:14AM +0100, Greg Kroah-Hartman wrote: > On Sun, Nov 28, 2021 at 10:50:38AM +0800, Lu Baolu wrote: > > Multiple platform devices may be placed in the same IOMMU group because > > they cannot be isolated from each other. These devices must either be > > entirely under kernel control or userspace control, never a mixture. This > > checks and sets DMA ownership during driver binding, and release the > > ownership during driver unbinding. > > > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > > claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, > > the userspace framework drivers (vfio etc.) which need to manually claim > > DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. > > Why would any vfio driver be a platform driver? Why not? VFIO implements drivers for most physical device types these days. Why wouldn't platform be included? > That should never be the case as they obviously are not platform > drivers, they are virtual ones. Huh? > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > > index 7c96f169d274..779bcf2a851c 100644 > > +++ b/include/linux/platform_device.h > > @@ -210,6 +210,7 @@ struct platform_driver { > > struct device_driver driver; > > const struct platform_device_id *id_table; > > bool prevent_deferred_probe; > > + bool suppress_auto_claim_dma_owner; > > What platform driver needs this change? It is in patch 12: --- a/drivers/vfio/platform/vfio_platform.c +++ b/drivers/vfio/platform/vfio_platform.c @@ -76,6 +76,7 @@ static struct platform_driver vfio_platform_driver = { .driver = { .name = "vfio-platform", }, + .suppress_auto_claim_dma_owner = true, }; Which is how VFIO provides support to DPDK for some Ethernet controllers embedded in a few ARM SOCs. It is also used in patch 17 in five tegra platform_drivers to make their sharing of an iommu group between possibly related platform_driver's safer. > > USE_PLATFORM_PM_SLEEP_OPS > > @@ -1478,7 +1505,8 @@ struct bus_type platform_bus_type = { > > .probe = platform_probe, > > .remove = platform_remove, > > .shutdown = platform_shutdown, > > - .dma_configure = platform_dma_configure, > > + .dma_configure = _platform_dma_configure, > > What happened to the original platform_dma_configure() function? It is still called. The issue here is that platform_dma_configure has nothing to do with platform and is being re-used by AMBA. Probably the resolution to both remarks is to rename platform_dma_configure to something sensible (firwmare dma configure maybe?) and use it in all places that do the of & acpi stuff - pci/amba/platform at least. Jason
On Sun, Nov 28, 2021 at 07:15:09PM -0400, Jason Gunthorpe wrote: > On Sun, Nov 28, 2021 at 09:10:14AM +0100, Greg Kroah-Hartman wrote: > > On Sun, Nov 28, 2021 at 10:50:38AM +0800, Lu Baolu wrote: > > > Multiple platform devices may be placed in the same IOMMU group because > > > they cannot be isolated from each other. These devices must either be > > > entirely under kernel control or userspace control, never a mixture. This > > > checks and sets DMA ownership during driver binding, and release the > > > ownership during driver unbinding. > > > > > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > > > claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, > > > the userspace framework drivers (vfio etc.) which need to manually claim > > > DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. > > > > Why would any vfio driver be a platform driver? > > Why not? VFIO implements drivers for most physical device types > these days. Why wouldn't platform be included? Because "platform" is not a real device type. It's a catch-all for devices that are only described by firmware, so why would you have a virtual device for that? Why would that be needed? > > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > > > index 7c96f169d274..779bcf2a851c 100644 > > > +++ b/include/linux/platform_device.h > > > @@ -210,6 +210,7 @@ struct platform_driver { > > > struct device_driver driver; > > > const struct platform_device_id *id_table; > > > bool prevent_deferred_probe; > > > + bool suppress_auto_claim_dma_owner; > > > > What platform driver needs this change? > > It is in patch 12: > > --- a/drivers/vfio/platform/vfio_platform.c > +++ b/drivers/vfio/platform/vfio_platform.c Ok, nevermind, you do have a virtual platform device, which personally, I find crazy as why would firmware export a "virtual device"? > @@ -76,6 +76,7 @@ static struct platform_driver vfio_platform_driver = { > .driver = { > .name = "vfio-platform", > }, > + .suppress_auto_claim_dma_owner = true, > }; > > Which is how VFIO provides support to DPDK for some Ethernet > controllers embedded in a few ARM SOCs. Ick. Where does the DT file for these devices live that describe a "virtual device" to match with this driver? > It is also used in patch 17 in five tegra platform_drivers to make > their sharing of an iommu group between possibly related > platform_driver's safer. Safer how? > > > USE_PLATFORM_PM_SLEEP_OPS > > > @@ -1478,7 +1505,8 @@ struct bus_type platform_bus_type = { > > > .probe = platform_probe, > > > .remove = platform_remove, > > > .shutdown = platform_shutdown, > > > - .dma_configure = platform_dma_configure, > > > + .dma_configure = _platform_dma_configure, > > > > What happened to the original platform_dma_configure() function? > > It is still called. The issue here is that platform_dma_configure has > nothing to do with platform and is being re-used by AMBA. Ick, why? AMBA needs to be a real bus type and use their own functions if needed. There is nothing here that makes this obvious that someone else is using those functions and that the platform bus should only be using these "new" functions. > Probably the resolution to both remarks is to rename > platform_dma_configure to something sensible (firwmare dma configure > maybe?) and use it in all places that do the of & acpi stuff - > pci/amba/platform at least. That would be better than what is being proposed here. thanks, greg k-h
On Mon, Nov 29, 2021 at 11:34:39AM +0100, Greg Kroah-Hartman wrote: > On Sun, Nov 28, 2021 at 07:15:09PM -0400, Jason Gunthorpe wrote: > > On Sun, Nov 28, 2021 at 09:10:14AM +0100, Greg Kroah-Hartman wrote: > > > On Sun, Nov 28, 2021 at 10:50:38AM +0800, Lu Baolu wrote: > > > > Multiple platform devices may be placed in the same IOMMU group because > > > > they cannot be isolated from each other. These devices must either be > > > > entirely under kernel control or userspace control, never a mixture. This > > > > checks and sets DMA ownership during driver binding, and release the > > > > ownership during driver unbinding. > > > > > > > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > > > > claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, > > > > the userspace framework drivers (vfio etc.) which need to manually claim > > > > DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. > > > > > > Why would any vfio driver be a platform driver? > > > > Why not? VFIO implements drivers for most physical device types > > these days. Why wouldn't platform be included? > > Because "platform" is not a real device type. It's a catch-all for > devices that are only described by firmware, so why would you have a > virtual device for that? Why would that be needed? Why does it matter how a physical device is enumerated? PCI/DT/ACPI/setup.c - it doesn't matter. There is still a physical device with physical DMA and MMIO. As long as people are making ethernet controllers and other interesting devices enumerated through platform_device there will be need to expose them to userspace through VFIO too. > Ok, nevermind, you do have a virtual platform device, which personally, > I find crazy as why would firmware export a "virtual device"? Why do you keep saying "virtual device"? The "VF" in vfio refers to language in the PCI-SIG SRIOV specification. You are better to think of the V as meaning "for virtualization". Today vfio is just a nonsense acronym. The subsystem's job is to allow user space to fully operate a physical HW device, including using its DMA and interrupts. With the advent of DPDK/SPDK/etc it isn't even related to virtualization use-cases any more. It is a lot like UIO except VFIO can operate devices that use DMA too. > > @@ -76,6 +76,7 @@ static struct platform_driver vfio_platform_driver = { > > .driver = { > > .name = "vfio-platform", > > }, > > + .suppress_auto_claim_dma_owner = true, > > }; > > > > Which is how VFIO provides support to DPDK for some Ethernet > > controllers embedded in a few ARM SOCs. > > Ick. Where does the DT file for these devices live that describe a > "virtual device" to match with this driver? The DT describes a physical ethernet device that would normally load it's netdev driver. If the admin wishes to use that physical ethernet device in userspace, say with DPDK, then the admin switches the driver from netdev to vfio. The OF compatible string 'calxeda,hb-xgmac' is one example: Documentation/devicetree/bindings/net/calxeda-xgmac.yaml arch/arm/boot/dts/ecx-common.dtsi drivers/net/ethernet/calxeda/xgmac.c drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c We all recently went over the switching mechanim in a lot of detail when you Ack'd this patch series: https://lore.kernel.org/kvm/20210721161609.68223-1-yishaih@nvidia.com/ (though vfio platform has not yet been revised to use this mechanism, it still uses the sysfs driver_override) > > It is also used in patch 17 in five tegra platform_drivers to make > > their sharing of an iommu group between possibly related > > platform_driver's safer. > > Safer how? tegra makes assumptions that the DT configures the IOMMU in a certain way. If the DT does something else then the kernel will probably corrupt memory with wild DMAs. After this series the conflicting IOMMU usages will be detected and blocked instead. Thanks, Jason
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 7c96f169d274..779bcf2a851c 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -210,6 +210,7 @@ struct platform_driver { struct device_driver driver; const struct platform_device_id *id_table; bool prevent_deferred_probe; + bool suppress_auto_claim_dma_owner; }; #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 598acf93a360..df4b385c8a52 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -30,6 +30,7 @@ #include <linux/property.h> #include <linux/kmemleak.h> #include <linux/types.h> +#include <linux/iommu.h> #include "base.h" #include "power/power.h" @@ -1465,6 +1466,32 @@ int platform_dma_configure(struct device *dev) return ret; } +static int _platform_dma_configure(struct device *dev) +{ + struct platform_driver *drv = to_platform_driver(dev->driver); + int ret; + + if (!drv->suppress_auto_claim_dma_owner) { + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL); + if (ret) + return ret; + } + + ret = platform_dma_configure(dev); + if (ret && !drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); + + return ret; +} + +static void _platform_dma_unconfigure(struct device *dev) +{ + struct platform_driver *drv = to_platform_driver(dev->driver); + + if (!drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API); +} + static const struct dev_pm_ops platform_dev_pm_ops = { SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL) USE_PLATFORM_PM_SLEEP_OPS @@ -1478,7 +1505,8 @@ struct bus_type platform_bus_type = { .probe = platform_probe, .remove = platform_remove, .shutdown = platform_shutdown, - .dma_configure = platform_dma_configure, + .dma_configure = _platform_dma_configure, + .dma_unconfigure = _platform_dma_unconfigure, .pm = &platform_dev_pm_ops, }; EXPORT_SYMBOL_GPL(platform_bus_type);
Multiple platform devices may be placed in the same IOMMU group because they cannot be isolated from each other. These devices must either be entirely under kernel control or userspace control, never a mixture. This checks and sets DMA ownership during driver binding, and release the ownership during driver unbinding. Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto claiming DMA_OWNER_DMA_API ownership in the binding process. For instance, the userspace framework drivers (vfio etc.) which need to manually claim DMA_OWNER_PRIVATE_DOMAIN_USER when assigning a device to userspace. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/platform_device.h | 1 + drivers/base/platform.c | 30 +++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)