[PATCH v3 2/2] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: Thursday, July 20, 2017 3:32 PM
> To: Shameerali Kolothum Thodi; Will Deacon
> Cc: lorenzo.pieralisi at arm.com; marc.zyngier at arm.com;
> sudeep.holla at arm.com; hanjun.guo at linaro.org; Gabriele Paoloni; John
> Garry; Linuxarm; linux-acpi at vger.kernel.org; iommu at lists.linux-
> foundation.org; Wangzhou (B); Guohanjun (Hanjun Guo); linux-arm-
> kernel at lists.infradead.org; devel at 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 at arm.com]
> >> Sent: Friday, July 14, 2017 8:33 PM
> >> To: Shameerali Kolothum Thodi
> >> Cc: lorenzo.pieralisi at arm.com; marc.zyngier at arm.com;
> >> sudeep.holla at arm.com; robin.murphy at arm.com;
> hanjun.guo at linaro.org;
> >> Gabriele Paoloni; John Garry; Linuxarm; linux-acpi at vger.kernel.org;
> >> iommu at lists.linux-foundation.org; Wangzhou (B); Guohanjun (Hanjun
> Guo);
> >> linux-arm-kernel at lists.infradead.org; devel at acpica.org
> >> Subject: Re: [PATCH v3 2/2] iommu/arm-smmu-v3:Enable ACPI based
> >> HiSilicon erratum 161010801
> >>
> > [...]
> >>>>> -	list_add_tail(&region->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;