Re: 0b6bacc7874f - main - Add ofw_bus_iommu_map() that maps PCI requester ID to an IOMMU specifier based on "iommu-map" DTS property.

On Fri, May 06, 2022 at 09:37:36AM -0700, Kyle Evans wrote:
> On Fri, May 6, 2022 at 9:25 AM Ravi Pokala <rpokala@freebsd.org> wrote:
> >
> > -----Original Message-----
> > From: <owner-src-committers@freebsd.org> on behalf of Ruslan Bukin <br@FreeBSD.org>
> > Date: 2022-05-06, Friday at 08:51
> > To: <src-committers@FreeBSD.org>, <dev-commits-src-all@FreeBSD.org>, <dev-commits-src-main@FreeBSD.org>
> > Subject: git: 0b6bacc7874f - main - Add ofw_bus_iommu_map() that maps PCI requester ID to an IOMMU specifier based on "iommu-map" DTS property.
> >
> >     The branch main has been updated by br:
> >
> >     URL: https://cgit.FreeBSD.org/src/commit/?id=0b6bacc7874f9c68ad35b06f1a7614171eb9ab10
> >
> >     commit 0b6bacc7874f9c68ad35b06f1a7614171eb9ab10
> >     Author:     Ruslan Bukin <br@FreeBSD.org>
> >     AuthorDate: 2022-05-06 15:41:11 +0000
> >     Commit:     Ruslan Bukin <br@FreeBSD.org>
> >     CommitDate: 2022-05-06 15:48:04 +0000
> >
> >         Add ofw_bus_iommu_map() that maps PCI requester ID to an IOMMU
> >         specifier based on "iommu-map" DTS property.
> >
> >         Sponsored by: UKRI
> >     ---
> >      sys/dev/ofw/ofw_bus_subr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >      sys/dev/ofw/ofw_bus_subr.h |  3 ++-
> >      2 files changed, 46 insertions(+), 1 deletion(-)
> >
> >     diff --git a/sys/dev/ofw/ofw_bus_subr.c b/sys/dev/ofw/ofw_bus_subr.c
> >     index ea57d1086779..408d554b3c7f 100644
> >     --- a/sys/dev/ofw/ofw_bus_subr.c
> >     +++ b/sys/dev/ofw/ofw_bus_subr.c
> >     @@ -491,6 +491,50 @@ ofw_bus_msimap(phandle_t node, uint16_t pci_rid, phandle_t *msi_parent,
> >         return (err);
> >      }
> >
> >     +int
> >     +ofw_bus_iommu_map(phandle_t node, uint16_t pci_rid, phandle_t *iommu_parent,
> >     +    uint32_t *iommu_rid)
> >     +{
> >     +   pcell_t mask, iommu_base, rid_base, rid_length;
> >     +   uint32_t masked_rid;
> >     +   pcell_t map[4];
> >     +   ssize_t len;
> >     +   int err, i;
> >     +
> >     +   len = OF_getproplen(node, "iommu-map");
> >     +   if (len <= 0)
> >     +           return (ENOENT);
> >     +        if (len > sizeof(map))
> >     +                return (ENOMEM);
> >     +
> >     +   len = OF_getencprop(node, "iommu-map", map, 16);
> >     +
> >     +   err = ENOENT;
> >     +   mask = 0xffffffff;
> >     +   OF_getencprop(node, "iommu-map-mask", &mask, sizeof(mask));
> >     +
> >     +   masked_rid = pci_rid & mask;
> >     +   for (i = 0; i < len; i += 4) {
> >     +           rid_base = map[i + 0];
> >     +           rid_length = map[i + 3];
> >
> > Hi Ruslan,
> >
> > I'm confused by this. 'map' is declared as a 4-element array, which means 'map[i + 0]' and 'map[i + 3]' are both only valid when 'i' is 0. But 'i' is the loop iterator; doesn't that mean you're accessing outside the array after the 0th time through the loop?
> >
> 
> The comparison looks like it's wrong and inherited from
> ofw_bus_msimap() -- i is in terms of # cells, OF_getencprop returns
> size in bytes, This version only holds one mapping (four cells) at a
> time, so the intention is just one iteration through the loop.
> 
> We'll need to extend this at some point, the M1 FDT has 3 4-cell mappings
> here. ofw_bus_msimap() was almost an exact fit, and an adapted version
> is what I use in my m1 branch.

iommu-map property is an arbitrary number of tuples of (rid-base,iommu,iommu-base,length).
Do you mean that some of the items in the tuple could occupy more than one 32-bit cell?
I grepped DTS tree, so far each of occurrence takes just 1 cell.

Ruslan