Re: 0b6bacc7874f - main - Add ofw_bus_iommu_map() that maps PCI requester ID to an IOMMU specifier based on "iommu-map" DTS property.
Date: Fri, 06 May 2022 18:44:01 UTC
On Fri, May 6, 2022 at 11:31 AM Ruslan Bukin <br@freebsd.org> wrote: > > 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. > Oh, sorry, I'm wrong about this still holding. What I was talking about specifically was: + for (i = 0; i < len; i += 4) { i is an index into map, but len should be the number of bytes (sizeof(pcell_t) * 4, so 16 in all of your cases) that was allocated for map rather than the number of entries in it. i.e., this should have been i * sizeof(pcell_t) or len / sizeof(pcell_t) as a terminal. But this is the difference between OF_*_multi() and the non-multi versions, the non-multi versions return the size in bytes of the map while the multi versions return the number of cells.