| Message ID | 20211217063708.1740334-4-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 Fri, Dec 17, 2021 at 02:36:58PM +0800, Lu Baolu wrote: > The pci_dma_configure() marks the iommu_group as containing only devices > with kernel drivers that manage DMA. I'm looking at pci_dma_configure(), and I don't see the connection to iommu_groups. > Avoid this default behavior for the > pci_stub because it does not program any DMA itself. This allows the > pci_stub still able to be used by the admin to block driver binding after > applying the DMA ownership to vfio. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/pci/pci-stub.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c > index e408099fea52..6324c68602b4 100644 > --- a/drivers/pci/pci-stub.c > +++ b/drivers/pci/pci-stub.c > @@ -36,6 +36,9 @@ static struct pci_driver stub_driver = { > .name = "pci-stub", > .id_table = NULL, /* only dynamic id's */ > .probe = pci_stub_probe, > + .driver = { > + .suppress_auto_claim_dma_owner = true, The new .suppress_auto_claim_dma_owner controls whether we call iommu_device_set_dma_owner(). I guess you added .suppress_auto_claim_dma_owner because iommu_device_set_dma_owner() must be done *before* we call the driver's .probe() method? Otherwise, we could call some new interface from .probe() instead of adding the flag to struct device_driver. > + }, > }; > > static int __init pci_stub_init(void) > -- > 2.25.1 >
Hi Bjorn, On 12/30/21 4:42 AM, Bjorn Helgaas wrote: > On Fri, Dec 17, 2021 at 02:36:58PM +0800, Lu Baolu wrote: >> The pci_dma_configure() marks the iommu_group as containing only devices >> with kernel drivers that manage DMA. > > I'm looking at pci_dma_configure(), and I don't see the connection to > iommu_groups. The 2nd patch "driver core: Set DMA ownership during driver bind/unbind" sets all drivers' DMA to be kernel-managed by default except a few ones which has a driver flag set. So by default, all iommu groups contains only devices with kernel drivers managing DMA. > >> Avoid this default behavior for the >> pci_stub because it does not program any DMA itself. This allows the >> pci_stub still able to be used by the admin to block driver binding after >> applying the DMA ownership to vfio. > >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> drivers/pci/pci-stub.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c >> index e408099fea52..6324c68602b4 100644 >> --- a/drivers/pci/pci-stub.c >> +++ b/drivers/pci/pci-stub.c >> @@ -36,6 +36,9 @@ static struct pci_driver stub_driver = { >> .name = "pci-stub", >> .id_table = NULL, /* only dynamic id's */ >> .probe = pci_stub_probe, >> + .driver = { >> + .suppress_auto_claim_dma_owner = true, > > The new .suppress_auto_claim_dma_owner controls whether we call > iommu_device_set_dma_owner(). I guess you added > .suppress_auto_claim_dma_owner because iommu_device_set_dma_owner() > must be done *before* we call the driver's .probe() method? As explained above, all drivers are set to kernel-managed dma by default. For those vfio and vfio-approved drivers, suppress_auto_claim_dma_owner is used to tell the driver core that "this driver is attached to device for userspace assignment purpose, do not claim it for kernel-management dma". > > Otherwise, we could call some new interface from .probe() instead of > adding the flag to struct device_driver. Most device drivers are of the kernel-managed DMA type. Only a few vfio and vfio-approved drivers need to use this flag. That's the reason why we claim kernel-managed DMA by default. > >> + }, >> }; >> >> static int __init pci_stub_init(void) >> -- >> 2.25.1 >> Best regards, baolu
On Thu, Dec 30, 2021 at 01:34:27PM +0800, Lu Baolu wrote: > Hi Bjorn, > > On 12/30/21 4:42 AM, Bjorn Helgaas wrote: > > On Fri, Dec 17, 2021 at 02:36:58PM +0800, Lu Baolu wrote: > > > The pci_dma_configure() marks the iommu_group as containing only devices > > > with kernel drivers that manage DMA. > > > > I'm looking at pci_dma_configure(), and I don't see the connection to > > iommu_groups. > > The 2nd patch "driver core: Set DMA ownership during driver bind/unbind" > sets all drivers' DMA to be kernel-managed by default except a few ones > which has a driver flag set. So by default, all iommu groups contains > only devices with kernel drivers managing DMA. It looks like that happens in device_dma_configure(), not pci_dma_configure(). > > > Avoid this default behavior for the > > > pci_stub because it does not program any DMA itself. This allows the > > > pci_stub still able to be used by the admin to block driver binding after > > > applying the DMA ownership to vfio. > > > > > > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > > > --- > > > drivers/pci/pci-stub.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c > > > index e408099fea52..6324c68602b4 100644 > > > --- a/drivers/pci/pci-stub.c > > > +++ b/drivers/pci/pci-stub.c > > > @@ -36,6 +36,9 @@ static struct pci_driver stub_driver = { > > > .name = "pci-stub", > > > .id_table = NULL, /* only dynamic id's */ > > > .probe = pci_stub_probe, > > > + .driver = { > > > + .suppress_auto_claim_dma_owner = true, > > > > The new .suppress_auto_claim_dma_owner controls whether we call > > iommu_device_set_dma_owner(). I guess you added > > .suppress_auto_claim_dma_owner because iommu_device_set_dma_owner() > > must be done *before* we call the driver's .probe() method? > > As explained above, all drivers are set to kernel-managed dma by > default. For those vfio and vfio-approved drivers, > suppress_auto_claim_dma_owner is used to tell the driver core that "this > driver is attached to device for userspace assignment purpose, do not > claim it for kernel-management dma". > > > Otherwise, we could call some new interface from .probe() instead of > > adding the flag to struct device_driver. > > Most device drivers are of the kernel-managed DMA type. Only a few vfio > and vfio-approved drivers need to use this flag. That's the reason why > we claim kernel-managed DMA by default. Yes. But you didn't answer the question of whether this must be done by a new flag in struct device_driver, or whether it could be done by having these few VFIO and "VFIO-approved" (whatever that means) drivers call a new interface. I was speculating that maybe the DMA ownership claiming must be done *before* the driver's .probe() method? If so, that would require a new flag. But I don't know whether that's the case. If DMA ownership could be claimed by the .probe() method, we wouldn't need the new flag in struct device_driver. Bjorn
On Thu, Dec 30, 2021 at 04:24:14PM -0600, Bjorn Helgaas wrote: > I was speculating that maybe the DMA ownership claiming must be done > *before* the driver's .probe() method? This is correct. > If DMA ownership could be claimed by the .probe() method, we > wouldn't need the new flag in struct device_driver. The other requirement is that every existing driver must claim ownership, so pushing this into the device driver's probe op would require revising almost every driver in Linux... In effect the new flag indicates if the driver will do the DMA ownership claim in it's probe, or should use the default claim the core code does. In almost every case a driver should do a claim. A driver like pci-stub, or a bridge, that doesn't actually operate MMIO on the device would be the exception. Jason
On 12/31/21 6:24 AM, Bjorn Helgaas wrote: > On Thu, Dec 30, 2021 at 01:34:27PM +0800, Lu Baolu wrote: >> Hi Bjorn, >> >> On 12/30/21 4:42 AM, Bjorn Helgaas wrote: >>> On Fri, Dec 17, 2021 at 02:36:58PM +0800, Lu Baolu wrote: >>>> The pci_dma_configure() marks the iommu_group as containing only devices >>>> with kernel drivers that manage DMA. >>> >>> I'm looking at pci_dma_configure(), and I don't see the connection to >>> iommu_groups. >> >> The 2nd patch "driver core: Set DMA ownership during driver bind/unbind" >> sets all drivers' DMA to be kernel-managed by default except a few ones >> which has a driver flag set. So by default, all iommu groups contains >> only devices with kernel drivers managing DMA. > > It looks like that happens in device_dma_configure(), not > pci_dma_configure(). > >>>> Avoid this default behavior for the >>>> pci_stub because it does not program any DMA itself. This allows the >>>> pci_stub still able to be used by the admin to block driver binding after >>>> applying the DMA ownership to vfio. >>> >>>> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>>> --- >>>> drivers/pci/pci-stub.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c >>>> index e408099fea52..6324c68602b4 100644 >>>> --- a/drivers/pci/pci-stub.c >>>> +++ b/drivers/pci/pci-stub.c >>>> @@ -36,6 +36,9 @@ static struct pci_driver stub_driver = { >>>> .name = "pci-stub", >>>> .id_table = NULL, /* only dynamic id's */ >>>> .probe = pci_stub_probe, >>>> + .driver = { >>>> + .suppress_auto_claim_dma_owner = true, >>> >>> The new .suppress_auto_claim_dma_owner controls whether we call >>> iommu_device_set_dma_owner(). I guess you added >>> .suppress_auto_claim_dma_owner because iommu_device_set_dma_owner() >>> must be done *before* we call the driver's .probe() method? >> >> As explained above, all drivers are set to kernel-managed dma by >> default. For those vfio and vfio-approved drivers, >> suppress_auto_claim_dma_owner is used to tell the driver core that "this >> driver is attached to device for userspace assignment purpose, do not >> claim it for kernel-management dma". >> >>> Otherwise, we could call some new interface from .probe() instead of >>> adding the flag to struct device_driver. >> >> Most device drivers are of the kernel-managed DMA type. Only a few vfio >> and vfio-approved drivers need to use this flag. That's the reason why >> we claim kernel-managed DMA by default. > > Yes. But you didn't answer the question of whether this must be done > by a new flag in struct device_driver, or whether it could be done by > having these few VFIO and "VFIO-approved" (whatever that means) > drivers call a new interface. > > I was speculating that maybe the DMA ownership claiming must be done > *before* the driver's .probe() method? If so, that would require a > new flag. But I don't know whether that's the case. If DMA > ownership could be claimed by the .probe() method, we wouldn't need > the new flag in struct device_driver. Yes. It's feasible. Hence we can remove the suppress flag which is only for some special drivers. I will come up with a new version so that you can further comment with the real code. Thank you! > > Bjorn > Best regards, baolu
Hi Jason, On 12/31/21 8:40 AM, Jason Gunthorpe wrote: > On Thu, Dec 30, 2021 at 04:24:14PM -0600, Bjorn Helgaas wrote: > >> I was speculating that maybe the DMA ownership claiming must be done >> *before* the driver's .probe() method? > > This is correct. > >> If DMA ownership could be claimed by the .probe() method, we >> wouldn't need the new flag in struct device_driver. > > The other requirement is that every existing driver must claim > ownership, so pushing this into the device driver's probe op would > require revising almost every driver in Linux... > > In effect the new flag indicates if the driver will do the DMA > ownership claim in it's probe, or should use the default claim the > core code does. > > In almost every case a driver should do a claim. A driver like > pci-stub, or a bridge, that doesn't actually operate MMIO on the > device would be the exception. We still need to call iommu_device_use_dma_api() in bus dma_configure() callback. But we can call iommu_device_unuse_dma_api() in the .probe() of vfio (and vfio-approved) drivers, so that we don't need the new flag anymore. > > Jason > Best regards, baolu
On 12/31/21 9:10 AM, Lu Baolu wrote: > > On 12/31/21 8:40 AM, Jason Gunthorpe wrote: >> On Thu, Dec 30, 2021 at 04:24:14PM -0600, Bjorn Helgaas wrote: >> >>> I was speculating that maybe the DMA ownership claiming must be done >>> *before* the driver's .probe() method? >> >> This is correct. >> >>> If DMA ownership could be claimed by the .probe() method, we >>> wouldn't need the new flag in struct device_driver. >> >> The other requirement is that every existing driver must claim >> ownership, so pushing this into the device driver's probe op would >> require revising almost every driver in Linux... >> >> In effect the new flag indicates if the driver will do the DMA >> ownership claim in it's probe, or should use the default claim the >> core code does. >> >> In almost every case a driver should do a claim. A driver like >> pci-stub, or a bridge, that doesn't actually operate MMIO on the >> device would be the exception. > > We still need to call iommu_device_use_dma_api() in bus dma_configure() > callback. But we can call iommu_device_unuse_dma_api() in the .probe() > of vfio (and vfio-approved) drivers, so that we don't need the new flag > anymore. Oh, wait. I didn't think about the hot-plug case. If we call iommu_device_use_dma_api() in bus dma_configure() anyway, we can't bind any (no matter vfio or none-vfio) driver to a device if it's group has already been assigned to user space. It seems that we can't omit this flag. Best regards, baolu
On Fri, Dec 31, 2021 at 09:10:43AM +0800, Lu Baolu wrote: > We still need to call iommu_device_use_dma_api() in bus dma_configure() > callback. But we can call iommu_device_unuse_dma_api() in the .probe() > of vfio (and vfio-approved) drivers, so that we don't need the new flag > anymore. No, we can't. The action that iommu_device_use_dma_api() takes is to not call probe, it obviously cannot be undone by code inside probe. Jason
On 1/4/22 3:53 AM, Jason Gunthorpe wrote: > On Fri, Dec 31, 2021 at 09:10:43AM +0800, Lu Baolu wrote: > >> We still need to call iommu_device_use_dma_api() in bus dma_configure() >> callback. But we can call iommu_device_unuse_dma_api() in the .probe() >> of vfio (and vfio-approved) drivers, so that we don't need the new flag >> anymore. > > No, we can't. The action that iommu_device_use_dma_api() takes is to > not call probe, it obviously cannot be undone by code inside probe. Yes. Agreed. > Jason > Best regards, baolu
diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c index e408099fea52..6324c68602b4 100644 --- a/drivers/pci/pci-stub.c +++ b/drivers/pci/pci-stub.c @@ -36,6 +36,9 @@ static struct pci_driver stub_driver = { .name = "pci-stub", .id_table = NULL, /* only dynamic id's */ .probe = pci_stub_probe, + .driver = { + .suppress_auto_claim_dma_owner = true, + }, }; static int __init pci_stub_init(void)
The pci_dma_configure() marks the iommu_group as containing only devices with kernel drivers that manage DMA. Avoid this default behavior for the pci_stub because it does not program any DMA itself. This allows the pci_stub still able to be used by the admin to block driver binding after applying the DMA ownership to vfio. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/pci/pci-stub.c | 3 +++ 1 file changed, 3 insertions(+)