| Message ID | f3ba9136-068a-280d-2849-f3a0081c1d90@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: Thursday, July 20, 2017 3:32 PM > To: Shameerali Kolothum Thodi; Will Deacon > Cc: lorenzo.pieralisi@arm.com; marc.zyngier@arm.com; > sudeep.holla@arm.com; hanjun.guo@linaro.org; Gabriele Paoloni; John > Garry; Linuxarm; linux-acpi@vger.kernel.org; iommu@lists.linux- > foundation.org; Wangzhou (B); Guohanjun (Hanjun Guo); linux-arm- > kernel@lists.infradead.org; devel@acpica.org > Subject: Re: [PATCH v3 2/2] iommu/arm-smmu-v3:Enable ACPI based > HiSilicon erratum 161010801 > > On 19/07/17 11:48, Shameerali Kolothum Thodi wrote: > > > > > >> -----Original Message----- > >> From: Will Deacon [mailto:will.deacon@arm.com] > >> Sent: Friday, July 14, 2017 8:33 PM > >> To: Shameerali Kolothum Thodi > >> Cc: lorenzo.pieralisi@arm.com; marc.zyngier@arm.com; > >> sudeep.holla@arm.com; robin.murphy@arm.com; > hanjun.guo@linaro.org; > >> Gabriele Paoloni; John Garry; Linuxarm; linux-acpi@vger.kernel.org; > >> iommu@lists.linux-foundation.org; Wangzhou (B); Guohanjun (Hanjun > Guo); > >> linux-arm-kernel@lists.infradead.org; devel@acpica.org > >> Subject: Re: [PATCH v3 2/2] iommu/arm-smmu-v3:Enable ACPI based > >> HiSilicon erratum 161010801 > >> > > [...] > >>>>> - list_add_tail(®ion->list, head); > >>>>> + if ((smmu->options & ARM_SMMU_OPT_RESV_HW_MSI)) { > >>>>> + > >>>>> + if (!is_of_node(smmu->dev->fwnode)) > >>>>> + resv = > iort_iommu_its_get_resv_regions(dev, head); > >>>> > >>>> How does this work when we're not using ACPI? Shouldn't of vs ACPI > >>>> be abstracted from the driver? > >>> > >>> At present ARM_SMMU_OPT_RESV_HW_MSI is only set for ACPI and > DT > >>> support for this is a low priority for us at the moment. Is the > >>> suggestion is to have a common function outside the smmu driver for > >>> _iommu_its_get_resv_regions() ? I am not sure what is the best way > here. > >> > >> Right, something like that. The driver shouldn't need to care whether or > not > >> it's using ACPI or DT when setting these options. > > > > Below is what I have in mind for the common function for msi reserve. > > But just wondering invoking iort_ functions from iommu code > > is acceptable or not . Could you please take a look and let me know. > > At that point, it seems like we might as well just roll it into > iommu_dma_get_resv_regions() directly[1]. It probably makes sense for > any DT equivalent to be described generically, rather than > SMMU-specific, so parsing that would fit into common code as well. > > Then in the SMMU drivers we can skip creating the SW_MSI region if > iommu-dma gave us back any real ones (and remove the apparently > unnecessary resv_msi check in VFIO). Or be lazy and just leave it, as it > doesn't seem to do much harm to have both. Ok, If I read that correctly, we don’t need any changes to SMMU driver for now and this will be a generic change rather than a HiSi quirk. So is it ok, if I just send the patch#1 rebased on 4.13-rc1? Thanks, Shameer > > [1] This is what I hacked up locally on top of patch #1: > ----->8----- > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 9d1cebe7f6cb..50292827da49 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -19,6 +19,7 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/acpi_iort.h> > #include <linux/device.h> > #include <linux/dma-iommu.h> > #include <linux/gfp.h> > @@ -174,6 +175,10 @@ void iommu_dma_get_resv_regions(struct device > *dev, > struct list_head *list) > struct pci_host_bridge *bridge; > struct resource_entry *window; > > + if (!is_of_node(dev->iommu_fwspec->iommu_fwnode) && > + iort_iommu_its_get_resv_regions(dev, list) < 0) > + return; > + > if (!dev_is_pci(dev)) > return;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 9d1cebe7f6cb..50292827da49 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -19,6 +19,7 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/acpi_iort.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/gfp.h> @@ -174,6 +175,10 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list) struct pci_host_bridge *bridge; struct resource_entry *window; + if (!is_of_node(dev->iommu_fwspec->iommu_fwnode) && + iort_iommu_its_get_resv_regions(dev, list) < 0) + return; + if (!dev_is_pci(dev)) return;