| Message ID | 20230302005959.2695267-4-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Re-enable IDXD kernel workqueue under DMA API | expand |
Hi Jacob,
I love your patch! Yet something to improve:
[auto build test ERROR on vkoul-dmaengine/next]
[also build test ERROR on v6.2]
[cannot apply to joro-iommu/next linus/master next-20230302]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacob-Pan/iommu-vt-d-Implement-set-device-pasid-op-for-default-domain/20230302-085748
base: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link: https://lore.kernel.org/r/20230302005959.2695267-4-jacob.jun.pan%40linux.intel.com
patch subject: [PATCH 3/4] iommu/sva: Support reservation of global PASIDs
config: arm64-randconfig-r013-20230302 (https://download.01.org/0day-ci/archive/20230302/202303021016.avd8l1rJ-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/b27170369658e99a0aafd84985de0ce48c1b2539
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacob-Pan/iommu-vt-d-Implement-set-device-pasid-op-for-default-domain/20230302-085748
git checkout b27170369658e99a0aafd84985de0ce48c1b2539
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/gpu/drm/exynos/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303021016.avd8l1rJ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/exynos/exynos_drm_dma.c:8:
>> include/linux/iommu.h:1215:1: error: expected identifier or '('
{
^
drivers/gpu/drm/exynos/exynos_drm_dma.c:54:35: warning: implicit conversion from 'unsigned long long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
dma_set_max_seg_size(subdrv_dev, DMA_BIT_MASK(32));
~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:40: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^~~~~
1 warning and 1 error generated.
vim +1215 include/linux/iommu.h
1213
1214 static inline ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max);
> 1215 {
1216 return IOMMU_PASID_INVALID;
1217 }
1218
Hi Jacob,
I love your patch! Yet something to improve:
[auto build test ERROR on vkoul-dmaengine/next]
[also build test ERROR on v6.2]
[cannot apply to joro-iommu/next linus/master next-20230302]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacob-Pan/iommu-vt-d-Implement-set-device-pasid-op-for-default-domain/20230302-085748
base: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link: https://lore.kernel.org/r/20230302005959.2695267-4-jacob.jun.pan%40linux.intel.com
patch subject: [PATCH 3/4] iommu/sva: Support reservation of global PASIDs
config: hexagon-buildonly-randconfig-r005-20230302 (https://download.01.org/0day-ci/archive/20230302/202303021143.qA8IMjmN-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/b27170369658e99a0aafd84985de0ce48c1b2539
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacob-Pan/iommu-vt-d-Implement-set-device-pasid-op-for-default-domain/20230302-085748
git checkout b27170369658e99a0aafd84985de0ce48c1b2539
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/crypto/hisilicon/sec/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303021143.qA8IMjmN-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/crypto/hisilicon/sec/sec_drv.c:11:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/crypto/hisilicon/sec/sec_drv.c:11:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/crypto/hisilicon/sec/sec_drv.c:11:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
In file included from drivers/crypto/hisilicon/sec/sec_drv.c:14:
>> include/linux/iommu.h:1215:1: error: expected identifier or '('
{
^
drivers/crypto/hisilicon/sec/sec_drv.c:1209:39: warning: shift count >= width of type [-Wshift-count-overflow]
ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
^~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^ ~~~
7 warnings and 1 error generated.
vim +1215 include/linux/iommu.h
1213
1214 static inline ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max);
> 1215 {
1216 return IOMMU_PASID_INVALID;
1217 }
1218
> From: Jacob Pan <jacob.jun.pan@linux.intel.com> > Sent: Thursday, March 2, 2023 9:00 AM > > Global PASID allocation is under IOMMU SVA code since it is the primary > use case. However, some architecture such as VT-d, global PASIDs are > necessary for its internal use of DMA API with PASID. No, global PASID is not a VT-d restriction. It's from ENQCMD/S hence a device requirement. > > This patch introduces SVA APIs to reserve and release global PASIDs. > > Link: https://lore.kernel.org/all/20230301235646.2692846-4- > jacob.jun.pan@linux.intel.com/ > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > drivers/iommu/iommu-sva.c | 25 +++++++++++++++++++++++++ > include/linux/iommu.h | 14 ++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c > index 8c92a145e15d..cfdeafde88a9 100644 > --- a/drivers/iommu/iommu-sva.c > +++ b/drivers/iommu/iommu-sva.c > @@ -149,6 +149,31 @@ u32 iommu_sva_get_pasid(struct iommu_sva > *handle) > } > EXPORT_SYMBOL_GPL(iommu_sva_get_pasid); > > +ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max) > +{ > + int ret; > + > + if (min == IOMMU_PASID_INVALID || max == > IOMMU_PASID_INVALID || > + min == 0 || max < min) > + return IOMMU_PASID_INVALID; > + > + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, > GFP_KERNEL); > + if (ret < 0) > + return IOMMU_PASID_INVALID; > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_sva_reserve_pasid); > + I'm not sure it's the right way. It's not related to SVA. We should move iommu_global_pasid_ida to iomm.c and then have another interface for allocation. Above is pretty generic so probably a general one like: ioasid_t iommu_allocate_global_pasid(struct device *dev) internally it can use [1, dev->iommu->max_pasids] as min/max instead of passed in from the caller.
Hi Kevin, On Thu, 2 Mar 2023 09:43:03 +0000, "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Sent: Thursday, March 2, 2023 9:00 AM > > > > Global PASID allocation is under IOMMU SVA code since it is the primary > > use case. However, some architecture such as VT-d, global PASIDs are > > necessary for its internal use of DMA API with PASID. > > No, global PASID is not a VT-d restriction. It's from ENQCMD/S hence a > device requirement. I meant VT-d based platforms, it is kind of intertwined in that ENQCMDS does not restrict RIDPASID!=DMA PASID, vt-d does. Without this restriction, there wouldn't be a need for this patch. Let me reword. > > > > This patch introduces SVA APIs to reserve and release global PASIDs. > > > > Link: https://lore.kernel.org/all/20230301235646.2692846-4- > > jacob.jun.pan@linux.intel.com/ > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > --- > > drivers/iommu/iommu-sva.c | 25 +++++++++++++++++++++++++ > > include/linux/iommu.h | 14 ++++++++++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c > > index 8c92a145e15d..cfdeafde88a9 100644 > > --- a/drivers/iommu/iommu-sva.c > > +++ b/drivers/iommu/iommu-sva.c > > @@ -149,6 +149,31 @@ u32 iommu_sva_get_pasid(struct iommu_sva > > *handle) > > } > > EXPORT_SYMBOL_GPL(iommu_sva_get_pasid); > > > > +ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max) > > +{ > > + int ret; > > + > > + if (min == IOMMU_PASID_INVALID || max == > > IOMMU_PASID_INVALID || > > + min == 0 || max < min) > > + return IOMMU_PASID_INVALID; > > + > > + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, > > GFP_KERNEL); > > + if (ret < 0) > > + return IOMMU_PASID_INVALID; > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(iommu_sva_reserve_pasid); > > + > > I'm not sure it's the right way. It's not related to SVA. > > We should move iommu_global_pasid_ida to iomm.c and then have > another interface for allocation. > > Above is pretty generic so probably a general one like: > > ioasid_t iommu_allocate_global_pasid(struct device *dev) > > internally it can use [1, dev->iommu->max_pasids] as min/max instead > of passed in from the caller. sounds good to me, will do. Thanks, Jacob
On Fri, Mar 03, 2023 at 01:47:53PM -0800, Jacob Pan wrote: > Hi Kevin, > > On Thu, 2 Mar 2023 09:43:03 +0000, "Tian, Kevin" <kevin.tian@intel.com> > wrote: > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > > Sent: Thursday, March 2, 2023 9:00 AM > > > > > > Global PASID allocation is under IOMMU SVA code since it is the primary > > > use case. However, some architecture such as VT-d, global PASIDs are > > > necessary for its internal use of DMA API with PASID. > > > > No, global PASID is not a VT-d restriction. It's from ENQCMD/S hence a > > device requirement. > I meant VT-d based platforms, it is kind of intertwined in that ENQCMDS > does not restrict RIDPASID!=DMA PASID, vt-d does. Without this > restriction, there wouldn't be a need for this patch. Let me reword. No, Kevin is right, there is nothing about VT-d that needs global PASID values. The driver should be managing RID2PASID itself to avoid conflicting with any in-use PASID, either by changing RID2PASID on demand or by setting it to a value that is not part of the PASID number space, eg we can make 0 entirely invalid, or the driver can reduce max_pasid of the devices it controls and use PASID_MAX. Jason
On Mon, Mar 06, 2023 at 09:44:08AM -0800, Jacob Pan wrote: > Hi Jason, > > On Mon, 6 Mar 2023 09:01:32 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Fri, Mar 03, 2023 at 01:47:53PM -0800, Jacob Pan wrote: > > > Hi Kevin, > > > > > > On Thu, 2 Mar 2023 09:43:03 +0000, "Tian, Kevin" <kevin.tian@intel.com> > > > wrote: > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > > > > Sent: Thursday, March 2, 2023 9:00 AM > > > > > > > > > > Global PASID allocation is under IOMMU SVA code since it is the > > > > > primary use case. However, some architecture such as VT-d, global > > > > > PASIDs are necessary for its internal use of DMA API with PASID. > > > > > > > > No, global PASID is not a VT-d restriction. It's from ENQCMD/S hence a > > > > device requirement. > > > I meant VT-d based platforms, it is kind of intertwined in that ENQCMDS > > > does not restrict RIDPASID!=DMA PASID, vt-d does. Without this > > > restriction, there wouldn't be a need for this patch. Let me reword. > > > > No, Kevin is right, there is nothing about VT-d that needs global > > PASID values. > > > > The driver should be managing RID2PASID itself to avoid conflicting > > with any in-use PASID, either by changing RID2PASID on demand or by > > setting it to a value that is not part of the PASID number space, eg > > we can make 0 entirely invalid, or the driver can reduce max_pasid of > > the devices it controls and use PASID_MAX. > > > I see, thank you both. how about > "This patch provide an API for device drivers to request global PASIDs as > needed. The device drivers will then gain the flexibility of choosing > PASIDs not conflicting with anyone in-use." Stil no, this functionality should be clearly and unambiguously tied to ENQCMD: Devices that rely on Intel ENQCMD have a single CPU register to store the current thread's PASID in. This necessarily makes the PASID a system-global value shared by all ENQCMD using devices. This matches the current allocator being used for the SVA PASID so for now allow ENQCMD drivers to access this PASID allocator for other uses. Jason
Hi Jason, On Mon, 6 Mar 2023 09:01:32 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote: > On Fri, Mar 03, 2023 at 01:47:53PM -0800, Jacob Pan wrote: > > Hi Kevin, > > > > On Thu, 2 Mar 2023 09:43:03 +0000, "Tian, Kevin" <kevin.tian@intel.com> > > wrote: > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > > > Sent: Thursday, March 2, 2023 9:00 AM > > > > > > > > Global PASID allocation is under IOMMU SVA code since it is the > > > > primary use case. However, some architecture such as VT-d, global > > > > PASIDs are necessary for its internal use of DMA API with PASID. > > > > > > No, global PASID is not a VT-d restriction. It's from ENQCMD/S hence a > > > device requirement. > > I meant VT-d based platforms, it is kind of intertwined in that ENQCMDS > > does not restrict RIDPASID!=DMA PASID, vt-d does. Without this > > restriction, there wouldn't be a need for this patch. Let me reword. > > No, Kevin is right, there is nothing about VT-d that needs global > PASID values. > > The driver should be managing RID2PASID itself to avoid conflicting > with any in-use PASID, either by changing RID2PASID on demand or by > setting it to a value that is not part of the PASID number space, eg > we can make 0 entirely invalid, or the driver can reduce max_pasid of > the devices it controls and use PASID_MAX. > I see, thank you both. how about "This patch provide an API for device drivers to request global PASIDs as needed. The device drivers will then gain the flexibility of choosing PASIDs not conflicting with anyone in-use." Thanks, Jacob
Hi Jason, On Mon, 6 Mar 2023 13:43:39 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote: > On Mon, Mar 06, 2023 at 09:44:08AM -0800, Jacob Pan wrote: > > Hi Jason, > > > > On Mon, 6 Mar 2023 09:01:32 -0400, Jason Gunthorpe <jgg@nvidia.com> > > wrote: > > > On Fri, Mar 03, 2023 at 01:47:53PM -0800, Jacob Pan wrote: > > > > Hi Kevin, > > > > > > > > On Thu, 2 Mar 2023 09:43:03 +0000, "Tian, Kevin" > > > > <kevin.tian@intel.com> wrote: > > > > > > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > > > > > Sent: Thursday, March 2, 2023 9:00 AM > > > > > > > > > > > > Global PASID allocation is under IOMMU SVA code since it is the > > > > > > primary use case. However, some architecture such as VT-d, > > > > > > global PASIDs are necessary for its internal use of DMA API > > > > > > with PASID. > > > > > > > > > > No, global PASID is not a VT-d restriction. It's from ENQCMD/S > > > > > hence a device requirement. > > > > I meant VT-d based platforms, it is kind of intertwined in that > > > > ENQCMDS does not restrict RIDPASID!=DMA PASID, vt-d does. Without > > > > this restriction, there wouldn't be a need for this patch. Let me > > > > reword. > > > > > > No, Kevin is right, there is nothing about VT-d that needs global > > > PASID values. > > > > > > The driver should be managing RID2PASID itself to avoid conflicting > > > with any in-use PASID, either by changing RID2PASID on demand or by > > > setting it to a value that is not part of the PASID number space, eg > > > we can make 0 entirely invalid, or the driver can reduce max_pasid of > > > the devices it controls and use PASID_MAX. > > > > > I see, thank you both. how about > > "This patch provide an API for device drivers to request global PASIDs > > as needed. The device drivers will then gain the flexibility of choosing > > PASIDs not conflicting with anyone in-use." > > Stil no, this functionality should be clearly and unambiguously tied > to ENQCMD: > > Devices that rely on Intel ENQCMD have a single CPU register to store > the current thread's PASID in. This necessarily makes the PASID a > system-global value shared by all ENQCMD using devices. > > This matches the current allocator being used for the SVA PASID so for > now allow ENQCMD drivers to access this PASID allocator for other > uses. > ENQCMDS does not have the restriction of using a single CPU MSR to store PASIDs, PASID is supplied to the instruction operand. Here we are adding API for ENQCMDS. Should we explain this as well? i.e. due the unforgiving nature of ENQCMD that requires global PASIDs, ENQCMDS has no choice but to allocate from the same numberspace to avoid conflict. Thanks, Jacob
On Mon, Mar 06, 2023 at 09:57:59AM -0800, Jacob Pan wrote: > ENQCMDS does not have the restriction of using a single CPU MSR to store > PASIDs, PASID is supplied to the instruction operand. Huh? That isn't what it says in the programming manual. It says the PASID only comes from the IA32_PASID msr and the only two operands are the destination MMIO and the memory source for the rest of the payload. Jason
>> ENQCMDS does not have the restriction of using a single CPU MSR to store >> PASIDs, PASID is supplied to the instruction operand. > > Huh? That isn't what it says in the programming manual. It says the > PASID only comes from the IA32_PASID msr and the only two operands are > the destination MMIO and the memory source for the rest of the payload. Jason, Two different instructions with only one letter different in the name. ENQCMD - ring 3 instruction. The PASID is inserted into the descriptor pushed to the device from the IA32_PASID MSR. ENQCMDS - ring 0 instruction (see that trailing "S" for Supervisor mode). In this case the submitter can include any PASID value they want in the in-memory copy of the descriptor and ENQCMDS will pass that to the device. -Tony
On Mon, Mar 06, 2023 at 06:48:43PM +0000, Luck, Tony wrote: > >> ENQCMDS does not have the restriction of using a single CPU MSR to store > >> PASIDs, PASID is supplied to the instruction operand. > > > > Huh? That isn't what it says in the programming manual. It says the > > PASID only comes from the IA32_PASID msr and the only two operands are > > the destination MMIO and the memory source for the rest of the payload. > > Jason, > > Two different instructions with only one letter different in the name. > > ENQCMD - ring 3 instruction. The PASID is inserted into the descriptor > pushed to the device from the IA32_PASID MSR. > > ENQCMDS - ring 0 instruction (see that trailing "S" for Supervisor mode). > In this case the submitter can include any PASID value they want in the > in-memory copy of the descriptor and ENQCMDS will pass that to the > device. Ah, well, my comment wasn't talking about ENQCMDS :) If ENQCMDS can take in an arbitary PASID then there is no justification here to use the global allocator. The rational is more like: IDXD uses PASIDs that come from the SVA allocator. It needs to create an internal kernel-only PASID that is non-overlapping so allow the SVA allocator to reserve PASIDs for driver use. IDXD has to use the global SVA PASID allocator beacuse its userspace will use ENQCMD which requires global PASIDs. Jason
Hi Jason, On Mon, 6 Mar 2023 15:05:27 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote: > On Mon, Mar 06, 2023 at 06:48:43PM +0000, Luck, Tony wrote: > > >> ENQCMDS does not have the restriction of using a single CPU MSR to > > >> store PASIDs, PASID is supplied to the instruction operand. > > > > > > Huh? That isn't what it says in the programming manual. It says the > > > PASID only comes from the IA32_PASID msr and the only two operands are > > > the destination MMIO and the memory source for the rest of the > > > payload. > > > > Jason, > > > > Two different instructions with only one letter different in the name. > > > > ENQCMD - ring 3 instruction. The PASID is inserted into the descriptor > > pushed to the device from the IA32_PASID MSR. > > > > ENQCMDS - ring 0 instruction (see that trailing "S" for Supervisor > > mode). In this case the submitter can include any PASID value they want > > in the in-memory copy of the descriptor and ENQCMDS will pass that to > > the device. > > Ah, well, my comment wasn't talking about ENQCMDS :) > > If ENQCMDS can take in an arbitary PASID then there is no > justification here to use the global allocator. > > The rational is more like: > > IDXD uses PASIDs that come from the SVA allocator. It needs to create > an internal kernel-only PASID that is non-overlapping so allow the SVA > allocator to reserve PASIDs for driver use. > > IDXD has to use the global SVA PASID allocator beacuse its userspace > will use ENQCMD which requires global PASIDs. > yes, great summary. I think that is the same as what I was trying to say earlier :) "due the unforgiving nature of ENQCMD that requires global PASIDs, ENQCMDS has no choice but to allocate from the same numberspace to avoid conflict." In that sense, I feel the global allocator should be staying with SVA instead of moving to iommu core (as Kevin suggested). Because we are trying to have non-overlapping pasid with SVA. Thanks, Jacob
> From: Jacob Pan <jacob.jun.pan@linux.intel.com> > Sent: Friday, March 10, 2023 1:06 AM > > Hi Jason, > > On Mon, 6 Mar 2023 15:05:27 -0400, Jason Gunthorpe <jgg@nvidia.com> > wrote: > > > On Mon, Mar 06, 2023 at 06:48:43PM +0000, Luck, Tony wrote: > > > >> ENQCMDS does not have the restriction of using a single CPU MSR to > > > >> store PASIDs, PASID is supplied to the instruction operand. > > > > > > > > Huh? That isn't what it says in the programming manual. It says the > > > > PASID only comes from the IA32_PASID msr and the only two operands > are > > > > the destination MMIO and the memory source for the rest of the > > > > payload. > > > > > > Jason, > > > > > > Two different instructions with only one letter different in the name. > > > > > > ENQCMD - ring 3 instruction. The PASID is inserted into the descriptor > > > pushed to the device from the IA32_PASID MSR. > > > > > > ENQCMDS - ring 0 instruction (see that trailing "S" for Supervisor > > > mode). In this case the submitter can include any PASID value they want > > > in the in-memory copy of the descriptor and ENQCMDS will pass that to > > > the device. > > > > Ah, well, my comment wasn't talking about ENQCMDS :) > > > > If ENQCMDS can take in an arbitary PASID then there is no > > justification here to use the global allocator. > > > > The rational is more like: > > > > IDXD uses PASIDs that come from the SVA allocator. It needs to create > > an internal kernel-only PASID that is non-overlapping so allow the SVA > > allocator to reserve PASIDs for driver use. > > > > IDXD has to use the global SVA PASID allocator beacuse its userspace > > will use ENQCMD which requires global PASIDs. > > > yes, great summary. I think that is the same as what I was trying to say > earlier :) > "due the unforgiving nature of ENQCMD that requires global PASIDs, > ENQCMDS > has no choice but to allocate from the same numberspace to avoid conflict." > > In that sense, I feel the global allocator should be staying with SVA > instead of moving to iommu core (as Kevin suggested). Because we are trying > to have non-overlapping pasid with SVA. > I still doubt 'reserve' is the right interface to define. for DMA domain probably yes as it's static and one-off. but thinking louder when the same driver starts to support SIOV we need allocating additional PASIDs on demand which is hardly to be fit in a reservation interface.
On Thu, Mar 16, 2023 at 07:25:17AM +0000, Tian, Kevin wrote: > I still doubt 'reserve' is the right interface to define. > > for DMA domain probably yes as it's static and one-off. > > but thinking louder when the same driver starts to support SIOV we > need allocating additional PASIDs on demand which is hardly to be > fit in a reservation interface. Sure, it is the same thing. It is "reserve" in the sense they are not assigned to mm structs and not used for SVA. Jason
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c index 8c92a145e15d..cfdeafde88a9 100644 --- a/drivers/iommu/iommu-sva.c +++ b/drivers/iommu/iommu-sva.c @@ -149,6 +149,31 @@ u32 iommu_sva_get_pasid(struct iommu_sva *handle) } EXPORT_SYMBOL_GPL(iommu_sva_get_pasid); +ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max) +{ + int ret; + + if (min == IOMMU_PASID_INVALID || max == IOMMU_PASID_INVALID || + min == 0 || max < min) + return IOMMU_PASID_INVALID; + + ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL); + if (ret < 0) + return IOMMU_PASID_INVALID; + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_sva_reserve_pasid); + +void iommu_sva_unreserve_pasid(ioasid_t pasid) +{ + if (!pasid_valid(pasid)) + return; + + ida_free(&iommu_global_pasid_ida, pasid); +} +EXPORT_SYMBOL_GPL(iommu_sva_unreserve_pasid); + /* * I/O page fault handler for SVA */ diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 39a97bd8f04a..8ba07eb03d32 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -1192,6 +1192,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm); void iommu_sva_unbind_device(struct iommu_sva *handle); u32 iommu_sva_get_pasid(struct iommu_sva *handle); +ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max); +void iommu_sva_unreserve_pasid(ioasid_t pasid); + #else static inline struct iommu_sva * iommu_sva_bind_device(struct device *dev, struct mm_struct *mm) @@ -1207,6 +1210,17 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle) { return IOMMU_PASID_INVALID; } + +static inline ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max); +{ + return IOMMU_PASID_INVALID; +} + +static inline void iommu_sva_unreserve_pasid(ioasid_t pasid) +{ + +} + #endif /* CONFIG_IOMMU_SVA */ #endif /* __LINUX_IOMMU_H */
Global PASID allocation is under IOMMU SVA code since it is the primary use case. However, some architecture such as VT-d, global PASIDs are necessary for its internal use of DMA API with PASID. This patch introduces SVA APIs to reserve and release global PASIDs. Link: https://lore.kernel.org/all/20230301235646.2692846-4-jacob.jun.pan@linux.intel.com/ Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- drivers/iommu/iommu-sva.c | 25 +++++++++++++++++++++++++ include/linux/iommu.h | 14 ++++++++++++++ 2 files changed, 39 insertions(+)