* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization [not found] ` <20170630052436.15212-5-aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> @ 2017-07-10 19:23 ` Bjorn Helgaas via iommu [not found] ` <CAErSpo4pAZfDx5p_S9Z8jR_ctH=ZrkgG6aNaNmPaN2H77dgEgQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-07-19 10:02 ` Alexey Kardashevskiy 0 siblings, 2 replies; 8+ messages in thread From: Bjorn Helgaas via iommu @ 2017-07-10 19:23 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, open list:INTEL IOMMU (VT-d), Yongji Xie, Yongji Xie, David Gibson [+cc Joerg, iommu] On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote: > From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > Some iommu drivers would be initialized after PCI device > enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set > when probing PCI devices although IOMMU enables capability > of IRQ remapping. This patch tests this capability and > set the flag when iommu driver is initialized. > > Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> > Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> > --- > drivers/iommu/iommu.c | 8 ++++++++ > drivers/pci/probe.c | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index cf7ca7e70777..0b5881ddca09 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) > const struct iommu_ops *ops = cb->ops; > int ret; > > + /* > + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU > + * have capability of IRQ remapping. > + */ > + if (dev_is_pci(dev) && ops->capable && > + ops->capable(IOMMU_CAP_INTR_REMAP)) > + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; This isn't my area, but this addition is really ugly. It doesn't look anything like the rest of the code in add_iommu_group(), so it just feels like a wart. > if (!ops->add_device) > return 0; > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index f2393b7d7ebf..14aac9df3d17 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -17,6 +17,7 @@ > #include <linux/acpi.h> > #include <linux/irqdomain.h> > #include <linux/pm_runtime.h> > +#include <linux/iommu.h> This obviously belongs in another patch, as the compile error showed. > #include "pci.h" > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAErSpo4pAZfDx5p_S9Z8jR_ctH=ZrkgG6aNaNmPaN2H77dgEgQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization [not found] ` <CAErSpo4pAZfDx5p_S9Z8jR_ctH=ZrkgG6aNaNmPaN2H77dgEgQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-07-11 10:39 ` Robin Murphy 2017-07-12 2:47 ` Alexey Kardashevskiy 2017-07-19 5:12 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 8+ messages in thread From: Robin Murphy @ 2017-07-11 10:39 UTC (permalink / raw) To: Bjorn Helgaas, Alexey Kardashevskiy Cc: Yongji Xie, open list:INTEL IOMMU (VT-d), Yongji Xie, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, David Gibson On 10/07/17 20:23, Bjorn Helgaas via iommu wrote: > [+cc Joerg, iommu] > > On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> wrote: >> From: Yongji Xie <elohimes-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> >> Some iommu drivers would be initialized after PCI device >> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set >> when probing PCI devices although IOMMU enables capability >> of IRQ remapping. This patch tests this capability and >> set the flag when iommu driver is initialized. >> >> Signed-off-by: Yongji Xie <xyjxie-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> >> Signed-off-by: Alexey Kardashevskiy <aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> >> --- >> drivers/iommu/iommu.c | 8 ++++++++ >> drivers/pci/probe.c | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index cf7ca7e70777..0b5881ddca09 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) >> const struct iommu_ops *ops = cb->ops; >> int ret; >> >> + /* >> + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU >> + * have capability of IRQ remapping. >> + */ >> + if (dev_is_pci(dev) && ops->capable && >> + ops->capable(IOMMU_CAP_INTR_REMAP)) >> + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; > > This isn't my area, but this addition is really ugly. It doesn't look > anything like the rest of the code in add_iommu_group(), so it just > feels like a wart. I have no idea what the context is here, but this flag looks wrong generally. IRQ remapping is a property of the irqchip and has nothing to do with PCI, so pretending it's a property of PCI buses looks like a massive hack around... something. Also, iommu_capable() is a fundamentally broken and unworkable interface anyway. The existing IRQ remapping code really wants updating to advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that IOMMU_CAP_INTR_REMAP can go away for good. That way, all consumers who actually care about whether IRQ remapping is implemented (see e.g. VFIO) can use irq_domain_check_msi_remap() or check specific devices in a sane and scalable manner. Robin. >> if (!ops->add_device) >> return 0; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index f2393b7d7ebf..14aac9df3d17 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -17,6 +17,7 @@ >> #include <linux/acpi.h> >> #include <linux/irqdomain.h> >> #include <linux/pm_runtime.h> >> +#include <linux/iommu.h> > > This obviously belongs in another patch, as the compile error showed. > >> #include "pci.h" >> >> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >> -- >> 2.11.0 >> > _______________________________________________ > iommu mailing list > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization 2017-07-11 10:39 ` Robin Murphy @ 2017-07-12 2:47 ` Alexey Kardashevskiy 2017-07-19 5:12 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-07-12 2:47 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas Cc: kvm@vger.kernel.org, open list:INTEL IOMMU (VT-d), Yongji Xie, Yongji Xie, David Gibson On 11/07/17 20:39, Robin Murphy wrote: > On 10/07/17 20:23, Bjorn Helgaas via iommu wrote: >> [+cc Joerg, iommu] >> >> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>> From: Yongji Xie <elohimes@gmail.com> >>> >>> Some iommu drivers would be initialized after PCI device >>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set >>> when probing PCI devices although IOMMU enables capability >>> of IRQ remapping. This patch tests this capability and >>> set the flag when iommu driver is initialized. >>> >>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> drivers/iommu/iommu.c | 8 ++++++++ >>> drivers/pci/probe.c | 1 + >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index cf7ca7e70777..0b5881ddca09 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) >>> const struct iommu_ops *ops = cb->ops; >>> int ret; >>> >>> + /* >>> + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU >>> + * have capability of IRQ remapping. >>> + */ >>> + if (dev_is_pci(dev) && ops->capable && >>> + ops->capable(IOMMU_CAP_INTR_REMAP)) >>> + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; >> >> This isn't my area, but this addition is really ugly. It doesn't look >> anything like the rest of the code in add_iommu_group(), so it just >> feels like a wart. > > I have no idea what the context is here, but this flag looks wrong > generally. IRQ remapping is a property of the irqchip and has nothing to > do with PCI, so pretending it's a property of PCI buses looks like a > massive hack around... something. The context here - should we allow mapping of MSIX BAR to a guest or not. Now it is disabled as the guest can write there anything and make device send MSIX messages to random addresses. However if a PCI bridge can somehow filter these writes, then it is safe. So it is not really remapping what we care about in this patchset, it is filtering/censoring. And the reason to allow MSIX mapping is that it may be adjacent to frequently used MMIO and also reside within same 64K page which happens to be the default page size on PPC64. So even though remapping/filtering is not a property of any PCI bus but it is a part on an IOMMU which is usually (always?) combined with the PCI host bridge adapter so the PCI bus flag makes sense. How else would we pass that flag from IOMMU to the actual device we want to mmap to the userspace? > Also, iommu_capable() is a fundamentally broken and unworkable interface > anyway. The existing IRQ remapping code really wants updating to > advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that > IOMMU_CAP_INTR_REMAP can go away for good. This IRQ_DOMAIN_FLAG_MSI_REMAP is used only on ARM, is there a plan/desire to use it in other places instead of capable(IOMMU_CAP_INTR_REMAP)? > That way, all consumers who > actually care about whether IRQ remapping is implemented (see e.g. VFIO) > can use irq_domain_check_msi_remap() or check specific devices in a sane > and scalable manner. What specific devices are you talking about here? Actual PCI controllers do not have control over MSIX remapping/filtering... > > Robin. > >>> if (!ops->add_device) >>> return 0; >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index f2393b7d7ebf..14aac9df3d17 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -17,6 +17,7 @@ >>> #include <linux/acpi.h> >>> #include <linux/irqdomain.h> >>> #include <linux/pm_runtime.h> >>> +#include <linux/iommu.h> >> >> This obviously belongs in another patch, as the compile error showed. >> >>> #include "pci.h" >>> >>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >>> -- >>> 2.11.0 >>> >> _______________________________________________ >> iommu mailing list >> iommu@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/iommu >> > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization 2017-07-11 10:39 ` Robin Murphy 2017-07-12 2:47 ` Alexey Kardashevskiy @ 2017-07-19 5:12 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 8+ messages in thread From: Benjamin Herrenschmidt @ 2017-07-19 5:12 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas, Alexey Kardashevskiy Cc: kvm@vger.kernel.org, open list:INTEL IOMMU (VT-d), Yongji Xie, Yongji Xie, David Gibson On Tue, 2017-07-11 at 11:39 +0100, Robin Murphy wrote: > I have no idea what the context is here, but this flag looks wrong > generally. IRQ remapping is a property of the irqchip and has nothing to > do with PCI, so pretending it's a property of PCI buses looks like a > massive hack around... something. > > Also, iommu_capable() is a fundamentally broken and unworkable interface > anyway. The existing IRQ remapping code really wants updating to > advertise IRQ_DOMAIN_FLAG_MSI_REMAP on the relevant MSI domains so that > IOMMU_CAP_INTR_REMAP can go away for good. That way, all consumers who > actually care about whether IRQ remapping is implemented (see e.g. VFIO) > can use irq_domain_check_msi_remap() or check specific devices in a sane > and scalable manner. As you rightfully said, you have no idea what the context is :-) This is not an interrupt domain. On powerpc we have a single global unified domains that contains all our MSIs, IPIs, internally interrupts and what not, because of the way our interrupts infrastructure works. So there is no such thing as "a property of the MSI domain". The way the HW works is that the PCI Host Bridge has the ability to filter which device can access which MSIs. This is done by the IOMMU portion of the bridge. Thus it's a filtering capability, not a remapping capability per-se, and it's a property of the IOMMU domain. Sicne this is also a paravitualized interface, the "remapping" part is handled by the HV calls done by the guest to configure the MSIs. The HW filtering ensures that an evil guest cannot do bad things if it goes manually whack the MSI table. Now this issue have been discussed and patches floated around for *YEARS* now and there is still no upstream solution for what is a completely trivial problem: Don't bloody bloock MSI BAR table access on pseries platforms. It kills performance on a number of device due to our 64K pages. A 1-liner fix in qemu would have done it YEARS ago. But instead we have now painted about 1000 bike sheds and going without anything that actually works. Yay. Ben. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization 2017-07-10 19:23 ` [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization Bjorn Helgaas via iommu [not found] ` <CAErSpo4pAZfDx5p_S9Z8jR_ctH=ZrkgG6aNaNmPaN2H77dgEgQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-07-19 10:02 ` Alexey Kardashevskiy 2017-07-25 6:09 ` Alexey Kardashevskiy [not found] ` <b444851a-3240-f98f-04b9-0223649ea856-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> 1 sibling, 2 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-07-19 10:02 UTC (permalink / raw) To: Bjorn Helgaas, Robin Murphy Cc: kvm@vger.kernel.org, David Gibson, Alex Williamson, Yongji Xie, Eric Auger, Yongji Xie, Joerg Roedel, open list:INTEL IOMMU (VT-d), Jike Song, Benjamin Herrenschmidt, Paul Mackerras, linux-pci On 11/07/17 05:23, Bjorn Helgaas wrote: > [+cc Joerg, iommu] > > On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> From: Yongji Xie <elohimes@gmail.com> >> >> Some iommu drivers would be initialized after PCI device >> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set >> when probing PCI devices although IOMMU enables capability >> of IRQ remapping. This patch tests this capability and >> set the flag when iommu driver is initialized. >> >> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> drivers/iommu/iommu.c | 8 ++++++++ >> drivers/pci/probe.c | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index cf7ca7e70777..0b5881ddca09 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) >> const struct iommu_ops *ops = cb->ops; >> int ret; >> >> + /* >> + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU >> + * have capability of IRQ remapping. >> + */ >> + if (dev_is_pci(dev) && ops->capable && >> + ops->capable(IOMMU_CAP_INTR_REMAP)) >> + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; > > This isn't my area, but this addition is really ugly. It doesn't look > anything like the rest of the code in add_iommu_group(), so it just > feels like a wart. It does look like a wart, however it did not cause immediate rejection after first couple of revisions of this before I took it over... So. Here is the problem - deliver an MSIX isolation flag from IOMMU to VFIO-PCI driver. The options are: 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a PCI bus property and it is "like a wart". 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED and set it when an IOMMU group is created; VFIO-PCI has ways to get to the group and read the flag. 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP; others already use these IOMMU domains. VFIO-PCI's mmap() hook could then check the capability via iommu_capable(bus). The problems is as Robin said: "iommu_capable() is a fundamentally broken and unworkable interface anyway"; however it is still not clear to me why it is unworkable in this particular case of isolation checking. I am missing knowledge about ARM, is there a good overview of these MSIX domains vs. IOMMU on ARM (as MSIX remapping seems not to be an IOMMU property)? Which one to pick? Thanks. > >> if (!ops->add_device) >> return 0; >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index f2393b7d7ebf..14aac9df3d17 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -17,6 +17,7 @@ >> #include <linux/acpi.h> >> #include <linux/irqdomain.h> >> #include <linux/pm_runtime.h> >> +#include <linux/iommu.h> > > This obviously belongs in another patch, as the compile error showed. > >> #include "pci.h" >> >> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >> -- >> 2.11.0 >> -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization 2017-07-19 10:02 ` Alexey Kardashevskiy @ 2017-07-25 6:09 ` Alexey Kardashevskiy [not found] ` <b444851a-3240-f98f-04b9-0223649ea856-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> 1 sibling, 0 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-07-25 6:09 UTC (permalink / raw) To: Bjorn Helgaas, Robin Murphy Cc: kvm@vger.kernel.org, David Gibson, Alex Williamson, Yongji Xie, Eric Auger, Yongji Xie, Joerg Roedel, open list:INTEL IOMMU (VT-d), Jike Song, Benjamin Herrenschmidt, Paul Mackerras, linux-pci On 19/07/17 20:02, Alexey Kardashevskiy wrote: > On 11/07/17 05:23, Bjorn Helgaas wrote: >> [+cc Joerg, iommu] >> >> On Fri, Jun 30, 2017 at 12:24 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>> From: Yongji Xie <elohimes@gmail.com> >>> >>> Some iommu drivers would be initialized after PCI device >>> enumeration. So PCI_BUS_FLAGS_MSI_REMAP would not be set >>> when probing PCI devices although IOMMU enables capability >>> of IRQ remapping. This patch tests this capability and >>> set the flag when iommu driver is initialized. >>> >>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> drivers/iommu/iommu.c | 8 ++++++++ >>> drivers/pci/probe.c | 1 + >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index cf7ca7e70777..0b5881ddca09 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) >>> const struct iommu_ops *ops = cb->ops; >>> int ret; >>> >>> + /* >>> + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU >>> + * have capability of IRQ remapping. >>> + */ >>> + if (dev_is_pci(dev) && ops->capable && >>> + ops->capable(IOMMU_CAP_INTR_REMAP)) >>> + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; >> >> This isn't my area, but this addition is really ugly. It doesn't look >> anything like the rest of the code in add_iommu_group(), so it just >> feels like a wart. > > > It does look like a wart, however it did not cause immediate rejection > after first couple of revisions of this before I took it over... > > So. Here is the problem - deliver an MSIX isolation flag from IOMMU to > VFIO-PCI driver. The options are: > > 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a > PCI bus property and it is "like a wart". > > 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED > and set it when an IOMMU group is created; VFIO-PCI has ways to get to the > group and read the flag. > > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP; > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then > check the capability via iommu_capable(bus). The problems is as Robin said: > "iommu_capable() is a fundamentally broken and unworkable interface > anyway"; however it is still not clear to me why it is unworkable in this > particular case of isolation checking. > > I am missing knowledge about ARM, is there a good overview of these MSIX > domains vs. IOMMU on ARM (as MSIX remapping seems not to be an IOMMU property)? > > > Which one to pick? Thanks. Anyone, please? > > > >> >>> if (!ops->add_device) >>> return 0; >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index f2393b7d7ebf..14aac9df3d17 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -17,6 +17,7 @@ >>> #include <linux/acpi.h> >>> #include <linux/irqdomain.h> >>> #include <linux/pm_runtime.h> >>> +#include <linux/iommu.h> >> >> This obviously belongs in another patch, as the compile error showed. >> >>> #include "pci.h" >>> >>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >>> -- >>> 2.11.0 >>> > > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <b444851a-3240-f98f-04b9-0223649ea856-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>]
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization [not found] ` <b444851a-3240-f98f-04b9-0223649ea856-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org> @ 2017-07-26 9:50 ` Joerg Roedel [not found] ` <20170726095053.GG15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Joerg Roedel @ 2017-07-26 9:50 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: Jike Song, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Benjamin Herrenschmidt, open list:INTEL IOMMU (VT-d), Yongji Xie, Yongji Xie, linux-pci, Bjorn Helgaas, Paul Mackerras, David Gibson On Wed, Jul 19, 2017 at 08:02:04PM +1000, Alexey Kardashevskiy wrote: > On 11/07/17 05:23, Bjorn Helgaas wrote: > >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >> index cf7ca7e70777..0b5881ddca09 100644 > >> --- a/drivers/iommu/iommu.c > >> +++ b/drivers/iommu/iommu.c > >> @@ -1063,6 +1063,14 @@ static int add_iommu_group(struct device *dev, void *data) > >> const struct iommu_ops *ops = cb->ops; > >> int ret; > >> > >> + /* > >> + * Set PCI_BUS_FLAGS_MSI_REMAP for all PCI buses when IOMMU > >> + * have capability of IRQ remapping. > >> + */ > >> + if (dev_is_pci(dev) && ops->capable && > >> + ops->capable(IOMMU_CAP_INTR_REMAP)) > >> + to_pci_dev(dev)->bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP; We avoid bus-specific hacks in generic iommu code. This has to be done in bus-specific iommu-group setup code. > 1. PCI_BUS_FLAGS_MSI_REMAP on a PCI bus - MSIX isolation is not really a > PCI bus property and it is "like a wart". This one is at least debatable. It could be a property of the bus. > 2. Introduce "flags" in iommu_group and define IOMMU_GROUP_MSIX_ISOLATED > and set it when an IOMMU group is created; VFIO-PCI has ways to get to the > group and read the flag. That's the best option I see here. > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP; > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then > check the capability via iommu_capable(bus). The problems is as Robin said: > "iommu_capable() is a fundamentally broken and unworkable interface > anyway"; however it is still not clear to me why it is unworkable in this > particular case of isolation checking. That one is wrong, IRQ remapping is not a property of a domain. A domain is an abstraction for a device address space. Attaching IRQ information there is just wrong. Joerg ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20170726095053.GG15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization [not found] ` <20170726095053.GG15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> @ 2017-07-26 11:33 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 8+ messages in thread From: Benjamin Herrenschmidt @ 2017-07-26 11:33 UTC (permalink / raw) To: Joerg Roedel, Alexey Kardashevskiy Cc: Jike Song, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pci, open list:INTEL IOMMU (VT-d), Yongji Xie, Yongji Xie, Bjorn Helgaas, Paul Mackerras, David Gibson On Wed, 2017-07-26 at 11:50 +0200, Joerg Roedel wrote: > > 3. Create IOMMU_DOMAIN_UNMANAGED IOMMU domains for PPC64/powernv IOMMU > > groups and only define capable() hook to report IOMMU_CAP_INTR_REMAP; > > others already use these IOMMU domains. VFIO-PCI's mmap() hook could then > > check the capability via iommu_capable(bus). The problems is as Robin said: > > "iommu_capable() is a fundamentally broken and unworkable interface > > anyway"; however it is still not clear to me why it is unworkable in this > > particular case of isolation checking. > > That one is wrong, IRQ remapping is not a property of a domain. A domain > is an abstraction for a device address space. Attaching IRQ information > there is just wrong. Except it somewhat is ... an MSI is a store in the device address space, the way MSIs are handled and/or filtered can be considered a property of the domain. In our case, it's the exact same piece of HW that defines domains and which MSIs they are allowed to generate. Cheers, Ben. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-07-26 11:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170630052436.15212-1-aik@ozlabs.ru>
[not found] ` <20170630052436.15212-5-aik@ozlabs.ru>
[not found] ` <20170630052436.15212-5-aik-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
2017-07-10 19:23 ` [PATCH kernel v4 4/6] iommu: Set PCI_BUS_FLAGS_MSI_REMAP on iommu driver initialization Bjorn Helgaas via iommu
[not found] ` <CAErSpo4pAZfDx5p_S9Z8jR_ctH=ZrkgG6aNaNmPaN2H77dgEgQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-07-11 10:39 ` Robin Murphy
2017-07-12 2:47 ` Alexey Kardashevskiy
2017-07-19 5:12 ` Benjamin Herrenschmidt
2017-07-19 10:02 ` Alexey Kardashevskiy
2017-07-25 6:09 ` Alexey Kardashevskiy
[not found] ` <b444851a-3240-f98f-04b9-0223649ea856-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
2017-07-26 9:50 ` Joerg Roedel
[not found] ` <20170726095053.GG15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
2017-07-26 11:33 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).