All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: vkoul@kernel.org, "Tian, Kevin" <kevin.tian@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Raj Ashok <ashok.raj@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	iommu@lists.linux-foundation.org,
	Jason Gunthorpe <jgg@nvidia.com>,
	zhangfei.gao@linaro.org,
	Jean-Philippe Brucker <jean-philippe@linaro.com>
Subject: Re: [PATCH 2/2] iommu/sva: Remove mm parameter from SVA bind API
Date: Tue, 13 Apr 2021 17:09:47 -0700	[thread overview]
Message-ID: <20210413170947.35ba9267@jacob-builder> (raw)
In-Reply-To: <20210409110305.6b0471d9@jacob-builder>

Hi Jean,

On Fri, 9 Apr 2021 11:03:05 -0700, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:

> > problems:
> > 
> > * We don't have a use-case for binding the mm of a remote process (and
> >   it's supposedly difficult for device drivers to do it securely). So
> > OK, we remove the mm argument from iommu_sva_bind_device() and use the
> >   current mm. But the IOMMU driver isn't going to do
> > get_task_mm(current) every time it needs the mm being bound, it will
> > take it from iommu_sva_bind_device(). Likewise iommu_sva_alloc_pasid()
> > shouldn't need to bother with get_task_mm().
> > 
> > * cgroup accounting for IOASIDs needs to be on the current task.
> > Removing the mm parameter from iommu_sva_alloc_pasid() doesn't help
> > with that. Sure it indicates that iommu_sva_alloc_pasid() needs a
> > specific task context but that's only for cgroup purpose, and I'd
> > rather pass the cgroup down from iommu_sva_bind_device() anyway (but am
> > fine with keeping it within ioasid_alloc() for now). Plus it's an
> > internal helper, easy for us to check that the callers are doing the
> > right thing. 
> With the above split, we really just have one allocation function:
> ioasid_alloc(), so it can manage current cgroup accounting within. Would
> this work?
After a few attempts, I don't think the split can work better. I will
restore the mm parameter and add a warning if mm != current->mm.

Thanks,

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

WARNING: multiple messages have this Message-ID (diff)
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	iommu@lists.linux-foundation.org, Joerg Roedel <joro@8bytes.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	Yi Liu <yi.l.liu@intel.com>, Raj Ashok <ashok.raj@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Dave Jiang <dave.jiang@intel.com>,
	wangzhou1@hisilicon.com, zhangfei.gao@linaro.org,
	vkoul@kernel.org, jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH 2/2] iommu/sva: Remove mm parameter from SVA bind API
Date: Tue, 13 Apr 2021 17:09:47 -0700	[thread overview]
Message-ID: <20210413170947.35ba9267@jacob-builder> (raw)
In-Reply-To: <20210409110305.6b0471d9@jacob-builder>

Hi Jean,

On Fri, 9 Apr 2021 11:03:05 -0700, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:

> > problems:
> > 
> > * We don't have a use-case for binding the mm of a remote process (and
> >   it's supposedly difficult for device drivers to do it securely). So
> > OK, we remove the mm argument from iommu_sva_bind_device() and use the
> >   current mm. But the IOMMU driver isn't going to do
> > get_task_mm(current) every time it needs the mm being bound, it will
> > take it from iommu_sva_bind_device(). Likewise iommu_sva_alloc_pasid()
> > shouldn't need to bother with get_task_mm().
> > 
> > * cgroup accounting for IOASIDs needs to be on the current task.
> > Removing the mm parameter from iommu_sva_alloc_pasid() doesn't help
> > with that. Sure it indicates that iommu_sva_alloc_pasid() needs a
> > specific task context but that's only for cgroup purpose, and I'd
> > rather pass the cgroup down from iommu_sva_bind_device() anyway (but am
> > fine with keeping it within ioasid_alloc() for now). Plus it's an
> > internal helper, easy for us to check that the callers are doing the
> > right thing. 
> With the above split, we really just have one allocation function:
> ioasid_alloc(), so it can manage current cgroup accounting within. Would
> this work?
After a few attempts, I don't think the split can work better. I will
restore the mm parameter and add a warning if mm != current->mm.

Thanks,

Jacob

  reply	other threads:[~2021-04-14  0:07 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 17:08 [PATCH 1/2] iommu/sva: Tighten SVA bind API with explicit flags Jacob Pan
2021-04-08 17:08 ` Jacob Pan
2021-04-08 17:08 ` [PATCH 2/2] iommu/sva: Remove mm parameter from SVA bind API Jacob Pan
2021-04-08 17:08   ` Jacob Pan
2021-04-09 10:11   ` Jean-Philippe Brucker
2021-04-09 10:11     ` Jean-Philippe Brucker
2021-04-09 18:03     ` Jacob Pan
2021-04-09 18:03       ` Jacob Pan
2021-04-14  0:09       ` Jacob Pan [this message]
2021-04-14  0:09         ` Jacob Pan
2021-04-14  6:22         ` Lu Baolu
2021-04-14  6:22           ` Lu Baolu
2021-04-14 11:26           ` Jason Gunthorpe
2021-04-14 11:26             ` Jason Gunthorpe
2021-04-15  5:33             ` Lu Baolu
2021-04-15  5:33               ` Lu Baolu
2021-04-15 11:59               ` Jason Gunthorpe
2021-04-15 11:59                 ` Jason Gunthorpe
2021-04-09 12:45   ` Lu Baolu
2021-04-09 12:45     ` Lu Baolu
2021-04-09 18:08     ` Jacob Pan
2021-04-09 18:08       ` Jacob Pan
2021-04-09 10:22 ` [PATCH 1/2] iommu/sva: Tighten SVA bind API with explicit flags Jean-Philippe Brucker
2021-04-09 10:22   ` Jean-Philippe Brucker
2021-04-09 21:57   ` Jacob Pan
2021-04-09 21:57     ` Jacob Pan
2021-04-09 12:24 ` Lu Baolu
2021-04-09 12:24   ` Lu Baolu
2021-04-13 22:13   ` Jacob Pan
2021-04-13 22:13     ` Jacob Pan
2021-04-13 13:02 ` kernel test robot
2021-04-13 13:02   ` kernel test robot
2021-04-13 13:02   ` kernel test robot
2021-04-14  4:11 ` kernel test robot
2021-04-14  4:11   ` kernel test robot
2021-04-14  4:11   ` kernel test robot

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=20210413170947.35ba9267@jacob-builder \
    --to=jacob.jun.pan@linux.intel.com \
    --cc=ashok.raj@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.com \
    --cc=jean-philippe@linaro.org \
    --cc=jgg@nvidia.com \
    --cc=kevin.tian@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=vkoul@kernel.org \
    --cc=zhangfei.gao@linaro.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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.