Device specific pass through in host systems - discuss user interface

[Prakhya, Sai Praneeth]

Hi All,

I am working on an IOMMU driver feature that allows a user to specify if the 
DMA from a device should be translated by IOMMU or not. Presently, we support 
only all devices or none mode i.e. if user specifies "iommu=pt" [X86] or 
"iommu.passthrough" [ARM64] through kernel command line, all the devices would 
be in pass through mode and we don't have per device granularity, but, we were 
requested by a customer to selectively put devices in pass through mode and not 
all.

Since, this feature could be generic across architectures, we thought it would 
be better if the user interface is discussed in the community first. We are 
envisioning this to be used both during boot time and runtime and hence having 
a kernel command line argument along with a sysfs entry are needed. So, please 
pour in your suggestions on how the user interface should look like to make it 
architecture agnostic.


1.  Have a kernel command line argument that takes a list of BDF's as an 
input and puts them in pass through mode

a.  Accepting BDF as an input has a downside - BDF is dynamic and could 
change if BIOS/OS enumerates a new device in next reboot

b.  Accepting  pair as an input has a downside - What 
to do when there are multiple such devices and user would like to put only some 
of them in PT mode

2.  Have a sysfs file which takes 1 or 0 as an input to enable/disable pass 
through mode. Some places that seem to be reasonable are

a.  /sys/class/iommu/dmar0/devices/

b.  /sys/kernel/iommu_groups//devices

I am looking for a consensus on *how the kernel command line argument should 
look like and path for sysfs entry*. Also, please note that if a device is put 
in pass through mode it won't be available for the guest and that's ok.

Regards,
Sai

PS: Idea credits: Ashok Raj
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 0/3] Add debugfs support to show scalable mode DMAR table

[Prakhya, Sai Praneeth]

> > Changes from V2 to V3:
> > --
> > Presently, for V2 patches if kernel command line argument "iommu=pt"
> > is passed, dumping DMAR table seg faults. This happens because in pass
> > through mode (for non-scalable DMAR's) 3rd bit of context entry is set
> > and it is misinterpreted as PASID enabled by debugfs code and hence
> > tries to dereference PASID directory pointer which leads to seg fault
> > (PASID directory pointer is undefined for non-scalable DMAR's). To fix
> > this, dereference PASID directory pointer only when 1. PASID is
> > supported and 2. PASID is enabled.
> >
> > This patch is tested on
> > 1. Non-scalable DMAR with and without iommu=pt 2. Scalable DMAR with
> > and without iommu=pt
> 
> Sorry, missed this patch-set. Applied this one instead of V2.

Thanks! This is the right patch set. I have also checked IOMMU tree and it 
looks good :)

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: Device specific pass through in host systems - discuss user interface

[Prakhya, Sai Praneeth]

Hi All,

+ Sohil and Rob Clark (as there are dropped from CC'list)

> > > Most iommu vendor drivers have switched from per-device to per-group
> > > domain (a.k.a. default domain). So per-group pass-through mode makes
> more sense?
> > >
> > > By the way, can we extend this to "per-group default domain type",
> > > instead of only "per-group pass-through mode"? Currently we have
> > > system level default domain type, if we have finer granularity of
> > > default domain type, both iommu drivers and end users will benefit from 
> > > it.
> >
> > Sure! Makes sense.. per-group default domain type sounds good.

I am planning to implement an RFC (supporting only runtime case for now) which 
works as below

1. User unbinds the driver by writing to sysfs
2. User puts a group in pass through mode by writing "1" to
/sys/kernel/iommu_groups//pt
3. User re-binds the driver by writing to sysfs

As suggested by Lu, Baolu will look into implementing this by using "per-group 
default domain type"

If anyone has suggestions/comments/concerns, please reply.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: Device specific pass through in host systems - discuss user interface

[Prakhya, Sai Praneeth]

> > > > Sure! Makes sense.. per-group default domain type sounds good.
> >
> > I am planning to implement an RFC (supporting only runtime case for
> > now) which works as below
> >
> > 1. User unbinds the driver by writing to sysfs 2. User puts a group in
> > pass through mode by writing "1" to
> > /sys/kernel/iommu_groups//pt
> 
> might be better to read current value of default domain for that group..
> /sys/kernel/iommu_groups//default_domain

Presently, we already have a file that gives out "type" of default_domain and 
the file is
/sys/kernel/iommu_groups//type

> reading the above value shows current setting.
> provide a differnet file next_def_domain, and you can echo "pt" or
> "dma_domain"
> to switch to pass-through, or normal dma isolation mode.

We have couple of options here:

1. Since we already have "type" file, which is "read-only", we could make it 
R/W.

The present value shows the existing type of default domain.
If user wants to change it (Eg: from DMA to IDENTITY or vice versa), he 
attempts to write the new value.
Kernel performs checks to make sure that the driver in unbinded and it's safe 
to change the default domain type.
After successfully changing the default_domain type internally, kernel reflects 
the new value in the file.
Ay errors in the process will be reported in dmesg.

2. As you have suggested, we could have a *new* file named 
"next_def_domain_type", which takes string as an input.

Please let me know if there is any preference among these approaches and why :)

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: Device specific pass through in host systems - discuss user interface

[Prakhya, Sai Praneeth]

> > I am working on an IOMMU driver feature that allows a user to specify
> > if the DMA from a device should be translated by IOMMU or not.
> > Presently, we support only all devices or none mode i.e. if user
> > specifies "iommu=pt" [X86] or "iommu.passthrough" [ARM64] through
> > kernel command line, all the devices would be in pass through mode and
> > we don't have per device granularity, but, we were requested by a
> > customer to selectively put devices in pass through mode and not all.
> 
> Most iommu vendor drivers have switched from per-device to per-group domain
> (a.k.a. default domain). So per-group pass-through mode makes more sense?
> 
> By the way, can we extend this to "per-group default domain type", instead of
> only "per-group pass-through mode"? Currently we have system level default
> domain type, if we have finer granularity of default domain type, both iommu
> drivers and end users will benefit from it.

Sure! Makes sense.. per-group default domain type sounds good.

> > I am looking for a consensus on **how the kernel command line argument
> > should look like and path for sysfs entry**. Also, please note that if
> > a device is put in pass through mode it won't be available for the
> > guest and that's ok.
> 
> Just out of curiosity, what's the limitation for a device using pass- through 
> DMA
> domain to be assignable.

Sorry! I don't know about assignable devices. Probably, Ashok or Jacob could 
answer this question

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 3/3] iommu/vt-d: Add debugfs support to show scalable mode DMAR table internals

[Prakhya, Sai Praneeth]

> >   static void ctx_tbl_walk(struct seq_file *m, struct intel_iommu *iommu, 
> > u16
> bus)
> >   {
> > struct context_entry *context;
> > -   u16 devfn;
> > +   u16 devfn, pasid_dir_size;
> > +   u64 pasid_dir_ptr;
> >
> > for (devfn = 0; devfn < 256; devfn++) {
> > struct tbl_walk tbl_wlk = {0};
> >
> > +   /*
> > +* Scalable mode root entry points to upper context table and
> > +* lower context table. Each scalable mode context table has
> > +* 128 context entries where as legacy mode context table has
> > +* 256 context entries. So for scalable mode, devfn > 127 is
> > +* invalid. But, iommu_context_addr() inherently handles this by
> 
> This comment is a bit misleading. :-)
> 
> devfn > 127 is also valid for scalable mode. The context entries for former 
> 128
> devices are in the lower scalable-mode context-table, while the latter 128
> devices in upper scalable-mode context-table.
> This has been handled in iommu_context_addr(), so the caller don't need to
> worry about this.

Yes.. that makes sense. Will correct it in V2.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/3] Add debugfs support to show scalable mode DMAR table

[Prakhya, Sai Praneeth]

> Hi Sai,
> 
> On 5/10/19 2:41 AM, Sai Praneeth Prakhya wrote:
> > From: Sai Praneeth 
> >
> > Presently, "/sys/kernel/debug/iommu/intel/dmar_translation_struct"
> > file dumps only legacy DMAR table which consists of root table and
> > context table. Scalable mode DMAR table adds PASID directory and PASID
> > table. Hence, add support to dump these tables as well.
> >
> > Directly extending the present dumping format for PASID tables will
> > make the output look clumsy. Hence, the first patch modifies the
> > present format to a tabular format. The second patch introduces macros
> > that are used during PASID table walk and the third patch actually
> > adds support to dump scalable mode DMAR table.
> >
> > Sai Praneeth (3):
> >iommu/vt-d: Modify the format of intel DMAR tables dump
> >iommu/vt-d: Introduce macros useful for dumping DMAR table
> >iommu/vt-d: Add debugfs support to show scalable mode DMAR table
> >  internals
> 
> This patch set looks good to me in general. One minor suggestion is that the
> author name and signed-of-by name should be consistent for all patches.

Thanks for the suggestion. Sorry! that I missed it.
Will quickly send a V2 fixing it.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: Device specific pass through in host systems - discuss user interface

[Prakhya, Sai Praneeth]

> > The present value shows the existing type of default domain.
> > If user wants to change it (Eg: from DMA to IDENTITY or vice versa), he
> attempts to write the new value.
> > Kernel performs checks to make sure that the driver in unbinded and it's 
> > safe
> to change the default domain type.
> > After successfully changing the default_domain type internally, kernel 
> > reflects
> the new value in the file.
> > Ay errors in the process will be reported in dmesg.
> 
> I prefer this way. Writing to the file should fail with -EBUSY when it is not 
> safe to
> change the default domain-type. Writing should only succeed when no device in
> the group is assigned to a device driver.

Thanks a lot! Joerg for the reply.
I have just sent out an RFC of this patch set to the IOMMU mailing list.
Could you please take a look at it and let me know your feedback?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

Hi Joerg,

Thanks a lot! for the review. I highly appreciate for sparing your time to 
review the patch :)

> On Tue, Aug 20, 2019 at 07:42:25PM -0700, Sai Praneeth Prakhya wrote:
> > +   /*
> > +* iommu_domain_alloc() takes "struct bus_type" as an argument which
> is
> > +* a member in "struct device". Changing a group's default domain type
> > +* deals at iommu_group level rather than device level and hence there
> > +* is no straight forward way to get "bus_type" of an iommu_group that
> > +* could be passed to iommu_domain_alloc(). So, instead of directly
> > +* calling iommu_domain_alloc(), use iommu_ops from previous default
> > +* domain.
> > +*/
> > +   if (!prev_domain || !prev_domain->ops ||
> > +   !prev_domain->ops->domain_alloc || !type)
> > +   return -EINVAL;
> 
> Hmm, this isn't really nice and clean, but I understand why you need it.

I agree.. It didn't look good for me either :(
But, I didn't find any other better solution. 

> I will think about a better way to get iommu_ops here.

Sure! That will be great!

> > +free_prev_domain:
> > +   /*
> > +* Free the existing default domain and replace it with the newly
> > +* created default domain. No need to set group->domain because
> > +* __iommu_attach_group() already does it on success.
> > +*/
> > +   iommu_domain_free(prev_domain);
> > +   group->default_domain = new_domain;
> > +   return 0;
> 
> It isn't obvious to me from this patch, how to are the dma_ops updated when
> the default domain changes between identity and dma?

Thanks for raising this.
For intel_iommu, dma_map_ops is defined at drivers/iommu/intel-iommu.c and
all the callbacks like alloc(), map_sg() and map_page(), check if the device 
needs DMA mapping or not 
by calling iommu_need_mapping(). The callbacks inherently do the right thing 
based on the outcome.
So, essentially the dma_ops are same for dma/identity domain.

I just realized (sorry!) that other iommu drivers (Eg: AMD) doesn't do it the 
same way i.e. looks like the callbacks 
aren't checking if the device needs a dma mapping or identity mapping.
I will take a look at other iommu drivers and will handle this in V2.

Please let me know if I missed something.

> > +   /* Check if any device in the group still has a driver binded to it */
> > +   if (iommu_group_for_each_dev(group, NULL, is_driver_binded)) {
> > +   pr_err("Active drivers exist for devices in the group\n");
> > +   return -EBUSY;
> > +   }
> 
> This is racy with device driver probing code. Unfortunatly there is no clean 
> way
> out of that either, locking all devices in the group and then do the 
> re-attach will
> introduce a lock-inversion with group->mutex. But please put a comment here
> saying that this might race with device driver probing.

Sure! Makes sense. Will add it in V2.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> >>> +free_prev_domain:
> >>> + /*
> >>> +  * Free the existing default domain and replace it with the newly
> >>> +  * created default domain. No need to set group->domain because
> >>> +  * __iommu_attach_group() already does it on success.
> >>> +  */
> >>> + iommu_domain_free(prev_domain);
> >>> + group->default_domain = new_domain;
> >>> + return 0;
> >>
> >> It isn't obvious to me from this patch, how to are the dma_ops
> >> updated when the default domain changes between identity and dma?
> >
> > Thanks for raising this.
> > For intel_iommu, dma_map_ops is defined at drivers/iommu/intel-iommu.c
> > and all the callbacks like alloc(), map_sg() and map_page(), check if
> > the device needs DMA mapping or not by calling iommu_need_mapping(). The
> callbacks inherently do the right thing based on the outcome.
> > So, essentially the dma_ops are same for dma/identity domain.
> 
> This isn't always true as we are considering per-device dma ops.

Ahh.. I see. I wasn't aware that per-device dma ops might change this.
Thanks for letting me know. I will take this into consideration as well for V2.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH RFC 1/4] iommu/vt-d: Modify device_def_domain_type() to use at runtime

[Prakhya, Sai Praneeth]

> On Tue, Jul 02, 2019 at 06:53:59PM -0700, Sai Praneeth Prakhya wrote:
> > device_def_domain_type() determines the domain type a device could
> > have and it's called only during boot. But, to change the domain of a
> > group through sysfs, kernel has to call this function during runtime.
> > Hence, add an argument to the function which lets the function know if
> > it's being called at boot time or runtime.
> 
> I don't think it should make a difference when the function is actually 
> called. The
> sysfs input is just another variable to take into account when the default 
> domain
> type is determined.

Sure! Makes sense. I will modify the code accordingly.

> What I'd like to see for example is that I can write 'auto' to the file and 
> get back
> the systems decision for the default domain type.

Sure! Sounds good to me. Will add this functionality.

> I'd also like to be able to forbid changing the type for e.g.  Thunderbolt 
> connected devices.

This is presently supported in the patch set. But, I got your point, will make 
sure that 
untrusted devices are not allowed to change the group.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH RFC 0/4] iommu: Add support to change default domain of a group

[Prakhya, Sai Praneeth]

> On Tue, Jul 02, 2019 at 06:53:58PM -0700, Sai Praneeth Prakhya wrote:
> > Assume a use case where-in the priviliged user would want to use the
> > device in pass-through mode when the device is used for host but would
> > want to switch to dma protected mode when switching for VFIO in user
> > space. Presently, this is not supported and hence add support to change
> default domain of a group dynamically.
> 
> VFIO does it's own iommu magic with the device and moves the group out of the
> default domain, so that doesn't sound like a valid use-case.

Thanks a lot! for the reply Joerg.
I wasn't aware about this as I have very limited exposure to VFIO.
I will take a look into this.

> More valid would be something like putting a device into passthrough 
>  mode to improve performance, or do you have other valid use-cases in mind?

Presently, we don't have anything else except that putting a device in pass 
through 
mode will improve its performance.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 1/1] iommu/vt-d: Correctly check format of page table in debugfs

[Prakhya, Sai Praneeth]

Hi Allen,

> diff --git a/drivers/iommu/intel-iommu-debugfs.c b/drivers/iommu/intel-
> iommu-debugfs.c
> index 73a552914455..e31c3b416351 100644
> --- a/drivers/iommu/intel-iommu-debugfs.c
> +++ b/drivers/iommu/intel-iommu-debugfs.c
> @@ -235,7 +235,7 @@ static void ctx_tbl_walk(struct seq_file *m, struct
> intel_iommu *iommu, u16 bus)
>   tbl_wlk.ctx_entry = context;
>   m->private = _wlk;
> 
> - if (pasid_supported(iommu) && is_pasid_enabled(context)) {
> + if (dmar_readq(iommu->reg + DMAR_RTADDR_REG) &
> DMA_RTADDR_SMT) {

Thanks for adding this, I do believe this is a good addition but I also think 
that we might 
need "is_pasid_enabled()" as well. It checks if PASIDE bit in context entry is 
enabled or not.

I am thinking that even though DMAR might be using scalable root and context 
table, the entry 
itself should have PASIDE bit set. Did I miss something here?

And I also think a macro would be better so that it could reused elsewhere (if 
need be).

Regards,
Sai

RE: [PATCH 1/1] iommu/vt-d: Correctly check format of page table in debugfs

[Prakhya, Sai Praneeth]

> On 7/22/19 1:21 PM, Prakhya, Sai Praneeth wrote:
> > Hi Allen,
> >
> >> diff --git a/drivers/iommu/intel-iommu-debugfs.c
> >> b/drivers/iommu/intel- iommu-debugfs.c index
> >> 73a552914455..e31c3b416351 100644
> >> --- a/drivers/iommu/intel-iommu-debugfs.c
> >> +++ b/drivers/iommu/intel-iommu-debugfs.c
> >> @@ -235,7 +235,7 @@ static void ctx_tbl_walk(struct seq_file *m,
> >> struct intel_iommu *iommu, u16 bus)
> >>tbl_wlk.ctx_entry = context;
> >>m->private = _wlk;
> >>
> >> -  if (pasid_supported(iommu) && is_pasid_enabled(context)) {
> >> +  if (dmar_readq(iommu->reg + DMAR_RTADDR_REG) &
> >> DMA_RTADDR_SMT) {
> >
> > Thanks for adding this, I do believe this is a good addition but I
> > also think that we might need "is_pasid_enabled()" as well. It checks if 
> > PASIDE
> bit in context entry is enabled or not.
> >
> > I am thinking that even though DMAR might be using scalable root and
> > context table, the entry itself should have PASIDE bit set. Did I miss 
> > something
> here?
> 
> No matter the PASIDE bit set or not, IOMMU always uses the scalable mode
> page table if scalable mode is enabled. If PASIDE is set, requests with PASID 
> will
> be handled. Otherwise, requests with PASID will be blocked (but request 
> without
> PASID will always be handled).
> 
> We are dumpling the page table of the IOMMU, so we only care about what
> page table format it is using. Do I understand it right>

Thanks! Baolu, for the explanation. Yes, it makes sense and I agree that we 
don't need the extra check for PASIDE bit.

I have tested this change on scalable/legacy DMAR's with/without iommu=pt and 
it works :)

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> On 2020/2/24 11:20, Prakhya, Sai Praneeth wrote:
> >>> + list_for_each_entry_safe(grp_dev, temp, >devices, list) {
> >>> + struct device *dev;
> >>> +
> >>> + dev = grp_dev->dev;
> >>> + iommu_release_device(dev);
> >>> +
> >>> + ret = iommu_group_add_device(group, dev);
> >>> + if (ret)
> >>> + dev_err(dev, "Failed to add to iommu group %d: %d\n",
> >>> + group->id, ret);
> >> Need to handle this error case.
> > I wasn't sure on how to handle the error ☹
> 
> Just roll back to the state before calling this function and return an 
> appropriate
> error value.
> 
> The likely behavior is detaching the new domains from all devices (if it has
> already attached), attaching the old domains to all devices in the group,

And while handling this error case, there is a possibility that attaching to 
old domain could fail.. so, we might need to handle that error case as well. If 
we plan to handle error case, since we have removed devices from group above, 
adding them back could fail too.. that would lead into handling error case for 
an error case.
So, given the probability of these functions failing here are very low, I 
think, why not bite the bullet and say, add code to handle these error cases if 
we see that these functions are failing frequently? Otherwise the error 
handling code is just a dead code.

> cleaning
> up all new resources allocated in this function, putting a error message to 
> tell
> the user why it fails and returning an error code.
> 
> > i.e. group's domain/default_domain are already updated to new domain and
> assume there are 'n' devices in the group and this failed for 'k'th device, I 
> wasn't
> sure how I could roll back the changes made for k-1 devices.
> 
> A successful attach could be checked by (group->domain ==
> group->default_domain).

No.. because I have manually set group->domain == group->default_domain = 
new_domain (did this because iommu_group_add_device() and 
iommu_group_create_direct_mappings() needs them)
So, probably we might need some other way to check successful attach.

> > So, I thought probably just alert the user that there was an error while
> changing default domain type and try updating for other devices in the group
> (hopefully other devices might succeed). Also,*generally*  we shouldn't see 
> any
> errors here because all these devices were already in the same group earlier 
> (we
> aren’t adding/removing new devices to the group). We are just changing default
> domain type and we already made sure that device could be in the requested
> default domain type.
> >
> >>> +
> >>> + ret = prv_dom->ops->add_device(dev);
> >>> + if (ret)
> >>> + dev_err(dev, "Error adding to iommu: %d\n", ret);
> >> Ditto.
> >>
> >>> + }
> >>> +
> >>> + iommu_group_put(group);
> >>> + iommu_domain_free(prv_dom);
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int is_driver_bound(struct device *dev, void *not_used) {
> >>> + int ret = 0;
> >>> +
> >>> + device_lock(dev);
> >>> + if (device_is_bound(dev))
> >>> + ret = 1;
> >>> + device_unlock(dev);
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> >>> +   const char *buf, size_t count) {
> >>> + int ret = 0, req_type = 0, req_auto = 0;
> >>> + struct iommu_domain *prv_dom;
> >>> + struct group_device *grp_dev;
> >>> + const struct iommu_ops *ops;
> >>> + struct device *dev;
> >>> +
> >>> + if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> >>> + return -EACCES;
> >>> +
> >>> + if (WARN_ON(!group))
> >>> + return -EINVAL;
> >>> +
> >>> + if (sysfs_streq(buf, "identity"))
> >>> + req_type = IOMMU_DOMAIN_IDENTITY;
> >>> + else if (sysfs_streq(buf, "DMA"))
> >>> + req_type = IOMMU_DOMAIN_DMA;
> >>> + else if (sysfs_streq(buf, "auto"))
> >>> + req_auto = 1;
> >>> + else
> >>> + return -EINVAL;
> >>> +
> >>> + /*
> >>> +  * Check if any device in the group still has a driver binded to it.
> >>&g

RE: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> On 2020/2/24 15:03, Prakhya, Sai Praneeth wrote:
> >> On 2020/2/24 11:20, Prakhya, Sai Praneeth wrote:
> >>>>> +   list_for_each_entry_safe(grp_dev, temp, >devices, list) {
> >>>>> +   struct device *dev;
> >>>>> +
> >>>>> +   dev = grp_dev->dev;
> >>>>> +   iommu_release_device(dev);
> >>>>> +
> >>>>> +   ret = iommu_group_add_device(group, dev);
> >>>>> +   if (ret)
> >>>>> +   dev_err(dev, "Failed to add to iommu group %d: 
> >>>>> %d\n",
> >>>>> +   group->id, ret);
> >>>> Need to handle this error case.
> >>> I wasn't sure on how to handle the error ☹
> >>
> >> Just roll back to the state before calling this function and return
> >> an appropriate error value.
> >>
> >> The likely behavior is detaching the new domains from all devices (if
> >> it has already attached), attaching the old domains to all devices in
> >> the group,
> >
> > And while handling this error case, there is a possibility that attaching 
> > to old
> domain could fail.. so, we might need to handle that error case as well. If we
> plan to handle error case, since we have removed devices from group above,
> adding them back could fail too.. that would lead into handling error case 
> for an
> error case.
> 
> We can assume that the old domain should always be attached back.
> Otherwise, there must be some bugs in the vendor iommu driver.
> 
> It must be able to role back, otherwise users have to reboot the system in 
> order
> to use the devices in the group. This is not acceptable in the production 
> kernel.

I agree that we should be able to roll back but I am afraid that the error 
handling code could become complex than the usual code that gets to run. For 
example, assume there are 'n' devices in the group, 'k' of them are 
successfully processed (i.e. default domain type has been changed) and if k+1 
fails while changing default domain type, to roll back state of k devices, we 
need to maintain a list of processed devices so that we now roll back state for 
devices in this list. The present code does not have any list because it's 
processing sequentially, it takes a device from the group, changes it domain 
and moves to other device and hence does not require a list.

All said, I could give this a try and see how complex the code could turn out 
to be. I hope I am wrong (i.e. turns out implementing error handling is simple).

> > So, given the probability of these functions failing here are very low, I 
> > think,
> why not bite the bullet and say, add code to handle these error cases if we 
> see
> that these functions are failing frequently? Otherwise the error handling 
> code is
> just a dead code.
> >
> >> cleaning
> >> up all new resources allocated in this function, putting a error
> >> message to tell the user why it fails and returning an error code.
> >>
> >>> i.e. group's domain/default_domain are already updated to new domain
> >>> and
> >> assume there are 'n' devices in the group and this failed for 'k'th
> >> device, I wasn't sure how I could roll back the changes made for k-1 
> >> devices.
> >>
> >> A successful attach could be checked by (group->domain ==
> >> group->default_domain).
> >
> > No.. because I have manually set group->domain ==
> > group->default_domain = new_domain (did this because
> > iommu_group_add_device() and iommu_group_create_direct_mappings()
> > needs them)
> 
> You could set group->domain to the default domain only after it has been
> attached to the device successfully, right?

Will reorder things and see how this could be handled.

> > So, probably we might need some other way to check successful attach.
> >
> >>> So, I thought probably just alert the user that there was an error
> >>> while
> >> changing default domain type and try updating for other devices in
> >> the group (hopefully other devices might succeed). Also,*generally*
> >> we shouldn't see any errors here because all these devices were
> >> already in the same group earlier (we aren’t adding/removing new
> >> devices to the group). We are just changing default domain type and
> >> we already made sure that device could be in the requested default domain
> type.

[snipped]

> >> At least I didn't see the iommu_def_domain_type is us

RE: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> On 2020/2/17 5:57, Sai Praneeth Prakhya wrote:
> > Presently, the default domain of an iommu_group is allocated during
> > boot time (i.e. when a device is being added to a group) and it cannot
> > be changed later. So, the device would typically be either in identity
> > (also known as pass_through) mode (controlled by "iommu=pt" kernel
> > command line
> 
> The default domain type is initialized according to the kernel build option, 
> and
> could be overrided by several kernel commands like iommu=pt and
> iommu.passthrough.

Yes, that's right. Will change this text.

> > argument) or the device would be in DMA mode as long as the machine is
> > up and running. There is no way to change the default domain type
> > dynamically i.e. after booting, a device cannot switch between
> > identity mode and DMA mode.
> >
> > But, assume a use case wherein the user trusts the device and also
> > believes that the OS is secure enough and hence wants *only* this
> > device to bypass IOMMU (so that it could be high performing) whereas
> > all the other devices to go through IOMMU (so that the system is
> > protected). Presently, this use case is not supported. Hence it will
> > be helpful if there is some way to change the default domain of a
> > B:D.F dynamically. Since, linux iommu subsystem prefers to deal at
> > iommu_group level instead of B:D.F level, it might be helpful if there
> > is some way to change the default domain of a
> > *iommu_group* dynamically. Hence, add such support.

[snipped]

> > +/*
> > + * Changes the default domain of a group
> > + *
> > + * @group: The group for which the default domain should be changed
> > + * @prv_dom: The previous domain that is being switched from
> 
> The previous domain is still kept in group->default_domain, so it's 
> unnecessary
> to pass it as a parameter, right?

Yes, you are right. I passed it as an argument thinking that I could make one 
assignment less in this function. The caller of this function 
iommu_group_store_type() already has prv_dom, so just passed it. But, will 
change this so that instead of every caller getting and passing the argument, 
it's easier to do it in this function.

> > + * @type: The type of the new default domain that gets associated
> > +with the group
> > + *
> > + * Returns 0 on success and error code on failure  */ static int
> > +iommu_group_change_def_domain(struct iommu_group *group,
> > +struct iommu_domain *prv_dom,
> > +int type)
> > +{
> > +   struct group_device *grp_dev, *temp;
> > +   struct iommu_domain *new_domain;
> > +   int ret;
> > +
> > +   /*
> > +* iommu_domain_alloc() takes "struct bus_type" as an argument which
> is
> > +* a member in "struct device". Changing a group's default domain type
> > +* deals at iommu_group level rather than device level and hence there
> > +* is no straight forward way to get "bus_type" of an iommu_group that
> > +* could be passed to iommu_domain_alloc(). So, instead of directly
> > +* calling iommu_domain_alloc(), use iommu_ops from previous default
> > +* domain.
> > +*/
> > +   if (!prv_dom || !prv_dom->ops || !prv_dom->ops->domain_alloc ||
> !type)
> > +   return -EINVAL;
> > +
> > +   /* Allocate a new domain of requested type */
> > +   new_domain = prv_dom->ops->domain_alloc(type);
> > +   if (!new_domain) {
> > +   pr_err("Unable to allocate memory for the new domain\n");
> > +   return -ENOMEM;
> > +   }
> > +
> > +   new_domain->type = type;
> > +   new_domain->ops = prv_dom->ops;
> > +   new_domain->pgsize_bitmap = prv_dom->pgsize_bitmap;
> > +
> > +   /*
> > +* Set these upfront here because iommu_group_add_device() and
> > +* iommu_group_create_direct_mappings() needs these to be set
> > +*/
> > +   mutex_lock(>mutex);
> > +   group->default_domain = new_domain;
> > +   group->domain = new_domain;
> > +   mutex_unlock(>mutex);
> > +
> > +   iommu_group_ref_get(group);
> > +
> > +   list_for_each_entry_safe(grp_dev, temp, >devices, list) {
> > +   struct device *dev;
> > +
> > +   dev = grp_dev->dev;
> > +   iommu_release_device(dev);
> > +
> > +   ret = iommu_group_add_device(group, dev);
> > +   if (ret)
> > +   dev_err(dev, "Failed to add to iommu group %d: %d\n",
> > +   group->id, ret);
> 
> Need to handle this error case.

I wasn't sure on how to handle the error ☹
i.e. group's domain/default_domain are already updated to new domain and assume 
there are 'n' devices in the group and this failed for 'k'th device, I wasn't 
sure how I could roll back the changes made for k-1 devices. So, I thought 
probably just alert the user that there was an error while changing default 
domain type and try updating for other devices in the group (hopefully other 
devices might succeed). Also, *generally* we shouldn't see any errors here 
because all these 

RE: [PATCH V2 2/5] iommu/vt-d: Rename device_def_domain_type() to intel_iommu_dev_def_domain_type()

[Prakhya, Sai Praneeth]

> >> On 2020/2/17 5:57, Sai Praneeth Prakhya wrote:
> >>> The functionality needed for iommu_ops->dev_def_domain_type() is
> >>> already provided by device_def_domain_type() in intel_iommu.c. But,
> >>> every call back function in intel_iommu_ops starts with intel_iommu
> >>> prefix, hence rename
> >>> device_def_domain_type() to intel_iommu_dev_def_domain_type() so
> >>> that it follows the same semantics.
> >>
> >> How about keep device_def_domain_type() and call it in the new
> >> intel_iommu_dev_def_domain_type()?
> >
> > Sure! I could but could you please explain the advantages we might get by
> doing so?
> > Less number of changes is what I could think of.. any other reasons?
> >
> 
> device_def_domain_type() is a quirk list for devices that must use a specified
> domain type. intel_iommu_dev_def_domain_type() tells the upper layer whether
> the device could switch to another type of domain. Put them in separated
> functions will make it easier for maintenance.

Ok. Will fix this.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 3/5] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> On 2020/2/24 16:12, Lu Baolu wrote:
> > On 2020/2/24 15:57, Prakhya, Sai Praneeth wrote:
> >>> On 2020/2/24 15:03, Prakhya, Sai Praneeth wrote:
> >>>>> On 2020/2/24 11:20, Prakhya, Sai Praneeth wrote:
> >>>>>>>> +    list_for_each_entry_safe(grp_dev, temp, >devices,
> >>>>>>>> list) {
> >>>>>>>> +    struct device *dev;
> >>>>>>>> +
> >>>>>>>> +    dev = grp_dev->dev;
> >>>>>>>> +    iommu_release_device(dev);
> >>>>>>>> +
> >>>>>>>> +    ret = iommu_group_add_device(group, dev);
> >>>>>>>> +    if (ret)
> >>>>>>>> +    dev_err(dev, "Failed to add to iommu group %d:
> >>>>>>>> +%d\n",
> >>>>>>>> +    group->id, ret);
> >>>>>>> Need to handle this error case.
> >>>>>> I wasn't sure on how to handle the error ☹
> >>>>> Just roll back to the state before calling this function and
> >>>>> return an appropriate error value.
> >>>>>
> >>>>> The likely behavior is detaching the new domains from all devices
> >>>>> (if it has already attached), attaching the old domains to all
> >>>>> devices in the group,
> >>>> And while handling this error case, there is a possibility that
> >>>> attaching to old
> >>> domain could fail.. so, we might need to handle that error case as
> >>> well. If we plan to handle error case, since we have removed devices
> >>> from group above, adding them back could fail too.. that would lead
> >>> into handling error case for an error case.
> >>>
> >>> We can assume that the old domain should always be attached back.
> >>> Otherwise, there must be some bugs in the vendor iommu driver.
> >>>
> >>> It must be able to role back, otherwise users have to reboot the
> >>> system in order to use the devices in the group. This is not
> >>> acceptable in the production kernel.
> >> I agree that we should be able to roll back but I am afraid that the
> >> error handling code could become complex than the usual code that
> >> gets to run. For example, assume there are 'n' devices in the group,
> >> 'k' of them are successfully processed (i.e. default domain type has
> >> been
> >> changed) and if k+1 fails while changing default domain type, to roll
> >> back state of k devices, we need to maintain a list of processed
> >> devices so that we now roll back state for devices in this list. The
> >> present code does not have any list because it's processing
> >> sequentially, it takes a device from the group, changes it domain and
> >> moves to other device and hence does not require a list.
> >>
> >> All said, I could give this a try and see how complex the code could
> >> turn out to be. I hope I am wrong (i.e. turns out implementing error
> >> handling is simple).
> >>
> >
> > I think something like below should work.
> >
> > static int iommu_group_do_attach_device(struct device *dev, void
> > *data) {
> >      struct iommu_domain *domain = data;
> >
> >      return __iommu_attach_device(domain, dev); }
> >
> > static int __iommu_attach_group(struct iommu_domain *domain,
> >      struct iommu_group *group) {
> >      int ret;
> >
> >      if (group->default_domain && group->domain !=
> > group->default_domain)
> >      return -EBUSY;
> >
> >      ret = __iommu_group_for_each_dev(group, domain,
> >
> > iommu_group_do_attach_device);
> >      if (ret == 0)
> >      group->domain = domain;
> >
> >      return ret;
> > }
> >
> > The vendor iommu driver should always deprecate the old domain if it
> > exists. Add a comment there.
> >
> 
> By the way, this is the expected behavior. Please check
> __iommu_detach_group().

Ok.. I will look into it.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 1/5] iommu: Add dev_def_domain_type() call back function to iommu_ops

[Prakhya, Sai Praneeth]

> 
> On 2020/2/17 5:57, Sai Praneeth Prakhya wrote:
> > When user requests kernel to change the default domain type of a group
> > through sysfs, kernel has to make sure that it's ok to change the
> > domain type of every device in the group to the requested domain
> > (every device may not support both the domain types i.e. DMA and
> > identity). Hence, add a call back function that could be implemented
> > per architecture that performs the above check.
> >
> > Cc: Christoph Hellwig 
> > Cc: Joerg Roedel 
> > Cc: Ashok Raj 
> > Cc: Will Deacon 
> > Cc: Lu Baolu 
> > Cc: Sohil Mehta 
> > Cc: Robin Murphy 
> > Cc: Jacob Pan 
> > Signed-off-by: Sai Praneeth Prakhya 
> > ---
> >   include/linux/iommu.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > d1b5f4d98569..3f4aaad0aeb7 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -248,6 +248,7 @@ struct iommu_iotlb_gather {
> >* @cache_invalidate: invalidate translation caches
> >* @sva_bind_gpasid: bind guest pasid and mm
> >* @sva_unbind_gpasid: unbind guest pasid and mm
> > + * @dev_def_domain_type: Return the required default domain type for
> > + a device
> 
> Can you please define the return value of this callback?

Sure! I will add it in the next version

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 2/5] iommu/vt-d: Rename device_def_domain_type() to intel_iommu_dev_def_domain_type()

[Prakhya, Sai Praneeth]

> 
> On 2020/2/17 5:57, Sai Praneeth Prakhya wrote:
> > The functionality needed for iommu_ops->dev_def_domain_type() is
> > already provided by device_def_domain_type() in intel_iommu.c. But,
> > every call back function in intel_iommu_ops starts with intel_iommu
> > prefix, hence rename
> > device_def_domain_type() to intel_iommu_dev_def_domain_type() so that
> > it follows the same semantics.
> 
> How about keep device_def_domain_type() and call it in the new
> intel_iommu_dev_def_domain_type()?

Sure! I could but could you please explain the advantages we might get by doing 
so?
Less number of changes is what I could think of.. any other reasons?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 0/5] iommu: Add support to change default domain of a group

[Prakhya, Sai Praneeth]

Hi Joerg,

> Presently, the default domain of a group is allocated during boot time and it
> cannot be changed later. So, the device would typically be either in identity
> (pass_through) mode or the device would be in DMA mode as long as the system
> is up and running. There is no way to change the default domain type
> dynamically i.e. after booting, a device cannot switch between identity mode
> and DMA mode.
> 
> Assume a use case wherein the privileged user would want to use the device in
> pass-through mode when the device is used for host so that it would be high
> performing. Presently, this is not supported. Hence add support to change the
> default domain of a group dynamically.
> 
> Support this through a sysfs file, namely
> "/sys/kernel/iommu_groups//type".
> 
> Hi Joerg,
> Sorry! for _huge_ delay in posting a V2 of this patch set. Was stuck with some
> internal PoC work. Will be consistent from now.
> 
> Changes from V1:
> 
> 1. V1 patch set wasn't updating dma_ops for some vendors (Eg: AMD), hence,
>change the logic of updating default domain as below (because adding a
> device
>to iommu_group automatically updates dma_ops)
>a. Allocate a new domain
>b. For every device in the group
>   i. Remove the device from the group
>   ii. Add the device back to the group
>c. Free previous domain
> 2. Drop 1st patch of V1 (iommu/vt-d: Modify device_def_domain_type() to use
> at
>runtime) because "iommu=pt" has no effect on this function anymore.
> 3. Added a patch to take/release lock while reading iommu_group-
> >default_domain->type
>because it can be changed any time by user.
> 4. Before changing default domain type of a group, check if the group is
>directly assigned for user level access. If so, abort.
> 5. Sanitize return path (using ternary operator) in iommu_group_store_type() 
> 6.
> Split 2nd patch of V1 (iommu: Add device_def_domain_type() call back function
>to iommu_ops) into two patches such that iommu generic changes are now in
> 1st
>patch of V2 and vt-d specific changes are in 2nd patch of V2.
> 7. Rename device_def_domain_type() to dev_def_domain_type() 8. Remove
> example from documentation 9. Change the value written to file
> "/sys/kernel/iommu_groups//type"
>from "dma" to "DMA".

Just a gentle reminder, could you please review this patch set and let me know 
your comments?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V2 5/5] iommu: Document usage of "/sys/kernel/iommu_groups//type" file

[Prakhya, Sai Praneeth]

> > +What: /sys/kernel/iommu_groups//type

> > +Date:  February 2020

> > +KernelVersion:  v5.6

> > +Contact: Sai Praneeth Prakhya 
> > mailto:sai.praneeth.prak...@intel.com>>

> > +Description:  Lets the user know the type of default domain in 
> > use by iommu

> > + for this group. A privileged user could request 
> > kernel to change

>

> Let the users know the default domain type of the IOMMU group by reading this

> file. A privileged user could request to change it by writing to this file.



Sure! Will fix it.



> > + the group type by writing to this file. Presently, 
> > only three

> > + types are supported

> > + 1. DMA: All the DMA transactions from the devices in 
> > this group

> > +are translated by the iommu.

> > + 2. identity: All the DMA transactions from the 
> > devices in this

> > + group are *not* translated by the 
> > iommu.

> > + 3. auto: Kernel decides one among DMA/identity mode 
> > and

> hence

> > +when the user reads the file he would 
> > never see "auto".

>

> Just out of curiosity, when could a user reads "auto" from this file?



Never, because when a user requests kernel to change to "auto" mode, kernel 
will select one among identity or DMA. So, default domain type cannot be auto, 
it's always either identity or DMA.

"auto" mode is just a write value where user is requesting kernel to select one 
among identity or DMA.

> > +This is just a write only value.

> > + Note:

> > + -

> > + A group type could be modified only when *all* the 
> > devices in

>

> group's default domain type (not group type).



Sure! Makes sense.. will fix it 



>

> > + the group are not binded to any device driver. So, 
> > the user has

>

> bound



Makes sense.. will fix this too.



Regards,

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

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

> -Original Message-
> From: Joerg Roedel 
> Sent: Friday, May 15, 2020 8:46 AM
> To: Lu Baolu 
> Cc: Prakhya, Sai Praneeth ;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Fri, May 15, 2020 at 08:55:42PM +0800, Lu Baolu wrote:
> > It seems that we can do like this:
> >
> > [1] mutex_lock(>lock)
> > [2] for_each_group_device(device_lock())
> > [3] if (for_each_group_device(!device_is_bound()))
> > change_default_domain()
> > [4] for_each_group_device_reverse(device_unlock())
> > [5] mutex_unlock(>lock)

>> A possible problem exists at step 2 when another thread is trying to lock 
>> devices in the reverse order at the same time. -> By Allen

Makes sense.. this could happen and we could prevent this if the iommu_group 
has only one device. So, going with Joerg's suggestion, if we support dynamic 
switching of default-domain of a group with only one device, I think we 
shouldn't hit this issue.

> The problem here is that I am pretty sure we also have:
> 
>   device_lock() /* from device/driver core code */
>   -> bus_notifier()
> -> iommu_bus_notifier()
>   -> ...
> -> mutex_lock(>lock)
> 
> Which would cause lock-inversion with the above code.

Thanks for this call chain, Joerg. It makes sense to me how lock inversion 
could happen

iommu_bus_notifier()
-> iommu_release_device()
 -> ops->release_device() (Eg: intel_iommu_release_device())
  -> iommu_group_remove_device()
   -> mutex_lock()

But, I added print statements to iommu_bus_notifier() and 
iommu_group_remove_device() and noticed that iommu_group_remove_device() wasn't 
being called upon modprobe -r  and iommu_probe_device() isn't 
called upon modprobe  because

if (action == BUS_NOTIFY_ADD_DEVICE) {
int ret;

ret = iommu_probe_device(dev);
return (ret) ? NOTIFY_DONE : NOTIFY_OK;
} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
iommu_release_device(dev);
return NOTIFY_OK;
}

"action" was != BUS_NOTIFY_ADD_DEVICE || BUS_NOTIFY_REMOVED_DEVICE upon 
modprobe. So (after booting [1]), I am wondering when would action be == to one 
of the above so that these iommu functions get called.

And,
switch (action) {
case BUS_NOTIFY_BIND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_BIND_DRIVER;
break;
case BUS_NOTIFY_BOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_BOUND_DRIVER;
break;
case BUS_NOTIFY_UNBIND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBIND_DRIVER;
break;
case BUS_NOTIFY_UNBOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
break;
}

if (group_action)
blocking_notifier_call_chain(>notifier,
 group_action, dev);

I also noticed that vfio is the only one that registers for group notifiers and 
from a first look it wasn't very obvious if vfio is trying to get group->mutex.

[1] I am interested in after booting because dynamic switching of iommu_group 
default-domain is supported only through sysfs.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Friday, May 15, 2020 2:59 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Lu Baolu 
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 11:12:52PM +0000, Prakhya, Sai Praneeth wrote:
> > +static int is_driver_bound(struct device *dev, void *not_used) {
> > +   int ret = 0;
> > +
> > +   device_lock(dev);
> > +   if (device_is_bound(dev))
> > +   ret = 1;
> > +   device_unlock(dev);
> > +   return ret;
> > +}
> 
> This locks only one device, so without lock-conversion there could be a driver
> probe after the device_unlock(), while we are probing the other devices of the
> group.
> 
> > [SNIP]
> >
> > +   /*
> > +* Check if any device in the group still has a driver binded to it.
> > +* This might race with device driver probing code and unfortunately
> > +* there is no clean way out of that either, locking all devices in the
> > +* group and then do the re-attach will introduce a lock-inversion with
> > +* group->mutex - Joerg.
> > +*/
> > +   if (iommu_group_for_each_dev(group, NULL, is_driver_bound)) {
> > +   pr_err("Active drivers exist for devices in the group\n");
> > +   return -EBUSY;
> > +   }
> 
> The lock inversion comes into the picture when this code is called from
> device(-driver) core through the bus-notifiers. The notifiers are called with 
> the
> device already locked.

Make sense. I will look through that code.

> > Another question I have is.. if it's racy then it should be racy even
> > for one device iommu groups.. right? Why would it be racy only with
> > multiple devices iommu group?
> 
> Valid point. So the device needs to be locked _while_ the default domain 
> change
> happens. If triggered by sysfs there should be no locking problems, I guess. 
> But
> you better try it out.

I will try this out and will update you.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Thursday, May 14, 2020 11:33 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Lu Baolu 
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 05:51:39PM +0000, Prakhya, Sai Praneeth wrote:
> > Sorry! didn't get that quite well. When you meant "per-group
> > default-domain patch-set", do you mean the patch set that I am working
> > on which changes iommu group default domain dynamically by writing to
> > sysfs file?
> 
> Not only the sysfs file, but also changing it at boot already. Note that 
> changing
> the default-domain at runtime is only possible for single-device groups.

Could you please explain why we shouldn't change default-domain for an iommu 
group that has multiple devices?

I am asking this particularly because the patch set I am working on allows to 
change default-domain for an iommu group that has multiple devices. The 
pre-requisite being that all the devices in the group should already be 
unbounded from the device driver and the default-domain preferences of all the 
devices in the group shouldn't have conflicting types i.e. some devices cannot 
say they *only* need identity domain while other devices in the same group say 
that they *only* need to be in DMA domain. In this case, we will not be able to 
decide upon a default-domain for the iommu group.

> I'll queue that patch tomorrow.

Great! I will take a look at it.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Thursday, May 14, 2020 12:56 PM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Lu Baolu 
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 06:44:16PM +0000, Prakhya, Sai Praneeth wrote:
> > Could you please explain why we shouldn't change default-domain for an
> > iommu group that has multiple devices?
> 
> Because you can't be sure that a device is bound to a driver while the default
> domain of the group is changed. As long as this race condition exists we can't
> change the default domains of groups with multiple devices at runtime.
> 
> > I am asking this particularly because the patch set I am working on
> > allows to change default-domain for an iommu group that has multiple
> > devices. The pre-requisite being that all the devices in the group
> > should already be unbounded from the device driver and the
> > default-domain preferences of all the devices in the group shouldn't
> > have conflicting types i.e. some devices cannot say they *only* need
> > identity domain while other devices in the same group say that they
> > *only* need to be in DMA domain. In this case, we will not be able to
> > decide upon a default-domain for the iommu group.
> 
> Yeah, but as I wrote above, this is racy and there is currently no way to fix 
> that.
> So we can't support it.

Ok.. below is a previous *similar* comment that you had for one of my patches 
that change default-domain dynamically.. could you please elaborate it more so 
that I could have better understanding?
Specifically this one.. "locking all devices in the group and then do the 
re-attach will introduce a lock-inversion with group->mutex". I didn't get 
which function call chain would lead us to lock inversion. Could you please 
shed some light here?

+static int is_driver_bound(struct device *dev, void *not_used)
+{
+   int ret = 0;
+
+   device_lock(dev);
+   if (device_is_bound(dev))
+   ret = 1;
+   device_unlock(dev);
+   return ret;
+}

[SNIP]

+   /*
+* Check if any device in the group still has a driver binded to it.
+* This might race with device driver probing code and unfortunately
+* there is no clean way out of that either, locking all devices in the
+* group and then do the re-attach will introduce a lock-inversion with
+* group->mutex - Joerg.
+*/
+   if (iommu_group_for_each_dev(group, NULL, is_driver_bound)) {
+   pr_err("Active drivers exist for devices in the group\n");
+   return -EBUSY;
+   }

Another question I have is.. if it's racy then it should be racy even for one 
device iommu groups.. right? Why would it be racy only with multiple devices 
iommu group?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Thursday, May 14, 2020 6:13 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Lu Baolu 
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Wed, May 13, 2020 at 03:47:21PM -0700, Sai Praneeth Prakhya wrote:
> > After moving iommu_group setup to iommu core code [1][2] and removing
> > private domain support in vt-d [3], there are no users for functions
> > such as iommu_request_dm_for_dev(),
> iommu_request_dma_domain_for_dev()
> > and request_default_domain_for_dev(). So, remove these functions.
> 
> I thought these functions are needed for the per-group default-domain patch-
> set? That is why I left them in for now, but I can also remove them if not.

Sorry! didn't get that quite well. When you meant "per-group default-domain 
patch-set", do you mean the patch set that I am working on which changes iommu 
group default domain dynamically by writing to sysfs file?
If so, they don't use these functions so it should be ok to remove them. If you 
meant some other patch set, could you please point me to them?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V6 1/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Friday, September 4, 2020 2:42 PM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj,
> Ashok ; Will Deacon ; Lu Baolu
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V6 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> On Fri, Sep 04, 2020 at 07:11:07PM +, Prakhya, Sai Praneeth wrote:
> > But couple of questions..
> > 1. Do you want me to post the entire patch series? (i.e. 3 patches) or
> > do you want me to post just this patch i.e. 1st patch only 2. Do you want 
> > me to
> bump the version number? i.e. post it as V7 ?
> > 3. Didn't get what you meant here.. "woth b4" ☹
> 
> Please resend all 3 patches a v7, b4 is just a tool I am using to download the
> patches from the mailing list and add all tags[1].

Thanks for answering the questions. It's clear to me now.. I will post a V7 

Regards,
Sai

> Regards,
> 
>   Joerg
> 
> [1] https://people.kernel.org/monsieuricon/introducing-b4-and-patch-
> attestation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V6 1/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Friday, September 4, 2020 1:31 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj,
> Ashok ; Will Deacon ; Lu Baolu
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V6 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> Hi Sai,
> 
> On Sun, Aug 23, 2020 at 10:17:26PM -0700, Sai Praneeth Prakhya wrote:
> >  drivers/iommu/iommu.c | 225
> > +-
> >  1 file changed, 224 insertions(+), 1 deletion(-)
> 
> Can you please post this as a new and separate thread so I can grab it woth 
> b4?

Sure! I will post a new version.

But couple of questions..
1. Do you want me to post the entire patch series? (i.e. 3 patches) or do you 
want me to post just this patch i.e. 1st patch only
2. Do you want me to bump the version number? i.e. post it as V7 ?
3. Didn't get what you meant here.. "woth b4" ☹

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Remove functions that support private domain

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Monday, May 25, 2020 6:57 AM
> To: Prakhya, Sai Praneeth 
> Cc: Lu Baolu ; iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Sun, May 17, 2020 at 08:29:17AM +0000, Prakhya, Sai Praneeth wrote:
> > iommu_bus_notifier()
> > -> iommu_release_device()
> >  -> ops->release_device() (Eg: intel_iommu_release_device())
> >   -> iommu_group_remove_device()
> >-> mutex_lock()
> >
> > But, I added print statements to iommu_bus_notifier() and
> > iommu_group_remove_device() and noticed that
> > iommu_group_remove_device() wasn't being called upon modprobe -r
> >  and iommu_probe_device() isn't called upon modprobe
> >  because
> 
> Calling modprobe is the device driver binding path, the release_device event 
> is
> called when a device is going away, e.g. on hotunplug or when a VF of a PCI
> device is released.
> 
> This could also happen at runtime, and the code needs to protect against that.
> My suggestion is still to limit runtime-changing of default domains to groups
> with only one device. The flow would be as follows:

Thanks for explaining how lock release path could be called at run time. It 
makes sense to me now.

> 
>   1. device_lock(dev);
>  // Device can't be bound to a driver or hot-removed from now
>  // on.
> 
>   2. if (device_is_bound_to_some_driver(dev))
>  goto 6;
> 
>   3. mutex_lock(>mutex);
>  // group of dev
> 
>   4. Change the default domain
> 
>   5. mutex_unlock(>mutex);
> 
>   6. device_unlock(dev);
> 
> This avoids lock inversion and should be safe with regard to hotplug and 
> device
> driver probing.

Thanks for the steps. I have implemented "changing default domain" following 
the steps you gave above. Could you please review the patch set when you have 
some time and let me know your comments?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V3 1/3] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

Hi Baolu,

> -Original Message-
> From: Lu Baolu 
> Sent: Thursday, May 28, 2020 7:43 PM
> To: Prakhya, Sai Praneeth ;
> iommu@lists.linux-foundation.org
> Cc: baolu...@linux.intel.com; Christoph Hellwig ; Joerg Roedel
> ; Raj, Ashok ; Will Deacon
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V3 1/3] iommu: Add support to change default domain of
> an iommu_group
> 
> Hi Sai,
> 
> On 5/29/20 3:23 AM, Sai Praneeth Prakhya wrote:
> > Presently, the default domain of an iommu_group is allocated during
> > boot time (i.e. when a device is being added to a group) and it cannot
> > be
>   
> 
> This is inaccurate as Joerg's code has deferred default domain allocation and
> attaching after group allocation. I'd suggest to remove this.

Ok.. makes sense. I will remove it. I think it should have been like below to 
accurately describe Joerg's changes, is that correct?

"Presently, the default domain of an iommu_group is allocated during
boot time (i.e. during device probe and after the device is added to a group) 
and it cannot
be"
 
> > changed later. So, the device would typically be either in identity
> > (also known as pass_through) mode (controlled by "iommu=pt" kernel
> > command line
>
> 
> There are other kernel parameters to put device in pass_through mode.
> I'd suggest to remove this.

Ok.. makes sense. I will remove it.
I think, you were talking about ARM parameters (iommu.passthrough).. am I right?

> > argument) or the device would be in DMA mode as long as the machine is
> > up and running. There is no way to change the default domain type
> > dynamically i.e. after booting, a device cannot switch between
> > identity mode and DMA mode.
> >
> > But, assume a use case wherein the user trusts the device and believes
> > that the OS is secure enough and hence wants *only* this device to
> > bypass IOMMU (so that it could be high performing) whereas all the
> > other devices to go through IOMMU (so that the system is protected).
> > Presently, this use case is not supported. It will be helpful if there
> > is some way to change the default domain of a B:D.F dynamically. Hence, add
> such support.
>^
> 
> Currently default domain is per iommu_group, we have no per device default
> domain yet. Probably, "default domain of an iommu group"?

Makes sense. I will change it.

> > A privileged user could request the kernel to change the default
> > domain type of a iommu_group by writing to
> > "/sys/kernel/iommu_groups//type" file. Presently, only three
> > values are supported 1. identity: all the DMA transactions from the
> > device in this group are
> >   *not* translated by the iommu 2. DMA: all the DMA
> > transactions from the device in this group are
> >  translated by the iommu
> > 3. auto: change to the type the device was booted with
> >
> > Note:
> > 1. Default domain of an iommu group with two or more devices cannot be
> > changed.
> > 2. The device in the iommu group shouldn't be bound to any driver.
> > 3. The device shouldn't be assigned to user for direct access.
> >
> > Please see "Documentation/ABI/testing/sysfs-kernel-iommu_groups" for
> > more information.
> >
> > Cc: Christoph Hellwig 
> > Cc: Joerg Roedel 
> > Cc: Ashok Raj 
> > Cc: Will Deacon 
> > Cc: Lu Baolu 
> > Cc: Sohil Mehta 
> > Cc: Robin Murphy 
> > Cc: Jacob Pan 
> > Signed-off-by: Sai Praneeth Prakhya 
> > ---
> >   drivers/iommu/iommu.c | 211
> +-
> >   1 file changed, 210 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > a4c2f122eb8b..2b6cca799055 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -92,6 +92,8 @@ static void __iommu_detach_group(struct
> iommu_domain *domain,
> >   static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
> >struct device *dev);
> >   static struct iommu_group *iommu_group_get_for_dev(struct device
> > *dev);
> > +static ssize_t iommu_group_store_type(struct iommu_group *group,
> > + const char *buf, size_t count);
> >
> >   #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)
>   \
> >   struct iommu_group_attribute iommu_group_attr_##_name =   \
> > @@ -5

RE: [PATCH V3 0/3] iommu: Add support to change default domain of an iommu

[Prakhya, Sai Praneeth]

Hi Baolu,

> -Original Message-
> From: Lu Baolu 
> Sent: Thursday, May 28, 2020 6:52 PM
> To: Prakhya, Sai Praneeth ;
> iommu@lists.linux-foundation.org
> Cc: baolu...@linux.intel.com; Christoph Hellwig ; Joerg Roedel
> ; Raj, Ashok ; Will Deacon
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V3 0/3] iommu: Add support to change default domain of
> an iommu
> 
> Hi Sai,
> 
> On 5/29/20 3:23 AM, Sai Praneeth Prakhya wrote:
> > Presently, the default domain of a iommu group is allocated during
> > boot time and it cannot be changed later. So, the device would
> > typically be either in identity
> > (pass_through) mode or the device would be in DMA mode as long as the
> > system is up and running. There is no way to change the default domain
> > type dynamically i.e. after booting, a device cannot switch between identity
> mode and DMA mode.
> >
> > Assume a use case wherein the privileged user would want to use the
> > device in pass-through mode when the device is used for host so that
> > it would be high performing. Presently, this is not supported. Hence
> > add support to change the default domain of an iommu group dynamically.
> >
> > Support this by writing to a sysfs file, namely
> > "/sys/kernel/iommu_groups//type".
> 
> The email subject
> 
> [PATCH V3 0/3] iommu: Add support to change default domain of an iommu
> 
> probably should be changed to
> 
> [PATCH V3 0/3] iommu: Add support to change default domain of an iommu
> group

Oops.. my bad. I have it in the patches that I have on my dev machine. I added 
"group" to a new line because the subject line crossed 80 character limit. 
Probably, I will just put it in the same line for the next version. Thanks for 
bringing this up.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V5 0/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Wednesday, July 29, 2020 5:37 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj,
> Ashok ; Will Deacon ; Lu Baolu
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V5 0/3] iommu: Add support to change default domain of
> an iommu group
> 
> Hi,
> 
> On Fri, Jul 24, 2020 at 12:51:57PM -0700, Sai Praneeth Prakhya wrote:
> > Tested only for intel_iommu/vt-d. Would appreciate if someone could
> > test on AMD and ARM based machines.
> 
> This looks good to me now, but I want to test it some more on other hardware
> and look up the implications of the probe_finalize calls when changing the
> default domain.

Makes sense to me.

> Since I am technically on vacation this week and next and don't have the
> resources around to do proper testing, I defer this for the next round.
> Please re-send this patch-set when the merge window closes.

Sure! Makes sense. I will resend the patch-set after this merge window closes.

Regards,
Sai

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

RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

Replying again because I noticed that I couldn't find this mail in the external 
iommu mailing list while I was able to find your comments on my patch. Also, 
could you please answer my two questions below?

> -Original Message-
> From: iommu  On Behalf Of
> Prakhya, Sai Praneeth
> Sent: Tuesday, June 30, 2020 8:04 PM
> To: Joerg Roedel 
> Cc: Raj, Ashok ; Will Deacon ;
> iommu@lists.linux-foundation.org; Robin Murphy ;
> Christoph Hellwig 
> Subject: RE: [PATCH V4 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> Hi Joerg,
> 
> > -Original Message-
> > From: Joerg Roedel 
> > Sent: Tuesday, June 30, 2020 2:16 AM
> > To: Prakhya, Sai Praneeth 
> > Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ;
> > Raj, Ashok ; Will Deacon ;
> > Lu Baolu ; Mehta, Sohil
> > ; Robin Murphy ; Jacob
> > Pan 
> > Subject: Re: [PATCH V4 1/3] iommu: Add support to change default
> > domain of an iommu group
> >
> > On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote:
> > > +static int iommu_change_dev_def_domain(struct iommu_group *group,
> > > +int
> > > +type) {
> > > + struct iommu_domain *prev_dom;
> > > + struct group_device *grp_dev;
> > > + const struct iommu_ops *ops;
> > > + int ret, dev_def_dom;
> > > + struct device *dev;
> > > +
> > > + if (!group)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(>mutex);
> > > +
> > > + if (group->default_domain != group->domain) {
> > > + pr_err_ratelimited("Group assigned to user level for direct
> > > +access\n");
> >
> > Make this message: "Group not assigned to default domain\n".
> 
> Sure! I will change it
> 
> > > + ret = -EBUSY;
> > > + goto out;
> > > + }
> > > +
> > > + /*
> > > +  * iommu group wasn't locked while acquiring device lock in
> > > +  * iommu_group_store_type(). So, make sure that the device count
> > hasn't
> > > +  * changed while acquiring device lock.
> > > +  *
> > > +  * Changing default domain of an iommu group with two or more
> > devices
> > > +  * isn't supported because there could be a potential deadlock. Consider
> > > +  * the following scenario. T1 is trying to acquire device locks of all
> > > +  * the devices in the group and before it could acquire all of them,
> > > +  * there could be another thread T2 (from different sub-system and use
> > > +  * case) that has already acquired some of the device locks and might be
> > > +  * waiting for T1 to release other device locks.
> > > +  */
> > > + if (iommu_group_device_count(group) != 1) {
> > > + pr_err_ratelimited("Cannot change default domain of a group
> > with
> > > +two or more devices\n");
> >
> > "Can not change default domain: Group has more than one device\n"
> 
> Ok.. make sense. I will change this.
> 
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + /* Since group has only one device */
> > > + list_for_each_entry(grp_dev, >devices, list)
> > > + dev = grp_dev->dev;
> > > +
> > > + prev_dom = group->default_domain;
> > > + if (!prev_dom || !prev_dom->ops || !prev_dom->ops-
> > >def_domain_type) {
> > > + pr_err_ratelimited("'def_domain_type' call back isn't
> > > +registered\n");
> >
> > This message isn't needed.
> 
> Ok. I will remove it.
> 
> > > + ret = __iommu_attach_device(group->default_domain, dev);
> > > + if (ret)
> > > + goto free_new_domain;
> > > +
> > > + group->domain = group->default_domain;
> > > +
> > > + ret = iommu_create_device_direct_mappings(group, dev);
> > > + if (ret)
> > > + goto free_new_domain;
> >
> > You need to create the direct mappings before you attach the device to
> > the new domain. Otherwise there might be a short time-window where
> > RMRR regions are not mapped.
> 
> Ok.. makes sense. I will change this accordingly.
> 
> > > +static ssize_t iommu_group_store_type(struct iommu_group *group,
> > > +   const char *buf, size_t count) {
> > > + struct group_device *grp_dev;
> > > + struct device *dev;
> > > + int ret, req_type;
> > > +
> > > + if

RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Joerg Roedel 
> Sent: Tuesday, June 30, 2020 2:16 AM
> To: Prakhya, Sai Praneeth 
> Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj,
> Ashok ; Will Deacon ; Lu Baolu
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote:
> > +static int iommu_change_dev_def_domain(struct iommu_group *group, int
> > +type) {
> > +   struct iommu_domain *prev_dom;
> > +   struct group_device *grp_dev;
> > +   const struct iommu_ops *ops;
> > +   int ret, dev_def_dom;
> > +   struct device *dev;
> > +
> > +   if (!group)
> > +   return -EINVAL;
> > +
> > +   mutex_lock(>mutex);
> > +
> > +   if (group->default_domain != group->domain) {
> > +   pr_err_ratelimited("Group assigned to user level for direct
> > +access\n");
> 
> Make this message: "Group not assigned to default domain\n".

Sure! I will change it

> > +   ret = -EBUSY;
> > +   goto out;
> > +   }
> > +
> > +   /*
> > +* iommu group wasn't locked while acquiring device lock in
> > +* iommu_group_store_type(). So, make sure that the device count
> hasn't
> > +* changed while acquiring device lock.
> > +*
> > +* Changing default domain of an iommu group with two or more
> devices
> > +* isn't supported because there could be a potential deadlock. Consider
> > +* the following scenario. T1 is trying to acquire device locks of all
> > +* the devices in the group and before it could acquire all of them,
> > +* there could be another thread T2 (from different sub-system and use
> > +* case) that has already acquired some of the device locks and might be
> > +* waiting for T1 to release other device locks.
> > +*/
> > +   if (iommu_group_device_count(group) != 1) {
> > +   pr_err_ratelimited("Cannot change default domain of a group
> with
> > +two or more devices\n");
> 
> "Can not change default domain: Group has more than one device\n"

Ok.. make sense. I will change this.

> > +   ret = -EINVAL;
> > +   goto out;
> > +   }
> > +
> > +   /* Since group has only one device */
> > +   list_for_each_entry(grp_dev, >devices, list)
> > +   dev = grp_dev->dev;
> > +
> > +   prev_dom = group->default_domain;
> > +   if (!prev_dom || !prev_dom->ops || !prev_dom->ops-
> >def_domain_type) {
> > +   pr_err_ratelimited("'def_domain_type' call back isn't
> > +registered\n");
> 
> This message isn't needed.

Ok. I will remove it.

> > +   ret = __iommu_attach_device(group->default_domain, dev);
> > +   if (ret)
> > +   goto free_new_domain;
> > +
> > +   group->domain = group->default_domain;
> > +
> > +   ret = iommu_create_device_direct_mappings(group, dev);
> > +   if (ret)
> > +   goto free_new_domain;
> 
> You need to create the direct mappings before you attach the device to the new
> domain. Otherwise there might be a short time-window where RMRR regions
> are not mapped.

Ok.. makes sense. I will change this accordingly.

> > +static ssize_t iommu_group_store_type(struct iommu_group *group,
> > + const char *buf, size_t count) {
> > +   struct group_device *grp_dev;
> > +   struct device *dev;
> > +   int ret, req_type;
> > +
> > +   if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> > +   return -EACCES;
> > +
> > +   if (WARN_ON(!group))
> > +   return -EINVAL;
> > +
> > +   if (sysfs_streq(buf, "identity"))
> > +   req_type = IOMMU_DOMAIN_IDENTITY;
> > +   else if (sysfs_streq(buf, "DMA"))
> > +   req_type = IOMMU_DOMAIN_DMA;
> > +   else if (sysfs_streq(buf, "auto"))
> > +   req_type = 0;
> > +   else
> > +   return -EINVAL;
> > +
> > +   /*
> > +* Lock/Unlock the group mutex here before device lock to
> > +* 1. Make sure that the iommu group has only one device (this is a
> > +*prerequisite for step 2)
> > +* 2. Get struct *dev which is needed to lock device
> > +*/
> > +   mutex_lock(>mutex);
> > +   if (iommu_group_device_count(group) != 1) {
> > +   mutex_unlock(>mutex);
> > +  

RE: [PATCH V4 0/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

> -Original Message-
> From: Prakhya, Sai Praneeth 
> Sent: Thursday, June 4, 2020 6:32 PM
> To: iommu@lists.linux-foundation.org
> Cc: Prakhya, Sai Praneeth ; Christoph Hellwig
> ; Joerg Roedel ; Raj, Ashok
> ; Will Deacon ; Lu Baolu
> ; Mehta, Sohil ; Robin
> Murphy ; Jacob Pan 
> Subject: [PATCH V4 0/3] iommu: Add support to change default domain of an
> iommu group
> 
> Presently, the default domain of an iommu group is allocated during boot time
> and it cannot be changed later. So, the device would typically be either in
> identity (pass_through) mode or the device would be in DMA mode as long as
> the system is up and running. There is no way to change the default domain 
> type
> dynamically i.e. after booting, a device cannot switch between identity mode
> and DMA mode.
> 
> Assume a use case wherein the privileged user would want to use the device in
> pass-through mode when the device is used for host so that it would be high
> performing. Presently, this is not supported. Hence add support to change the
> default domain of an iommu group dynamically.
> 
> Support this by writing to a sysfs file, namely
> "/sys/kernel/iommu_groups//type".
> 
> Testing:
> 
> Tested by dynamically changing storage device (nvme) from 1. identity mode to
> DMA and making sure file transfer works 2. DMA mode to identity mode and
> making sure file transfer works Tested only for intel_iommu/vt-d. Would
> appreciate if someone could test on AMD and ARM based machines.
> 
> Based on iommu maintainer's 'next' branch.
> 
> Changes from V3:
> 
> 1. Made changes to commit message as suggested by Baolu.
> 2. Don't pass "prev_dom" and "dev" as parameters to
>iommu_change_dev_def_domain(). Instead get them from group.
> 3. Sanitize the logic to validate user default domain type request. The logic
>remains same but is implmented differently.
> 4. Push lot of error checking into iommu_change_dev_def_domain() from
>iommu_group_store_type().
> 5. iommu_change_dev_def_domain() takes/releases group mutex as needed.
> So, it
>shouldn't be called holding a group mutex.
> 6. Use pr_err_ratelimited() instead of pr_err() to avoid DOS attack.

Could you please review this patch set and let me know if you have any comments?

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group

[Prakhya, Sai Praneeth]

Hi Joerg,

Thanks for the reply. I will spin another version of the patch addressing your 
comments.

> -Original Message-
> From: Joerg Roedel 
> Sent: Wednesday, July 22, 2020 6:53 AM
> To: Prakhya, Sai Praneeth 
> Cc: Raj, Ashok ; Will Deacon ;
> iommu@lists.linux-foundation.org; Robin Murphy ;
> Christoph Hellwig 
> Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> On Tue, Jul 14, 2020 at 06:23:54PM +, Prakhya, Sai Praneeth wrote:
> > Q1:
> > > Presently, iommu_change_dev_def_domain() checks if the iommu group
> > > still has only one device or not. Hence, checking if iommu group has
> > > one device or not is done twice, once before taking device_lock()
> > > and the other, after taking device_lock().
> > >
> > > I agree that the code isn't checking if the iommu group still has
> > > the _same_ device or not.
> > > One way, I could think of doing it is by storing "dev" temporarily
> > > and checking for it.
> > > Do you think that's ok? Or would you rather suggest something else?
> 
> That sounds reasonable, get the device from the group, lock it, take
> group->mutex, and check whether the same device is still alone in the
> group.

Sounds good! I will implement this.

Regards,
Sai
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu