RE: [PATCH 5/7] x86/mmu: Allocate/free PASID
> There are two users of a PASID, mm and device driver(FD). If > either one is not done with the PASID, it cannot be reclaimed. As you > mentioned, it could take a long time for the driver to abort. If the > abort ends *after* mmdrop, we are in trouble. > If driver drops reference after abort/drain PASID is done, then we are > safe. I don't think there should be an abort ... suppose the application requested the DSA to copy some large block of important results from DDR4 to persistent memory. Driver should wait for that copy operation to complete. Note that for the operation to succeed, the kernel should still be processing and fixing page faults for the "mm" (some parts of the data that the user wanted to save to persistent memory may have been paged out). The wait by the DSA diver needs to by synchronous ... the "mm" cannot be freed until DSA says all the pending operations have completed. Even without persistent memory, there are cases where you want the operations to complete (mmap'd files, shared memory with other processes). -Tony ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Tue, 28 Apr 2020 13:59:43 -0700 "Luck, Tony" wrote: > >> So the driver needs to use flush/drain operations to make sure all > >> the in-flight work has completed before releasing/re-using the > >> PASID. > > Are you suggesting we should let driver also hold a reference of the > > PASID? > > The sequence for bare metal is: > > process is queuing requests to DSA > process exits (either deliberately, or crashes, or is killed) > kernel does exit processing > DSA driver is called as part of tear down of "mm" > issues drain/flush commands to ensure that all > queued operations on the PASID for this mm have > completed > PASID can be freed > > There's a 1:1 map from "mm" to PASID ... so reference counting seems > like overkill. Once the kernel is in the "exit" path, we know that no > more work can be queued using this PASID. > There are two users of a PASID, mm and device driver(FD). If either one is not done with the PASID, it cannot be reclaimed. As you mentioned, it could take a long time for the driver to abort. If the abort ends *after* mmdrop, we are in trouble. If driver drops reference after abort/drain PASID is done, then we are safe. > -Tony ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 5/7] x86/mmu: Allocate/free PASID
>> So the driver needs to use flush/drain operations to make sure all >> the in-flight work has completed before releasing/re-using the PASID. >> > Are you suggesting we should let driver also hold a reference of the > PASID? The sequence for bare metal is: process is queuing requests to DSA process exits (either deliberately, or crashes, or is killed) kernel does exit processing DSA driver is called as part of tear down of "mm" issues drain/flush commands to ensure that all queued operations on the PASID for this mm have completed PASID can be freed There's a 1:1 map from "mm" to PASID ... so reference counting seems like overkill. Once the kernel is in the "exit" path, we know that no more work can be queued using this PASID. -Tony ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Sun, Apr 26, 2020 at 04:55:25PM +0200, Thomas Gleixner wrote: > Fenghua Yu writes: > > diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h > > index bdeae9291e5c..137bf51f19e6 100644 > > --- a/arch/x86/include/asm/mmu.h > > +++ b/arch/x86/include/asm/mmu.h > > @@ -50,6 +50,10 @@ typedef struct { > > u16 pkey_allocation_map; > > s16 execute_only_pkey; > > #endif > > + > > +#ifdef CONFIG_INTEL_IOMMU_SVM > > + int pasid; > > int? It's a value which gets programmed into the MSR along with the > valid bit (bit 31) set. BTW, ARM is working on PASID as well. Christoph suggested that the PASID should be defined in mm_struct instead of mm->context so that both ARM and X86 can access it: https://lore.kernel.org/linux-iommu/20200414170252.714402-1-jean-phili...@linaro.org/T/#mb57110ffe1aaa24750eeea4f93b611f0d1913911 So I will define "pasid" to mm_struct in a separate patch in the next version. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Tue, 28 Apr 2020 12:07:25 -0700 "Luck, Tony" wrote: > > If fd release cleans up then how should there be something in > > flight at the final mmdrop? > > ENQCMD from the user is only synchronous in that it lets the user > know their request has been added to a queue (or not). Execution of > the request may happen later (if the device is busy working on > requests for other users). The request will take some time to > complete. Someone told me the theoretical worst case once, which I've > since forgotten, but it can be a long time. > > So the driver needs to use flush/drain operations to make sure all > the in-flight work has completed before releasing/re-using the PASID. > Are you suggesting we should let driver also hold a reference of the PASID? > -Tony ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Tue, 28 Apr 2020 20:54:01 +0200 Thomas Gleixner wrote: > "Jacob Pan (Jun)" writes: > > On Sun, 26 Apr 2020 16:55:25 +0200 > > Thomas Gleixner wrote: > >> Fenghua Yu writes: > >> > The PASID is freed when the process exits (so no need to keep > >> > reference counts on how many SVM devices are sharing the > >> > PASID). > >> > >> I'm not buying that. If there is an outstanding request with the > >> PASID of a process then tearing down the process address space and > >> freeing the PASID (which might be reused) is fundamentally broken. > >> > > Device driver unbind PASID is tied to FD release. So when a process > > exits, FD close causes driver to do the following: > > > > 1. stops DMA > > 2. unbind PASID (clears the PASID entry in IOMMU, flush all TLBs, > > drain in flight page requests) > > Fair enough. Explaining that somewhere might be helpful. > Will do. I plan to document this in a kernel doc for IOASID/PASID lifecycle management. > > For bare metal SVM, if the last mmdrop always happens after FD > > release, we can ensure no outstanding requests at the point of > > ioasid_free(). Perhaps this is a wrong assumption? > > If fd release cleans up then how should there be something in flight > at the final mmdrop? > > > For guest SVM, there will be more users of a PASID. I am also > > working on adding refcounting to ioasid. ioasid_free() will not > > release the PASID back to the pool until all references are > > dropped. > > What does more users mean? For VT-d, a PASID can be used by VFIO, IOMMU driver, KVM, and Virtual Device Composition Module (VDCM*) at the same time. *https://software.intel.com/en-us/download/intel-data-streaming-accelerator-preliminary-architecture-specification There are HW context associated with the PASID in IOMMU, KVM, and VDCM. So before the lifetime of the PASID is over, clean up must be done in all of the above. PASID cannot be reclaimed until the last user drops its reference. Our plan is to do notification and refcouting. > > >> > +if (mm && mm->context.pasid && !(flags & > >> > SVM_FLAG_PRIVATE_PASID)) { > >> > +/* > >> > + * Once a PASID is allocated for this mm, the > >> > PASID > >> > + * stays with the mm until the mm is dropped. > >> > Reuse > >> > + * the PASID which has been already allocated > >> > for the > >> > + * mm instead of allocating a new one. > >> > + */ > >> > +ioasid_set_data(mm->context.pasid, svm); > >> > >> So if the PASID is reused several times for different SVMs then > >> every time ioasid_data->private is set to a different SVM. How is > >> that supposed to work? > >> > > For the lifetime of the mm, there is only one PASID. > > svm_bind/unbind_mm could happen many times with different private > > data: intel_svm. Multiple devices can bind to the same PASID as > > well. But private data don't change within the first bind and last > > unbind. > > Ok. I read through that spaghetti of intel_svm_bind_mm() again and > now I start to get an idea how that is supposed to work. What a mess. > > That function really wants to be restructured in a way so it is > understandable to mere mortals. > Agreed. We are adding many new features and converging with generic sva_bind_device. Things will get more clear after we have fewer moving pieces. > Thanks, > > tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 5/7] x86/mmu: Allocate/free PASID
> If fd release cleans up then how should there be something in flight at > the final mmdrop? ENQCMD from the user is only synchronous in that it lets the user know their request has been added to a queue (or not). Execution of the request may happen later (if the device is busy working on requests for other users). The request will take some time to complete. Someone told me the theoretical worst case once, which I've since forgotten, but it can be a long time. So the driver needs to use flush/drain operations to make sure all the in-flight work has completed before releasing/re-using the PASID. -Tony ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
"Jacob Pan (Jun)" writes: > On Sun, 26 Apr 2020 16:55:25 +0200 > Thomas Gleixner wrote: >> Fenghua Yu writes: >> > The PASID is freed when the process exits (so no need to keep >> > reference counts on how many SVM devices are sharing the PASID). >> >> I'm not buying that. If there is an outstanding request with the PASID >> of a process then tearing down the process address space and freeing >> the PASID (which might be reused) is fundamentally broken. >> > Device driver unbind PASID is tied to FD release. So when a process > exits, FD close causes driver to do the following: > > 1. stops DMA > 2. unbind PASID (clears the PASID entry in IOMMU, flush all TLBs, drain > in flight page requests) Fair enough. Explaining that somewhere might be helpful. > For bare metal SVM, if the last mmdrop always happens after FD release, > we can ensure no outstanding requests at the point of ioasid_free(). > Perhaps this is a wrong assumption? If fd release cleans up then how should there be something in flight at the final mmdrop? > For guest SVM, there will be more users of a PASID. I am also > working on adding refcounting to ioasid. ioasid_free() will not release > the PASID back to the pool until all references are dropped. What does more users mean? >> > + if (mm && mm->context.pasid && !(flags & >> > SVM_FLAG_PRIVATE_PASID)) { >> > + /* >> > + * Once a PASID is allocated for this mm, the PASID >> > + * stays with the mm until the mm is dropped. Reuse >> > + * the PASID which has been already allocated for >> > the >> > + * mm instead of allocating a new one. >> > + */ >> > + ioasid_set_data(mm->context.pasid, svm); >> >> So if the PASID is reused several times for different SVMs then every >> time ioasid_data->private is set to a different SVM. How is that >> supposed to work? >> > For the lifetime of the mm, there is only one PASID. svm_bind/unbind_mm > could happen many times with different private data: intel_svm. > Multiple devices can bind to the same PASID as well. But private data > don't change within the first bind and last unbind. Ok. I read through that spaghetti of intel_svm_bind_mm() again and now I start to get an idea how that is supposed to work. What a mess. That function really wants to be restructured in a way so it is understandable to mere mortals. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Sun, 26 Apr 2020 16:55:25 +0200 Thomas Gleixner wrote: > Fenghua Yu writes: > > > PASID is shared by all threads in a process. So the logical place > > to keep track of it is in the "mm". Add the field to the > > architecture specific mm_context_t structure. > > > > A PASID is allocated for an "mm" the first time any thread attaches > > to an SVM capable device. Later device atatches (whether to the > > same > > atatches? > > > device or another SVM device) will re-use the same PASID. > > > > The PASID is freed when the process exits (so no need to keep > > reference counts on how many SVM devices are sharing the PASID). > > I'm not buying that. If there is an outstanding request with the PASID > of a process then tearing down the process address space and freeing > the PASID (which might be reused) is fundamentally broken. > Device driver unbind PASID is tied to FD release. So when a process exits, FD close causes driver to do the following: 1. stops DMA 2. unbind PASID (clears the PASID entry in IOMMU, flush all TLBs, drain in flight page requests) For bare metal SVM, if the last mmdrop always happens after FD release, we can ensure no outstanding requests at the point of ioasid_free(). Perhaps this is a wrong assumption? For guest SVM, there will be more users of a PASID. I am also working on adding refcounting to ioasid. ioasid_free() will not release the PASID back to the pool until all references are dropped. > > +void __free_pasid(struct mm_struct *mm); > > + > > #endif /* _ASM_X86_IOMMU_H */ > > diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h > > index bdeae9291e5c..137bf51f19e6 100644 > > --- a/arch/x86/include/asm/mmu.h > > +++ b/arch/x86/include/asm/mmu.h > > @@ -50,6 +50,10 @@ typedef struct { > > u16 pkey_allocation_map; > > s16 execute_only_pkey; > > #endif > > + > > +#ifdef CONFIG_INTEL_IOMMU_SVM > > + int pasid; > > int? It's a value which gets programmed into the MSR along with the > valid bit (bit 31) set. > > > extern void switch_mm(struct mm_struct *prev, struct mm_struct > > *next, diff --git a/drivers/iommu/intel-svm.c > > b/drivers/iommu/intel-svm.c index d7f2a5358900..da718a49e91e 100644 > > --- a/drivers/iommu/intel-svm.c > > +++ b/drivers/iommu/intel-svm.c > > @@ -226,6 +226,45 @@ static LIST_HEAD(global_svm_list); > > list_for_each_entry((sdev), &(svm)->devs, list) \ > > if ((d) != (sdev)->dev) {} else > > > > +/* > > + * If this mm already has a PASID we can use it. Otherwise > > allocate a new one. > > + * Let the caller know if we did an allocation via 'new_pasid'. > > + */ > > +static int alloc_pasid(struct intel_svm *svm, struct mm_struct *mm, > > + int pasid_max, bool *new_pasid, int > > flags) > > Again, data types please. flags are generally unsigned and not plain > int. Also pasid_max is certainly not plain int either. > > > +{ > > + int pasid; > > + > > + /* > > +* Reuse the PASID if the mm already has a PASID and not a > > private > > +* PASID is requested. > > +*/ > > + if (mm && mm->context.pasid && !(flags & > > SVM_FLAG_PRIVATE_PASID)) { > > + /* > > +* Once a PASID is allocated for this mm, the PASID > > +* stays with the mm until the mm is dropped. Reuse > > +* the PASID which has been already allocated for > > the > > +* mm instead of allocating a new one. > > +*/ > > + ioasid_set_data(mm->context.pasid, svm); > > So if the PASID is reused several times for different SVMs then every > time ioasid_data->private is set to a different SVM. How is that > supposed to work? > For the lifetime of the mm, there is only one PASID. svm_bind/unbind_mm could happen many times with different private data: intel_svm. Multiple devices can bind to the same PASID as well. But private data don't change within the first bind and last unbind. > > + *new_pasid = false; > > + > > + return mm->context.pasid; > > + } > > + > > + /* > > +* Allocate a new pasid. Do not use PASID 0, reserved for > > RID to > > +* PASID. > > +*/ > > + pasid = ioasid_alloc(NULL, PASID_MIN, pasid_max - 1, > > svm); > > ioasid_alloc() uses ioasid_t which is > > typedef unsigned int ioasid_t; > > Can we please have consistent types and behaviour all over the place? > > > + if (pasid == INVALID_IOASID) > > + return -ENOSPC; > > + > > + *new_pasid = true; > > + > > + return pasid; > > +} > > + > > int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, > > struct svm_dev_ops *ops) { > > struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); > > @@ -324,6 +363,8 @@ int intel_svm_bind_mm(struct device *dev, int > > *pasid, int flags, struct svm_dev_ init_rcu_head(>rcu); > > > > if (!svm) { > > + bool new_pasid; > > + > > svm = kzalloc(sizeof(*svm), GFP_KERNEL); > > if (!svm) { > >
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
Fenghua Yu writes: > On Sun, Apr 26, 2020 at 04:55:25PM +0200, Thomas Gleixner wrote: >> Fenghua Yu writes: >> > + +#ifdef CONFIG_INTEL_IOMMU_SVM + int pasid; >> >> int? It's a value which gets programmed into the MSR along with the valid >> bit (bit 31) set. > > The pasid is defined as "int" in struct intel_svm and in > intel_svm_bind_mm() and intel_svm_unbind_mm(). So the pasid defined in this > patch follows the same type defined in those places. Which are wrong to begin with. >> ioasid_alloc() uses ioasid_t which is >> >> typedef unsigned int ioasid_t; >> >> Can we please have consistent types and behaviour all over the place? > > Should I just define "pasid", "pasid_max", "flags" as "unsigned int" for > the new functions/code? > > Or should I also change their types to "unsigned int" in the original > svm code (struct intel_svm, ...bind_mm(), etc)? I'm afraid that will be > a lot of changes and should be in a separate preparation patch. Yes, please. The existance of non-sensical code is not an excuse to proliferate it. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
On Sun, Apr 26, 2020 at 04:55:25PM +0200, Thomas Gleixner wrote: > Fenghua Yu writes: > > +++ b/arch/x86/include/asm/mmu.h @@ -50,6 +50,10 @@ typedef struct { > > u16 pkey_allocation_map; s16 execute_only_pkey; > > #endif > > + +#ifdef CONFIG_INTEL_IOMMU_SVM + int pasid; > > int? It's a value which gets programmed into the MSR along with the valid > bit (bit 31) set. The pasid is defined as "int" in struct intel_svm and in intel_svm_bind_mm() and intel_svm_unbind_mm(). So the pasid defined in this patch follows the same type defined in those places. But as you pointed out below, ioasid_t is defined as "unsigned int". > > > extern void switch_mm(struct mm_struct *prev, struct mm_struct *next, > > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c > > index d7f2a5358900..da718a49e91e 100644 --- a/drivers/iommu/intel-svm.c > > +++ b/drivers/iommu/intel-svm.c @@ -226,6 +226,45 @@ static > > LIST_HEAD(global_svm_list); > > list_for_each_entry((sdev), &(svm)->devs, list) \ > > if ((d) != (sdev)->dev) {} else > > > > +/* + * If this mm already has a PASID we can use it. Otherwise > > allocate a new one. + * Let the caller know if we did an allocation via > > 'new_pasid'. + */ +static int alloc_pasid(struct intel_svm *svm, struct > > mm_struct *mm, + int pasid_max, bool *new_pasid, int flags) > > Again, data types please. flags are generally unsigned and not plain int. > Also pasid_max is certainly not plain int either. The caller defines pasid_max and flags as "int". This function just follows the caller's definitions. But I will change their definitions to "unsigned int" here. > > > + *new_pasid = false; + + return mm->context.pasid; + } + + /* + * > > Allocate a new pasid. Do not use PASID 0, reserved for RID to + * > > PASID. + */ + pasid = ioasid_alloc(NULL, PASID_MIN, pasid_max - 1, > > svm); > > ioasid_alloc() uses ioasid_t which is > > typedef unsigned int ioasid_t; > > Can we please have consistent types and behaviour all over the place? Should I just define "pasid", "pasid_max", "flags" as "unsigned int" for the new functions/code? Or should I also change their types to "unsigned int" in the original svm code (struct intel_svm, ...bind_mm(), etc)? I'm afraid that will be a lot of changes and should be in a separate preparation patch. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] x86/mmu: Allocate/free PASID
Fenghua Yu writes: > PASID is shared by all threads in a process. So the logical place to keep > track of it is in the "mm". Add the field to the architecture specific > mm_context_t structure. > > A PASID is allocated for an "mm" the first time any thread attaches > to an SVM capable device. Later device atatches (whether to the same atatches? > device or another SVM device) will re-use the same PASID. > > The PASID is freed when the process exits (so no need to keep > reference counts on how many SVM devices are sharing the PASID). I'm not buying that. If there is an outstanding request with the PASID of a process then tearing down the process address space and freeing the PASID (which might be reused) is fundamentally broken. > +void __free_pasid(struct mm_struct *mm); > + > #endif /* _ASM_X86_IOMMU_H */ > diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h > index bdeae9291e5c..137bf51f19e6 100644 > --- a/arch/x86/include/asm/mmu.h > +++ b/arch/x86/include/asm/mmu.h > @@ -50,6 +50,10 @@ typedef struct { > u16 pkey_allocation_map; > s16 execute_only_pkey; > #endif > + > +#ifdef CONFIG_INTEL_IOMMU_SVM > + int pasid; int? It's a value which gets programmed into the MSR along with the valid bit (bit 31) set. > extern void switch_mm(struct mm_struct *prev, struct mm_struct *next, > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c > index d7f2a5358900..da718a49e91e 100644 > --- a/drivers/iommu/intel-svm.c > +++ b/drivers/iommu/intel-svm.c > @@ -226,6 +226,45 @@ static LIST_HEAD(global_svm_list); > list_for_each_entry((sdev), &(svm)->devs, list) \ > if ((d) != (sdev)->dev) {} else > > +/* > + * If this mm already has a PASID we can use it. Otherwise allocate a new > one. > + * Let the caller know if we did an allocation via 'new_pasid'. > + */ > +static int alloc_pasid(struct intel_svm *svm, struct mm_struct *mm, > +int pasid_max, bool *new_pasid, int flags) Again, data types please. flags are generally unsigned and not plain int. Also pasid_max is certainly not plain int either. > +{ > + int pasid; > + > + /* > + * Reuse the PASID if the mm already has a PASID and not a private > + * PASID is requested. > + */ > + if (mm && mm->context.pasid && !(flags & SVM_FLAG_PRIVATE_PASID)) { > + /* > + * Once a PASID is allocated for this mm, the PASID > + * stays with the mm until the mm is dropped. Reuse > + * the PASID which has been already allocated for the > + * mm instead of allocating a new one. > + */ > + ioasid_set_data(mm->context.pasid, svm); So if the PASID is reused several times for different SVMs then every time ioasid_data->private is set to a different SVM. How is that supposed to work? > + *new_pasid = false; > + > + return mm->context.pasid; > + } > + > + /* > + * Allocate a new pasid. Do not use PASID 0, reserved for RID to > + * PASID. > + */ > + pasid = ioasid_alloc(NULL, PASID_MIN, pasid_max - 1, svm); ioasid_alloc() uses ioasid_t which is typedef unsigned int ioasid_t; Can we please have consistent types and behaviour all over the place? > + if (pasid == INVALID_IOASID) > + return -ENOSPC; > + > + *new_pasid = true; > + > + return pasid; > +} > + > int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct > svm_dev_ops *ops) > { > struct intel_iommu *iommu = intel_svm_device_to_iommu(dev); > @@ -324,6 +363,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int > flags, struct svm_dev_ > init_rcu_head(>rcu); > > if (!svm) { > + bool new_pasid; > + > svm = kzalloc(sizeof(*svm), GFP_KERNEL); > if (!svm) { > ret = -ENOMEM; > @@ -335,15 +376,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, > int flags, struct svm_dev_ > if (pasid_max > intel_pasid_max_id) > pasid_max = intel_pasid_max_id; > > - /* Do not use PASID 0, reserved for RID to PASID */ > - svm->pasid = ioasid_alloc(NULL, PASID_MIN, > - pasid_max - 1, svm); > - if (svm->pasid == INVALID_IOASID) { > + svm->pasid = alloc_pasid(svm, mm, pasid_max, _pasid, flags); > + if (svm->pasid < 0) { > kfree(svm); > kfree(sdev); > - ret = -ENOSPC; ret gets magically initialized to an error return value, right? > goto out; > } > + > svm->notifier.ops = _mmuops; > svm->mm = mm; > svm->flags = flags; > @@ -353,7 +392,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int > flags, struct svm_dev_ > if (mm) { > ret =