Re: [RFC PATCH 2/6] drivers core: Add I/O ASID allocator

[Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]

Hi Jean-Philippe,

On Fri, Oct 19, 2018 at 07:11:54PM +0100, Jean-Philippe Brucker wrote:
> The allocator doesn't really belong in drivers/iommu because some
> drivers would like to allocate PASIDs for devices that aren't managed by
> an IOMMU, using the same ID space as IOMMU. It doesn't really belong in
> drivers/pci either since platform device also support PASID. Add the
> allocator in drivers/base.

I don't really buy this argument, in the end it is the IOMMU that
enforces the PASID mappings for a device. Why should a device not behind
an IOMMU need to allocate a pasid? Do you have examples of such devices
which also don't have their own iommu built-in?

> +int ioasid_for_each(struct ioasid_set *set, ioasid_iter_t func, void *data)
> +{
> +	int ret;
> +	struct ioasid_iter_data iter_data = {
> +		.set	= set,
> +		.func	= func,
> +		.data	= data,
> +	};
> +
> +	idr_lock(&ioasid_idr);
> +	ret = idr_for_each(&ioasid_idr, ioasid_iter, &iter_data);
> +	idr_unlock(&ioasid_idr);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_for_each);

What is the intended use-case for this function?

> +void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid)
> +{
> +	void *priv = NULL;
> +	struct ioasid_data *ioasid_data;
> +
> +	idr_lock(&ioasid_idr);
> +	ioasid_data = idr_find(&ioasid_idr, ioasid);
> +	if (ioasid_data && ioasid_data->set == set)
> +		priv = ioasid_data->private;
> +	idr_unlock(&ioasid_idr);
> +
> +	return priv;
> +}

I think using the idr_lock here will become a performance problem, as we
need this function in the SVA page-fault path to retrieve the mm_struct
belonging to a PASID.

The head comment in lib/idr.c states that it should be safe to use
rcu_readlock() on read-only iterations. If so, this should be used here
so that concurrent lookups are possible.

Regards,

	Joerg