Re: [PATCH v2 04/17] driver core: platform: Add driver dma ownership management

[Jason Gunthorpe]

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