| Message ID | 1461761010-5452-3-git-send-email-xyjxie@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 27, 2016 at 08:43:27PM +0800, Yongji Xie wrote: > The capability of IRQ remapping is abstracted on IOMMU side on > some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this. > > To have a universal flag to test this capability for different > archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses > when IOMMU_CAP_INTR_REMAP is set. > > Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> > --- > drivers/iommu/iommu.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 0e3b009..5d2b6f6 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev) > return group; > } > > +static void pci_check_msi_remapping(struct pci_dev *pdev, > + const struct iommu_ops *ops) > +{ > + struct pci_bus *bus = pdev->bus; > + > + if (ops->capable(IOMMU_CAP_INTR_REMAP) && > + !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) > + bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; > +} This looks an awful lot like the pci_bus_check_msi_remapping() you add elsewhere. Why do we need both? > /** > * iommu_group_get_for_dev - Find or create the IOMMU group for a device > * @dev: target device > @@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data) > const struct iommu_ops *ops = cb->ops; > int ret; > > + if (dev_is_pci(dev) && ops->capable) > + pci_check_msi_remapping(to_pci_dev(dev), ops); > + > if (!ops->add_device) > return 0; > > @@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb, > * result in ADD/DEL notifiers to group->notifier > */ > if (action == BUS_NOTIFY_ADD_DEVICE) { > + if (dev_is_pci(dev) && ops->capable) > + pci_check_msi_remapping(to_pci_dev(dev), ops); These calls don't smell right either. Why do we need dev_is_pci() checks here? Can't this be done in the PCI probe path somehow, e.g., in pci_set_bus_msi_domain() or something? > if (ops->add_device) > return ops->add_device(dev); > } else if (action == BUS_NOTIFY_REMOVED_DEVICE) { > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/5/25 5:11, Bjorn Helgaas wrote: > On Wed, Apr 27, 2016 at 08:43:27PM +0800, Yongji Xie wrote: >> The capability of IRQ remapping is abstracted on IOMMU side on >> some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this. >> >> To have a universal flag to test this capability for different >> archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses >> when IOMMU_CAP_INTR_REMAP is set. >> >> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> --- >> drivers/iommu/iommu.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 0e3b009..5d2b6f6 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev) >> return group; >> } >> >> +static void pci_check_msi_remapping(struct pci_dev *pdev, >> + const struct iommu_ops *ops) >> +{ >> + struct pci_bus *bus = pdev->bus; >> + >> + if (ops->capable(IOMMU_CAP_INTR_REMAP) && >> + !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) >> + bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; >> +} > This looks an awful lot like the pci_bus_check_msi_remapping() you add > elsewhere. Why do we need both? I will modify this function as you suggested. And we add this function here because some iommu drivers would be initialed after PCI probing. >> /** >> * iommu_group_get_for_dev - Find or create the IOMMU group for a device >> * @dev: target device >> @@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data) >> const struct iommu_ops *ops = cb->ops; >> int ret; >> >> + if (dev_is_pci(dev) && ops->capable) >> + pci_check_msi_remapping(to_pci_dev(dev), ops); >> + >> if (!ops->add_device) >> return 0; >> >> @@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb, >> * result in ADD/DEL notifiers to group->notifier >> */ >> if (action == BUS_NOTIFY_ADD_DEVICE) { >> + if (dev_is_pci(dev) && ops->capable) >> + pci_check_msi_remapping(to_pci_dev(dev), ops); > These calls don't smell right either. Why do we need dev_is_pci() > checks here? Some platform devices may also call this. > Can't this be done in the PCI probe path somehow, e.g., > in pci_set_bus_msi_domain() or something? > Yes, this can be done in pci_create_root_bus(). But it could only handle the case that iommu drivers are initialed before PCI probing. Regards, Yongji -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 01:54:23PM +0800, Yongji Xie wrote: > On 2016/5/25 5:11, Bjorn Helgaas wrote: > >On Wed, Apr 27, 2016 at 08:43:27PM +0800, Yongji Xie wrote: > >>The capability of IRQ remapping is abstracted on IOMMU side on > >>some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this. > >> > >>To have a universal flag to test this capability for different > >>archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses > >>when IOMMU_CAP_INTR_REMAP is set. > >> > >>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> > >>--- > >> drivers/iommu/iommu.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >>index 0e3b009..5d2b6f6 100644 > >>--- a/drivers/iommu/iommu.c > >>+++ b/drivers/iommu/iommu.c > >>@@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev) > >> return group; > >> } > >>+static void pci_check_msi_remapping(struct pci_dev *pdev, > >>+ const struct iommu_ops *ops) > >>+{ > >>+ struct pci_bus *bus = pdev->bus; > >>+ > >>+ if (ops->capable(IOMMU_CAP_INTR_REMAP) && > >>+ !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) > >>+ bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; > >>+} > >This looks an awful lot like the pci_bus_check_msi_remapping() you add > >elsewhere. Why do we need both? > > I will modify this function as you suggested. And we add this function > here because some iommu drivers would be initialed after PCI probing. > > >> /** > >> * iommu_group_get_for_dev - Find or create the IOMMU group for a device > >> * @dev: target device > >>@@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data) > >> const struct iommu_ops *ops = cb->ops; > >> int ret; > >>+ if (dev_is_pci(dev) && ops->capable) > >>+ pci_check_msi_remapping(to_pci_dev(dev), ops); > >>+ > >> if (!ops->add_device) > >> return 0; > >>@@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb, > >> * result in ADD/DEL notifiers to group->notifier > >> */ > >> if (action == BUS_NOTIFY_ADD_DEVICE) { > >>+ if (dev_is_pci(dev) && ops->capable) > >>+ pci_check_msi_remapping(to_pci_dev(dev), ops); > >These calls don't smell right either. Why do we need dev_is_pci() > >checks here? > > Some platform devices may also call this. > > >Can't this be done in the PCI probe path somehow, e.g., > >in pci_set_bus_msi_domain() or something? > > Yes, this can be done in pci_create_root_bus(). But it could only > handle the case that iommu drivers are initialed before PCI probing. Hmm, that's sort of what I expected, but I don't like it. I don't think initializing IOMMU drivers after probing PCI devices is the optimal design. As soon as we add a PCI device, a driver can bind to it and start using it. If we set up an IOMMU after a driver has claimed the device, something is going to break. The driver may have already made DMA mappings that would now be invalid. IOMMU drivers are logically between the CPU and the device, so they should be initialized before the device is enumerated. I know there are probably some legacy issues behind this design, and I know it has nothing to do with your patch, so this is not a very constructive comment. I just want to be on record that I'm not a fan of this out-of-order initialization, and I don't like the notification scheme for handling device hotplug either. Notifiers make the code hard to read, and you can't tell what order things happen in. It just seems like a hack to connect things together when they should really have been designed to be more tightly integrated from the beginning. Bjorn -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 0e3b009..5d2b6f6 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev) return group; } +static void pci_check_msi_remapping(struct pci_dev *pdev, + const struct iommu_ops *ops) +{ + struct pci_bus *bus = pdev->bus; + + if (ops->capable(IOMMU_CAP_INTR_REMAP) && + !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP)) + bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; +} + /** * iommu_group_get_for_dev - Find or create the IOMMU group for a device * @dev: target device @@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data) const struct iommu_ops *ops = cb->ops; int ret; + if (dev_is_pci(dev) && ops->capable) + pci_check_msi_remapping(to_pci_dev(dev), ops); + if (!ops->add_device) return 0; @@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb, * result in ADD/DEL notifiers to group->notifier */ if (action == BUS_NOTIFY_ADD_DEVICE) { + if (dev_is_pci(dev) && ops->capable) + pci_check_msi_remapping(to_pci_dev(dev), ops); if (ops->add_device) return ops->add_device(dev); } else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
The capability of IRQ remapping is abstracted on IOMMU side on some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this. To have a universal flag to test this capability for different archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses when IOMMU_CAP_INTR_REMAP is set. Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> --- drivers/iommu/iommu.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)