Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
	jacob.jun.pan@linux.intel.com, will.deacon@arm.com,
	linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
	robh+dt@kernel.org,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	robin.murphy@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/8] iommu: Add I/O ASID allocator
Date: Tue, 11 Jun 2019 11:13:33 -0700
Message-ID: <20190611111333.425ce809@jacob-builder> (raw)
In-Reply-To: <62d1f310-0cba-4d55-0f16-68bba3c64927@arm.com>

On Tue, 11 Jun 2019 15:35:22 +0100
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> On 11/06/2019 10:36, Jonathan Cameron wrote:
> >> +/**
> >> + * ioasid_alloc - Allocate an IOASID
> >> + * @set: the IOASID set
> >> + * @min: the minimum ID (inclusive)
> >> + * @max: the maximum ID (inclusive)
> >> + * @private: data private to the caller
> >> + *
> >> + * Allocate an ID between @min and @max. The @private pointer is
> >> stored
> >> + * internally and can be retrieved with ioasid_find().
> >> + *
> >> + * Return: the allocated ID on success, or %INVALID_IOASID on
> >> failure.
> >> + */
> >> +ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
> >> ioasid_t max,
> >> +		      void *private)
> >> +{
> >> +	u32 id = INVALID_IOASID;
> >> +	struct ioasid_data *data;
> >> +
> >> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> >> +	if (!data)
> >> +		return INVALID_IOASID;
> >> +
> >> +	data->set = set;
> >> +	data->private = private;
> >> +
> >> +	if (xa_alloc(&ioasid_xa, &id, data, XA_LIMIT(min, max),
> >> GFP_KERNEL)) {
> >> +		pr_err("Failed to alloc ioasid from %d to %d\n",
> >> min, max);
> >> +		goto exit_free;
> >> +	}
> >> +	data->id = id;
> >> +
> >> +exit_free:  
> > 
> > This error flow is perhaps a little more confusing than it needs to
> > be?
> > 
> > My assumption (perhaps wrong) is that we only have an id ==
> > INVALID_IOASID if the xa_alloc fails, and that we will always have
> > such an id value if it does (I'm not totally sure this second
> > element is true in __xa_alloc).
> > 
> > If I'm missing something perhaps a comment on how else we'd get
> > here.  
> 
> Yes we can simplify this:
> 
> 		return id;
> 	exit_free:
> 		kfree(data)
> 		return INVALID_IOASID;
> 	}
> 
> The XA API doesn't say that @id passed to xa_alloc() won't be modified
> in case of error. It's true in the current implementation, but won't
> necessarily stay that way. On the other hand I think it's safe to
> expect @id to always be set when xa_alloc() succeeds.
> 
the flow with custom allocator is slightly different, but you are right
I can simplify it as you suggested.
Jonathan, I will add you to the cc list in next version. If you could
also review the current version, it would be greatly appreciated.

https://lore.kernel.org/lkml/1560087862-57608-13-git-send-email-jacob.jun.pan@linux.intel.com/

> >> +/**
> >> + * ioasid_find - Find IOASID data
> >> + * @set: the IOASID set
> >> + * @ioasid: the IOASID to find
> >> + * @getter: function to call on the found object
> >> + *
> >> + * The optional getter function allows to take a reference to the
> >> found object
> >> + * under the rcu lock. The function can also check if the object
> >> is still valid:
> >> + * if @getter returns false, then the object is invalid and NULL
> >> is returned.
> >> + *
> >> + * If the IOASID has been allocated for this set, return the
> >> private pointer
> >> + * passed to ioasid_alloc. Private data can be NULL if not set.
> >> Return an error
> >> + * if the IOASID is not found or does not belong to the set.  
> > 
> > Perhaps should make it clear that @set can be null.  
> 
> Indeed. But I'm not sure allowing @set to be NULL is such a good idea,
> because the data type associated to an ioasid depends on its set. For
> example SVA will put an mm_struct in there, and auxiliary domains use
> some structure private to the IOMMU domain.
> 
I am not sure we need to count on @set to decipher data type. Whoever
does the allocation and owns the IOASID should knows its own data type.
My thought was that @set is only used to group IDs, permission check
etc.

> Jacob, could me make @set mandatory, or do you see a use for a global
> search? If @set is NULL, then callers can check if the return pointer
> is NULL, but will run into trouble if they try to dereference it.
> 
A global search use case can be for PRQ. IOMMU driver gets a IOASID
(first interrupt then retrieve from a queue), it has no idea which
@set it belongs to. But the data types are the same for all IOASIDs
used by the IOMMU.
If @set is NULL, the search does not check set match. It is separate
from return pointer. Sorry i am not seeing the problems here.

> >   
> >> + */
> >> +void *ioasid_find(struct ioasid_set *set, ioasid_t ioasid,
> >> +		  bool (*getter)(void *))
> >> +{
> >> +	void *priv = NULL;  
> > 
> > Set in all paths, so does need to be set here.  
> 
> Right
> 
> Thanks,
> Jean

[Jacob Pan]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-10 18:47 [PATCH 0/8] iommu: Add auxiliary domain and PASID support to Arm SMMUv3 Jean-Philippe Brucker
2019-06-10 18:47 ` [PATCH 1/8] iommu: Add I/O ASID allocator Jean-Philippe Brucker
2019-06-11  9:36   ` Jonathan Cameron
2019-06-11 14:35     ` Jean-Philippe Brucker
2019-06-11 18:13       ` Jacob Pan [this message]
2019-06-18 14:22         ` Jean-Philippe Brucker
2019-06-18 17:05           ` Jacob Pan
2019-06-19 14:26             ` Jean-Philippe Brucker
2019-06-11 12:26   ` Jacob Pan
2019-06-11 14:37     ` Jean-Philippe Brucker
2019-06-11 17:10       ` Jacob Pan
2019-06-12 11:30         ` Jean-Philippe Brucker
2019-06-10 18:47 ` [PATCH 2/8] dt-bindings: document PASID property for IOMMU masters Jean-Philippe Brucker
2019-07-08  7:58   ` Auger Eric
2019-06-10 18:47 ` [PATCH 3/8] iommu/arm-smmu-v3: Support platform SSID Jean-Philippe Brucker
2019-06-11  9:42   ` Jonathan Cameron
2019-06-11 14:35     ` Jean-Philippe Brucker
2019-06-18 18:08   ` Will Deacon
2019-06-19 11:53     ` Jean-Philippe Brucker
2019-07-08  7:58   ` Auger Eric
2019-06-10 18:47 ` [PATCH 4/8] iommu/arm-smmu-v3: Add support for Substream IDs Jean-Philippe Brucker
2019-06-11 10:19   ` Jonathan Cameron
2019-06-11 14:35     ` Jean-Philippe Brucker
2019-06-26 18:00   ` Will Deacon
2019-07-04  9:33     ` Jean-Philippe Brucker
2019-07-08 15:31   ` Auger Eric
2019-06-10 18:47 ` [PATCH 5/8] iommu/arm-smmu-v3: Add second level of context descriptor table Jean-Philippe Brucker
2019-06-11 10:24   ` Jonathan Cameron
2019-07-08 15:13   ` Auger Eric
2019-06-10 18:47 ` [PATCH 6/8] iommu/arm-smmu-v3: Support auxiliary domains Jean-Philippe Brucker
2019-06-26 17:59   ` Will Deacon
2019-07-05 16:29     ` Jean-Philippe Brucker
2019-06-10 18:47 ` [PATCH 7/8] iommu/arm-smmu-v3: Improve add_device() error handling Jean-Philippe Brucker
2019-07-08  7:58   ` Auger Eric
2019-06-10 18:47 ` [PATCH 8/8] iommu/arm-smmu-v3: Add support for PCI PASID Jean-Philippe Brucker
2019-06-11 10:45   ` Jonathan Cameron
2019-06-11 14:35     ` Jean-Philippe Brucker
2019-07-08  7:58   ` Auger Eric

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190611111333.425ce809@jacob-builder \
    --to=jacob.jun.pan@linux.intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe.brucker@arm.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox