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", );
> >>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, );
> >>
> >>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(>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
