On Fri, May 01, 2015 at 02:01:17PM +1000, Alexey Kardashevskiy wrote: > On 04/29/2015 04:31 PM, David Gibson wrote: > >On Sat, Apr 25, 2015 at 10:14:50PM +1000, Alexey Kardashevskiy wrote: > >>In order to support memory pre-registration, we need a way to track > >>the use of every registered memory region and only allow unregistration > >>if a region is not in use anymore. So we need a way to tell from what > >>region the just cleared TCE was from. > >> > >>This adds a userspace view of the TCE table into iommu_table struct. > >>It contains userspace address, one per TCE entry. The table is only > >>allocated when the ownership over an IOMMU group is taken which means > >>it is only used from outside of the powernv code (such as VFIO). > >> > >>Signed-off-by: Alexey Kardashevskiy <[email protected]> > >>--- > >>Changes: > >>v9: > >>* fixed code flow in error cases added in v8 > >> > >>v8: > >>* added ENOMEM on failed vzalloc() > >>--- > >> arch/powerpc/include/asm/iommu.h | 6 ++++++ > >> arch/powerpc/kernel/iommu.c | 18 ++++++++++++++++++ > >> arch/powerpc/platforms/powernv/pci-ioda.c | 22 ++++++++++++++++++++-- > >> 3 files changed, 44 insertions(+), 2 deletions(-) > >> > >>diff --git a/arch/powerpc/include/asm/iommu.h > >>b/arch/powerpc/include/asm/iommu.h > >>index 7694546..1472de3 100644 > >>--- a/arch/powerpc/include/asm/iommu.h > >>+++ b/arch/powerpc/include/asm/iommu.h > >>@@ -111,9 +111,15 @@ struct iommu_table { > >> unsigned long *it_map; /* A simple allocation bitmap for now */ > >> unsigned long it_page_shift;/* table iommu page size */ > >> struct iommu_table_group *it_table_group; > >>+ unsigned long *it_userspace; /* userspace view of the table */ > > > >A single unsigned long doesn't seem like enough. > > Why single? This is an array.
As in single per page.
> > How do you know
> >which process's address space this address refers to?
>
> It is a current task. Multiple userspaces cannot use the same
> container/tables.
Where is that enforced?
More to the point, that's a VFIO constraint, but it's here affecting
the design of a structure owned by the platform code.
[snip]
> >> static void pnv_pci_ioda_setup_opal_tce_kill(struct pnv_phb *phb,
> >>@@ -2062,12 +2071,21 @@ static long pnv_pci_ioda2_create_table(struct
> >>iommu_table_group *table_group,
> >> int nid = pe->phb->hose->node;
> >> __u64 bus_offset = num ? pe->tce_bypass_base : 0;
> >> long ret;
> >>+ unsigned long *uas, uas_cb = sizeof(*uas) * (window_size >> page_shift);
> >>+
> >>+ uas = vzalloc(uas_cb);
> >>+ if (!uas)
> >>+ return -ENOMEM;
> >
> >I don't see why this is allocated both here as well as in
> >take_ownership.
>
> Where else? The only alternative is vfio_iommu_spapr_tce but I really do not
> want to touch iommu_table fields there.
Well to put it another way, why isn't take_ownership calling create
itself (or at least a common helper).
Clearly the it_userspace table needs to have lifetime which matches
the TCE table itself, so there should be a single function that marks
the beginning of that joint lifetime.
> >Isn't this function used for core-kernel users of the
> >iommu as well, in which case it shouldn't need the it_userspace.
>
>
> No. This is an iommu_table_group_ops callback which calls what the platform
> code calls (pnv_pci_create_table()) plus allocates this it_userspace thing.
> The callback is only called from VFIO.
Ok.
As touched on above it seems more like this should be owned by VFIO
code than the platform code.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpaLDumcgaa0.pgp
Description: PGP signature
