IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "Raj, Ashok" <ashok.raj@intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Jonathan Cameron <jic23@kernel.org>
Subject: Re: [PATCH 10/10] iommu/vt-d: Register PASID notifier for status change
Date: Fri, 27 Mar 2020 16:47:01 -0700
Message-ID: <20200327164701.5cfee1c3@jacob-builder> (raw)
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D19D7ED65A@SHSMSX104.ccr.corp.intel.com>

On Fri, 27 Mar 2020 10:22:57 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jacob Pan
> > Sent: Thursday, March 26, 2020 1:56 AM
> > 
> > In bare metal SVA, IOMMU driver ensures that IOASID free call
> > always comes after IOASID unbind operation.
> > 
> > However, for guest SVA the unbind and free call come from user space
> > via VFIO, which could be out of order. This patch registers a
> > notifier block in case IOASID free() comes before unbind such that
> > VT-d driver can take action to clean up PASID context and data.  
> 
> clearly the patch includes more than above usage. It also handles the
> bind usage to notify KVM for setting PASID translation table.
> 
> > 
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  drivers/iommu/intel-svm.c   | 68
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/intel-iommu.h |  1 +
> >  2 files changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index f511855d187b..779dd2c6f9e1 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -23,6 +23,7 @@
> >  #include "intel-pasid.h"
> > 
> >  static irqreturn_t prq_event_thread(int irq, void *d);
> > +static DEFINE_MUTEX(pasid_mutex);
> > 
> >  #define PRQ_ORDER 0
> > 
> > @@ -92,6 +93,65 @@ static inline bool intel_svm_capable(struct
> > intel_iommu *iommu)
> >  	return iommu->flags & VTD_FLAG_SVM_CAPABLE;
> >  }
> > 
> > +#define pasid_lock_held() lock_is_held(&pasid_mutex.dep_map)
> > +
> > +static int pasid_status_change(struct notifier_block *nb,
> > +				unsigned long code, void *data)
> > +{
> > +	struct ioasid_nb_args *args = (struct ioasid_nb_args
> > *)data;
> > +	struct intel_svm_dev *sdev;
> > +	struct intel_svm *svm;
> > +	int ret = NOTIFY_DONE;
> > +
> > +	if (code == IOASID_FREE) {
> > +		/*
> > +		 * Unbind all devices associated with this PASID
> > which is
> > +		 * being freed by other users such as VFIO.
> > +		 */
> > +		mutex_lock(&pasid_mutex);
> > +		svm = ioasid_find(INVALID_IOASID_SET, args->id,
> > NULL);
> > +		if (!svm || !svm->iommu)
> > +			goto done_unlock;  
> 
> should we treat !svm->iommu as an error condition? if not, do you have
> an example when it may occur in normal situation?
> 
Right, should be an error. Initially, unbind could retrieve iommu from
struct device, but with notifier we have to set svm->iommu all the time.

> > +
> > +		if (IS_ERR(svm)) {
> > +			ret = NOTIFY_BAD;
> > +			goto done_unlock;
> > +		}  
> 
> svm->iommu should be referenced after IS_ERR check
> 
Good point, will move up.
> > +
> > +		list_for_each_entry_rcu(sdev, &svm->devs, list,
> > pasid_lock_held()) {
> > +			/* Does not poison forward pointer */
> > +			list_del_rcu(&sdev->list);
> > +			intel_pasid_tear_down_entry(svm->iommu,
> > sdev-  
> > >dev,  
> > +						    svm->pasid);
> > +			kfree_rcu(sdev, rcu);
> > +
> > +			/*
> > +			 * Free before unbind only happens with
> > guest usaged
> > +			 * host PASIDs. IOASID free will detach
> > private data
> > +			 * and free the IOASID entry.  
> 
> "guest usaged host PASIDs"?
> 
I mean VM used PASID, will change to
/*
 * Free before unbind only happens with PASIDs used
 * by VM. IOASID free will detach private data
 * and free the IOASID entry.
 */

> > +			 */
> > +			if (list_empty(&svm->devs))
> > +				kfree(svm);
> > +		}
> > +		mutex_unlock(&pasid_mutex);
> > +
> > +		return NOTIFY_OK;
> > +	}
> > +
> > +done_unlock:
> > +	mutex_unlock(&pasid_mutex);
> > +	return ret;
> > +}
> > +
> > +static struct notifier_block pasid_nb = {
> > +		.notifier_call = pasid_status_change,
> > +};
> > +
> > +void intel_svm_add_pasid_notifier(void)
> > +{
> > +	ioasid_add_notifier(&pasid_nb);
> > +}
> > +
> >  void intel_svm_check(struct intel_iommu *iommu)
> >  {
> >  	if (!pasid_supported(iommu))
> > @@ -219,7 +279,6 @@ static const struct mmu_notifier_ops
> > intel_mmuops = {
> >  	.invalidate_range = intel_invalidate_range,
> >  };
> > 
> > -static DEFINE_MUTEX(pasid_mutex);
> >  static LIST_HEAD(global_svm_list);
> > 
> >  #define for_each_svm_dev(sdev, svm, d)			\
> > @@ -319,6 +378,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> > *domain,
> >  			svm->gpasid = data->gpasid;
> >  			svm->flags |= SVM_FLAG_GUEST_PASID;
> >  		}
> > +		svm->iommu = iommu;  
> 
> ah, it's interesting to see we have a field defined before but never
> used. 😊
> 
The unbind call can retrieve iommu from struct device.

> > 
> >  		ioasid_attach_data(data->hpasid, svm);
> >  		INIT_LIST_HEAD_RCU(&svm->devs);
> > @@ -383,6 +443,11 @@ int intel_svm_bind_gpasid(struct iommu_domain
> > *domain,
> >  	}
> >  	svm->flags |= SVM_FLAG_GUEST_MODE;
> > 
> > +	/*
> > +	 * Notify KVM new host-guest PASID bind is ready. KVM will
> > set up
> > +	 * PASID translation table to support guest ENQCMD.
> > +	 */  
> 
> should take it as an example instead of the only possible usage.
> 
True, we don;t know who is getting notified.

> > +	ioasid_notify(data->hpasid, IOASID_BIND);
> >  	init_rcu_head(&sdev->rcu);
> >  	list_add_rcu(&sdev->list, &svm->devs);
> >   out:
> > @@ -440,6 +505,7 @@ int intel_svm_unbind_gpasid(struct device *dev,
> > int pasid)
> >  				 * used by another.
> >  				 */
> >  				ioasid_attach_data(pasid, NULL);
> > +				ioasid_notify(pasid,
> > IOASID_UNBIND); kfree(svm);
> >  			}
> >  		}
> > diff --git a/include/linux/intel-iommu.h
> > b/include/linux/intel-iommu.h index f8504a980981..64799067ea58
> > 100644 --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -708,6 +708,7 @@ extern struct iommu_sva *
> >  intel_svm_bind(struct device *dev, struct mm_struct *mm, void
> > *drvdata); extern void intel_svm_unbind(struct iommu_sva *handle);
> >  extern int intel_svm_get_pasid(struct iommu_sva *handle);
> > +extern void intel_svm_add_pasid_notifier(void);
> > 
> >  struct svm_dev_ops;
> > 
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu  

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 17:55 [PATCH 00/10] IOASID extensions for guest SVA Jacob Pan
2020-03-25 17:55 ` [PATCH 01/10] iommu/ioasid: Introduce system-wide capacity Jacob Pan
2020-03-27  8:07   ` Tian, Kevin
2020-03-27 16:08     ` Jacob Pan
2020-04-01 13:45   ` Jean-Philippe Brucker
2020-04-01 22:50     ` Jacob Pan
2020-03-25 17:55 ` [PATCH 02/10] iommu/vt-d: Set IOASID capacity when SVM is enabled Jacob Pan
2020-03-27  8:08   ` Tian, Kevin
2020-03-25 17:55 ` [PATCH 03/10] iommu/ioasid: Introduce per set allocation APIs Jacob Pan
2020-03-26  2:12   ` Lu Baolu
2020-03-26 21:30     ` Jacob Pan
2020-03-27  8:38   ` Tian, Kevin
2020-03-27 16:59     ` Jacob Pan
2020-03-28  6:32       ` Tian, Kevin
2020-04-01 13:47   ` Jean-Philippe Brucker
2020-04-06 20:02     ` Jacob Pan
2020-04-07 11:01       ` Jean-Philippe Brucker
2020-04-21 21:51         ` Jacob Pan
2020-03-25 17:55 ` [PATCH 04/10] iommu/ioasid: Rename ioasid_set_data to avoid confusion with ioasid_set Jacob Pan
2020-03-27  9:35   ` Tian, Kevin
2020-03-25 17:55 ` [PATCH 05/10] iommu/ioasid: Create an IOASID set for host SVA use Jacob Pan
2020-03-27  9:41   ` Tian, Kevin
2020-03-27 17:28     ` Jacob Pan
2020-03-28  6:33       ` Tian, Kevin
2020-04-01 13:53   ` Jean-Philippe Brucker
2020-04-06 15:33     ` Jacob Pan
2020-04-07 11:01       ` Jean-Philippe Brucker
2020-04-13 22:06         ` Jacob Pan
2020-04-15 15:10           ` Jean-Philippe Brucker
2020-03-25 17:55 ` [PATCH 06/10] iommu/ioasid: Convert to set aware allocations Jacob Pan
2020-03-27  9:54   ` Tian, Kevin
2020-03-27 17:41     ` Jacob Pan
2020-03-28  6:40       ` Tian, Kevin
2020-04-06 20:07         ` Jacob Pan
2020-04-01 13:55   ` Jean-Philippe Brucker
2020-04-01 22:45     ` Jacob Pan
2020-03-25 17:55 ` [PATCH 07/10] iommu/ioasid: Use mutex instead of spinlock Jacob Pan
2020-03-27  9:55   ` Tian, Kevin
2020-04-01 13:58   ` Jean-Philippe Brucker
2020-03-25 17:55 ` [PATCH 08/10] iommu/ioasid: Introduce notifier APIs Jacob Pan
2020-03-27 10:03   ` Tian, Kevin
2020-03-27 18:36     ` Jacob Pan
2020-03-28  6:43       ` Tian, Kevin
2020-03-31 15:13         ` Jacob Pan
2020-04-01 14:00   ` Jean-Philippe Brucker
2020-04-10 15:43     ` Jacob Pan
2020-03-25 17:55 ` [PATCH 09/10] iommu/ioasid: Support ioasid_set quota adjustment Jacob Pan
2020-03-27 10:09   ` Tian, Kevin
2020-03-27 23:30     ` Jacob Pan
2020-03-28  6:44       ` Tian, Kevin
2020-03-25 17:55 ` [PATCH 10/10] iommu/vt-d: Register PASID notifier for status change Jacob Pan
2020-03-27 10:22   ` Tian, Kevin
2020-03-27 23:47     ` Jacob Pan [this message]
2020-04-01 14:03 ` [PATCH 00/10] IOASID extensions for guest SVA Jean-Philippe Brucker
2020-04-01 23:38   ` Jacob Pan
2020-04-02 12:26     ` Jean-Philippe Brucker
2020-04-02 16:09       ` Jacob Pan

Reply instructions:

You may reply publicly 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=20200327164701.5cfee1c3@jacob-builder \
    --to=jacob.jun.pan@linux.intel.com \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=dwmw2@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.com \
    --cc=jic23@kernel.org \
    --cc=kevin.tian@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    /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

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.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-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


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