Re: [Devel] [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801

[Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>]

[-- Attachment #1: Type: text/plain, Size: 5550 bytes --]

On Tue, Sep 05, 2017 at 12:07:59PM +0100, John Garry wrote:
> >>
> >>Hi Will, Lorenzo, Robin,
> >>
> >>I have created the patch to add DT support for this erratum.
> >>However, currently I have only added support for pci-based devices.
> >>I'm a bit stumped on how to add platform device support, or if we
> >>should also add support at all. And I would rather ask before
> >>sending the patches.
> >>
> >>The specific issue is that I know of no platform device with an ITS
> >>msi-parent which we would want reserve, i.e. do not translate msi.
> >>And, if there were, how to do it.
> >>
> >>The only platform devices I know off with msi ITS parents are SMMUv3
> >>and mbigen. For mbigen, the msi are not translated. Actually, as I
> >>see under IORT solution, for mbigen we don't reserve the hw msi
> >>region as the SMMUv3 does not have an ID mapping. And we have no
> >>equivalent ID mapping property for DT SMMUv3, so cannot create an
> >>equivalent check.
> >>
> >>So here is how the DT iommu get reserved region function with only
> >>pci device support looks:
> >>
> >>/**
> >> * of_iommu_its_get_resv_regions - Reserved region driver helper
> >> * @dev: Device from iommu_get_resv_regions()
> >> * @list: Reserved region list from iommu_get_resv_regions()
> >> *
> >> * Returns: Number of reserved regions on success(0 if no associated ITS),
> >> *          appropriate error value otherwise.
> >> */
> >>int of_iommu_its_get_resv_regions(struct device *dev, struct
> >>list_head *head)
> >>{
> >>    struct device_node *of_node = NULL;
> >>    struct device *parent_dev;
> >>    const __be32 *msi_map;
> >>    int num_mappings, i, err, len, resv = 0;
> >>
> >>    /* walk up to find the parent with a msi-map property */
> >>    for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >>        if (!parent_dev->of_node)
> >>            continue;
> >>
> >>        /*
> >>         * Current logic to reserve ITS regions for only PCI root
> >>         * complex.
> >>         */
> >>        msi_map = of_get_property(parent_dev->of_node, "msi-map", &len);
> >>        if (msi_map) {
> >>            of_node = parent_dev->of_node;
> >>            break;
> >>        }
> >>    }
> >>
> >>    if (!of_node)
> >>        return 0;
> >>
> >>    num_mappings = of_count_phandle_with_args(of_node, "msi-map",
> >>NULL) / 4;
> >>
> >>    for (i = 0; i < num_mappings; i++) {
> >>        int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >>        struct iommu_resv_region *region;
> >>        struct device_node *msi_node;
> >>        struct resource resource;
> >>        u32 phandle;
> >>
> >>        phandle = be32_to_cpup(msi_map + 1);
> >>        msi_node = of_find_node_by_phandle(phandle);
> >>        if (!msi_node)
> >>            return -ENODEV;
> >>
> >>        /*
> >>         * There may be different types of msi-controller, so check
> >>         * for ITS.
> >>         */
> >>        if (!of_device_is_compatible(msi_node, "arm,gic-v3-its")) {
> >>            of_node_put(msi_node);
> >>            continue;
> >>        }
> >>
> >>        err = of_address_to_resource(msi_node, 0, &resource);
> >>
> >>        of_node_put(msi_node);
> >>        if (err)
> >>            return err;
> >>
> >>        region = iommu_alloc_resv_region(resource.start, SZ_128K, prot,
> >>                         IOMMU_RESV_MSI);
> >>        if (region) {
> >>            list_add_tail(&region->list, head);
> >>            resv++;
> >>        }
> >>    }
> >>
> >>    return (resv == num_mappings) ? resv : -ENODEV;
> >>}
> >>
> >>Any feedback is appreciated.
> >
> >Hi John,
> >
> >I appreciate it is not trivial to make the code generic, part of the
> >snippet above can be shared between ACPI/IORT and OF, the only sticking
> >point is probably the "compatible" string that has to be parameterized
> >since having this code in generic IOMMU layer is a bit horrible to
> >have it ITS specific (and it is a problem that was existing already
> >in the original patch with the hardcoded ITS node type or function
> >name).
> 
> Hi Lorenzo,
> 
> Agreed, checking the ITS compatible string in IOMMU code is not
> nice. However the function is just trying to replicate our ACPI
> version, which calls IORT code directly, and this is ITS specific.
> Anyway, we can make it generic.
> 
> >
> >To sum it up the hook:
> >
> >- has to be called from SMMU driver in a firmware agnostic way
> 
> ok
> 
> >- somehow it has to ascertain which interrupt controller "compatible"
> >  (which in IORT is a node type) to match against so the hook has to
> >  take an id of sorts
> 
> OK
> 
> I will mention 2 more points after discussion on OF solution with Shameer:
> - for platform device, we can add suppport by checking for the
> devices msi-parent property
> - for both pci and platform device, we should check device rid for
> msi-controller also, like IORT solution
> 
> BTW, even though merge window is open, we would like to send some
> solution to the lists this week, so any outstanding topics can
> potentially be discussed at LPC next week. Let me know if you're
> unhappy about this.

I am happy to find a way forward for this next week, posting patches
this week would not help much with the merge window ongoing, I think
it is better to try to find a way forward and post the resulting code
after reaching an agreement, at -rc1 (or right after the IOMMU PR is
merged).

Thanks,
Lorenzo