Messages in this thread | | | Subject | Re: [PATCH 1/2] iommu: Add iommu_group_get/set_domain() | From | Lu Baolu <> | Date | Thu, 2 Jul 2020 10:36:21 +0800 |
| |
Hi Robin,
On 7/1/20 8:18 PM, Robin Murphy wrote: > On 2020-07-01 08:32, Lu Baolu wrote: >> Hi Robin, >> >> On 2020/7/1 0:51, Robin Murphy wrote: >>> On 2020-06-30 02:03, Lu Baolu wrote: >>>> Hi Robin, >>>> >>>> On 6/29/20 7:56 PM, Robin Murphy wrote: >>>>> On 2020-06-27 04:15, Lu Baolu wrote: >>>>>> The hardware assistant vfio mediated device is a use case of iommu >>>>>> aux-domain. The interactions between vfio/mdev and iommu during mdev >>>>>> creation and passthr are: >>>>>> >>>>>> - Create a group for mdev with iommu_group_alloc(); >>>>>> - Add the device to the group with >>>>>> group = iommu_group_alloc(); >>>>>> if (IS_ERR(group)) >>>>>> return PTR_ERR(group); >>>>>> >>>>>> ret = iommu_group_add_device(group, &mdev->dev); >>>>>> if (!ret) >>>>>> dev_info(&mdev->dev, "MDEV: group_id = %d\n", >>>>>> iommu_group_id(group)); >>>>>> - Allocate an aux-domain >>>>>> iommu_domain_alloc() >>>>>> - Attach the aux-domain to the physical device from which the mdev is >>>>>> created. >>>>>> iommu_aux_attach_device() >>>>>> >>>>>> In the whole process, an iommu group was allocated for the mdev >>>>>> and an >>>>>> iommu domain was attached to the group, but the group->domain leaves >>>>>> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore. >>>>>> >>>>>> This adds iommu_group_get/set_domain() so that group->domain could be >>>>>> managed whenever a domain is attached or detached through the >>>>>> aux-domain >>>>>> api's. >>>>> >>>>> Letting external callers poke around directly in the internals of >>>>> iommu_group doesn't look right to me. >>>> >>>> Unfortunately, it seems that the vifo iommu abstraction is deeply bound >>>> to the IOMMU subsystem. We can easily find other examples: >>>> >>>> iommu_group_get/set_iommudata() >>>> iommu_group_get/set_name() >>>> ... >>> >>> Sure, but those are ways for users of a group to attach useful >>> information of their own to it, that doesn't matter to the IOMMU >>> subsystem itself. The interface you've proposed gives callers rich >>> new opportunities to fundamentally break correct operation of the API: >>> >>> dom = iommu_domain_alloc(); >>> iommu_attach_group(dom, grp); >>> ... >>> iommu_group_set_domain(grp, NULL); >>> // oops, leaked and can't ever detach properly now >>> >>> or perhaps: >>> >>> grp = iommu_group_alloc(); >>> iommu_group_add_device(grp, dev); >>> iommu_group_set_domain(grp, dom); >>> ... >>> iommu_detach_group(dom, grp); >>> // oops, IOMMU driver might not handle this >>> >>>>> If a regular device is attached to one or more aux domains for >>>>> PASID use, iommu_get_domain_for_dev() is still going to return the >>>>> primary domain, so why should it be expected to behave differently >>>>> for mediated >>>> >>>> Unlike the normal device attach, we will encounter two devices when it >>>> comes to aux-domain. >>>> >>>> - Parent physical device - this might be, for example, a PCIe device >>>> with PASID feature support, hence it is able to tag an unique PASID >>>> for DMA transfers originated from its subset. The device driver hence >>>> is able to wrapper this subset into an isolated: >>>> >>>> - Mediated device - a fake device created by the device driver >>>> mentioned >>>> above. >>>> >>>> Yes. All you mentioned are right for the parent device. But for >>>> mediated >>>> device, iommu_get_domain_for_dev() doesn't work even it has an valid >>>> iommu_group and iommu_domain. >>>> >>>> iommu_get_domain_for_dev() is a necessary interface for device drivers >>>> which want to support aux-domain. For example, >>> >>> Only if they want to follow this very specific notion of using >>> made-up devices and groups to represent aux attachments. Even if a >>> driver managing its own aux domains entirely privately does create >>> child devices for them, it's not like it can't keep its domain >>> pointers in drvdata if it wants to ;) >>> >>> Let's not conflate the current implementation of vfio_mdev with the >>> general concepts involved here. >>> >>>> struct iommu_domain *domain; >>>> struct device *dev = mdev_dev(mdev); >>>> unsigned long pasid; >>>> >>>> domain = iommu_get_domain_for_dev(dev); >>>> if (!domain) >>>> return -ENODEV; >>>> >>>> pasid = iommu_aux_get_pasid(domain, dev->parent); >>>> if (pasid == IOASID_INVALID) >>>> return -EINVAL; >>>> >>>> /* Program the device context with the PASID value */ >>>> .... >>>> >>>> Without this fix, iommu_get_domain_for_dev() always returns NULL and >>>> the >>>> device driver has no means to support aux-domain. >>> >>> So either the IOMMU API itself is missing the ability to do the right >>> thing internally, or the mdev layer isn't using it appropriately. >>> Either way, simply punching holes in the API for mdev to hack around >>> its own mess doesn't seem like the best thing to do. >>> >>> The initial impression I got was that it's implicitly assumed here >>> that the mdev itself is attached to exactly one aux domain and >>> nothing else, at which point I would wonder why it's using aux at >>> all, but are you saying that in fact no attach happens with the mdev >>> group either way, only to the parent device? >>> >>> I'll admit I'm not hugely familiar with any of this, but it seems to >>> me that the logical flow should be: >>> >>> - allocate domain >>> - attach as aux to parent >>> - retrieve aux domain PASID >>> - create mdev child based on PASID >>> - attach mdev to domain (normally) >>> >>> Of course that might require giving the IOMMU API a proper >>> first-class notion of mediated devices, such that it knows the mdev >>> represents the PASID, and can recognise the mdev attach is equivalent >>> to the earlier parent aux attach so not just blindly hand it down to >>> an IOMMU driver that's never heard of this new device before. Or >>> perhaps the IOMMU drivers do their own bookkeeping for the mdev bus, >>> such that they do handle the attach call, and just validate it >>> internally based on the associated parent device and PASID. Either >>> way, the inside maintains self-consistency and from the outside it >>> looks like standard API usage without nasty hacks. >>> >>> I'm pretty sure I've heard suggestions of using mediated devices >>> beyond VFIO (e.g. within the kernel itself), so chances are this is a >>> direction that we'll have to take at some point anyway. >>> >>> And, that said, even if people do want an immediate quick fix >>> regardless of technical debt, I'd still be a lot happier to see >>> iommu_group_set_domain() lightly respun as iommu_attach_mdev() ;) >> >> Get your point and I agree with your concerns. >> >> To maintain the relationship between mdev's iommu_group and >> iommu_domain, how about extending below existing aux_attach api >> >> int iommu_aux_attach_device(struct iommu_domain *domain, >> struct device *dev) >> >> by adding the mdev's iommu_group? >> >> int iommu_aux_attach_device(struct iommu_domain *domain, >> struct device *dev, >> struct iommu_group *group) >> >> And, in iommu_aux_attach_device(), we require, >> - @group only has a single device; >> - @group hasn't been attached by any devices; >> - Set the @domain to @group >> >> Just like what we've done in iommu_attach_device(). >> >> Any thoughts? > > Rather than pass a bare iommu_group with implicit restrictions, it might > be neater to just pass an mdev_device, so that the IOMMU core can also > take care of allocating and setting up the group. Then we flag the group > internally as a special "mdev group" such that we can prevent callers > from subsequently trying to add/remove devices or attach/detach its > domain directly. That seems like it would make a pretty straightforward > and robust API extension, as long as the mdev argument here is optional > so that SVA and other aux users don't have to care. Other than the > slightly different ordering where caller would have to allocate the mdev > first, then finish it's PASID-based configuration afterwards, I guess > it's not far off what I was thinking yesterday :)
It looks good to me if we pass an *optional* made-up device instead of iommu_group. But it seems that vfio/mdev assumes an iommu_group first and then attaches domains to the groups. Hence, it's hard to move the group allocation and setting up into the attach interface.
As proposed, the new iommu_aux_attach_device() might look like this:
int iommu_aux_attach_device(struct iommu_domain *domain, struct device *phys_dev, struct device *dev)
where,
@phys_dev: The physical device which supports IOMMU_DEV_FEAT_AUX; @dev: a made-up device which presents the subset resources binding to the aux-domain. An example use case is vfio/mdev. For cases where no made-up devices are used, pass NULL instead.
With @dev passed, we can require
- single device in group; - no previous attaching; - set up internal logistics between group and domain;
The iommu_aux_detach_device() needs the equivalent extensions.
Best regards, baolu
| |