[Hi Ilya, I think your response didn't go to the list because it was
not plain text only (please fix your email client). I'm inserting
your response manually below.]
Ilya wrote:
> On Thu, Feb 25, 2016 at 09:37:26AM -0600, Bjorn Helgaas wrote:
> > On Thu, Feb 04, 2016 at 10:28:55AM +0200, Ilya Lesokhin wrote:
> > > Add a new PCI_DEV_FLAGS_UNTRUSTED to indicate that a PCI device
> > > is probed by a driver that gives untrusted entities access to that device.
> > > Make iommu_group_get_for_pci_dev check the new flag when an IOMMU
> > > group is selected for a virtual function.
> > > Mark VFIO devices with the new flag.
> > >
> > > Signed-off-by: Ilya Lesokhin <ilyal@xxxxxxxxxxxx>
> > > ---
> > > drivers/iommu/iommu.c | 4 ++++
> > > drivers/vfio/pci/vfio_pci.c | 3 +++
> > > include/linux/pci.h | 1 +
> > > 3 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 049df49..864b459 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -738,6 +738,10 @@ static struct iommu_group *iommu_group_get_for_pci_dev(struct pci_dev *pdev)
> > > struct iommu_group *group = NULL;
> > > u64 devfns[4] = { 0 };
> > >
> > > + if (pdev->is_virtfn &&
> > > + (pdev->physfn->dev_flags & PCI_DEV_FLAGS_UNTRUSTED))
> > > + return iommu_group_get(&pdev->physfn->dev);
> > > +
> > > /*
> > > * Find the upstream DMA alias for the device. A device must not
> > > * be aliased due to topology in order to have its own IOMMU group.
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index 964ad57..ddcfd2c 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -982,6 +982,8 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > pci_set_power_state(pdev, PCI_D3hot);
> > > }
> > >
> > > + pdev->dev_flags |= PCI_DEV_FLAGS_UNTRUSTED;
> > > +
> > > return ret;
> > > }
> > >
> > > @@ -989,6 +991,7 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> > > {
> > > struct vfio_pci_device *vdev;
> > >
> > > + pdev->dev_flags &= ~PCI_DEV_FLAGS_UNTRUSTED;
> > > vdev = vfio_del_group_dev(&pdev->dev);
> > > if (!vdev)
> > > return;
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index e90eb22..6330327 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -182,6 +182,7 @@ enum pci_dev_flags {
> > > PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> > > /* Get VPD from function 0 VPD */
> > > PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> > > + PCI_DEV_FLAGS_UNTRUSTED = (__force pci_dev_flags_t) (1 << 9),
> >
> > I'm raising my eyebrows a bit at this. PCI_DEV_FLAGS_UNTRUSTED
> > doesn't seem like a PCI core property, so it seems like the PCI core
> > is an innocent bystander here (it neither sets nor checks the flag),
> > and you're asking it to keep track of bookkeeping details for other
> > unrelated entities.
>
> PCI_DEV_FLAGS_UNTRUSTED is quite similar to
> PCI_DEV_FLAGS_DMA_ALIAS_DEVFN,
>
> they both indicate that a device needs to be put in the same IOMMU
> group as another device.
>
> In fact, we initially though about overloading the meaning of
> PCI_DEV_FLAGS_DMA_ALIAS_DEVFN
>
> To force the VFs in the same group as the PF (if its probed by VFIO).
It's true that it's similar to PCI_DEV_FLAGS_DMA_ALIAS_DEVFN. I don't
really like that either :)
There's a little bit of current discussion about that here:
http://lkml.kernel.org/r/20160224194406.7585.17447.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
I don't know if I have a better suggestion yet. Having a real PCI
interface, even if just simple wrappers that set/test the bit, at
least provides a place for an explanatory comment, so that would be a
little better in my mind.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
