On 1/13/2021 2:26 PM, Alex Williamson wrote:
> On Mon, 11 Jan 2021 16:12:18 -0500
> Steven Sistare <steven.sistare@xxxxxxxxxx> wrote:
>
>> On 1/8/2021 2:39 PM, Alex Williamson wrote:
>>> On Tue, 5 Jan 2021 07:36:51 -0800
>>> Steve Sistare <steven.sistare@xxxxxxxxxx> wrote:
>>>
>>>> Add a function that detects if an iommu_group has a valid container.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
>>>> ---
>>>> drivers/vfio/vfio.c | 12 ++++++++++++
>>>> include/linux/vfio.h | 1 +
>>>> 2 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>>> index 262ab0e..f89ab80 100644
>>>> --- a/drivers/vfio/vfio.c
>>>> +++ b/drivers/vfio/vfio.c
>>>> @@ -61,6 +61,7 @@ struct vfio_container {
>>>> struct vfio_iommu_driver *iommu_driver;
>>>> void *iommu_data;
>>>> bool noiommu;
>>>> + bool closed;
>>>> };
>>>>
>>>> struct vfio_unbound_dev {
>>>> @@ -1223,6 +1224,7 @@ static int vfio_fops_release(struct inode *inode, struct file *filep)
>>>>
>>>> filep->private_data = NULL;
>>>>
>>>> + container->closed = true;
>>>> vfio_container_put(container);
>>>>
>>>> return 0;
>>>> @@ -2216,6 +2218,16 @@ void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
>>>> }
>>>> EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
>>>>
>>>> +bool vfio_iommu_group_contained(struct iommu_group *iommu_group)
>>>> +{
>>>> + struct vfio_group *group = vfio_group_get_from_iommu(iommu_group);
>>>> + bool ret = group && group->container && !group->container->closed;
>>>> +
>>>> + vfio_group_put(group);
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(vfio_iommu_group_contained);
>>>
>>> This seems like a pointless interface, the result is immediately stale.
>>> Anything that relies on the container staying open needs to add itself
>>> as a user. We already have some interfaces for that, ex.
>>> vfio_group_get_external_user(). Thanks,
>>
>> The significant part here is container->closed, which is only set if userland closes the
>> container fd, which is a one-way trip -- the fd for this instance can never be opened
>> again. The container object may still have other references, and not be destroyed yet.
>> In patch 5, kernel code that waits for the RESUME ioctl calls this accessor to test if
>> the ioctl will never arrive.
>
> Ok, that makes sense, the "contained" naming notsomuch. You mention on
> 5/5 that you considered defining a new backend interface to call from
> the core, but considered it overkill. I'm not sure what you're
> imagining that would be overkill. We can pretty simply add an optional
> callback to struct vfio_iommu_drivers_ops that would allow the iommu
> driver to be notified when the container fd is released. That seems
> quite simple and avoids the inverted calling structure presented here.
> Thanks,
OK.
Thanks very much for all your comments. Off to write PATCH V2 ...
- Steve
