| Message ID | 1609861013-129801-4-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio virtual address update | expand |
On Tue, 5 Jan 2021 07:36:51 -0800 Steve Sistare <steven.sistare@oracle.com> wrote: > Add a function that detects if an iommu_group has a valid container. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > 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, Alex > + > static int vfio_register_group_notifier(struct vfio_group *group, > unsigned long *events, > struct notifier_block *nb) > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 38d3c6a..b2724e7 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -148,6 +148,7 @@ extern int vfio_unregister_notifier(struct device *dev, > > struct kvm; > extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm); > +extern bool vfio_iommu_group_contained(struct iommu_group *group); > > /* > * Sub-module helpers
On 1/8/2021 2:39 PM, Alex Williamson wrote: > On Tue, 5 Jan 2021 07:36:51 -0800 > Steve Sistare <steven.sistare@oracle.com> wrote: > >> Add a function that detects if an iommu_group has a valid container. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> 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. - Steve >> + >> static int vfio_register_group_notifier(struct vfio_group *group, >> unsigned long *events, >> struct notifier_block *nb) >> diff --git a/include/linux/vfio.h b/include/linux/vfio.h >> index 38d3c6a..b2724e7 100644 >> --- a/include/linux/vfio.h >> +++ b/include/linux/vfio.h >> @@ -148,6 +148,7 @@ extern int vfio_unregister_notifier(struct device *dev, >> >> struct kvm; >> extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm); >> +extern bool vfio_iommu_group_contained(struct iommu_group *group); >> >> /* >> * Sub-module helpers >
On Mon, 11 Jan 2021 16:12:18 -0500 Steven Sistare <steven.sistare@oracle.com> wrote: > On 1/8/2021 2:39 PM, Alex Williamson wrote: > > On Tue, 5 Jan 2021 07:36:51 -0800 > > Steve Sistare <steven.sistare@oracle.com> wrote: > > > >> Add a function that detects if an iommu_group has a valid container. > >> > >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > >> --- > >> 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, Alex
On 1/13/2021 2:26 PM, Alex Williamson wrote: > On Mon, 11 Jan 2021 16:12:18 -0500 > Steven Sistare <steven.sistare@oracle.com> wrote: > >> On 1/8/2021 2:39 PM, Alex Williamson wrote: >>> On Tue, 5 Jan 2021 07:36:51 -0800 >>> Steve Sistare <steven.sistare@oracle.com> wrote: >>> >>>> Add a function that detects if an iommu_group has a valid container. >>>> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> 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
On Wed, 13 Jan 2021 12:26:09 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 11 Jan 2021 16:12:18 -0500 > Steven Sistare <steven.sistare@oracle.com> wrote: > > > On 1/8/2021 2:39 PM, Alex Williamson wrote: > > > On Tue, 5 Jan 2021 07:36:51 -0800 > > > Steve Sistare <steven.sistare@oracle.com> wrote: > > > > > >> Add a function that detects if an iommu_group has a valid container. > > >> > > >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > >> --- > > >> 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. A callback into the iommu backend would also allow you to use a waitqueue and wake_up for the blocking mechanism while vaddr is invalid. Seems like an improvement over the polling model. Thanks, Alex
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); + static int vfio_register_group_notifier(struct vfio_group *group, unsigned long *events, struct notifier_block *nb) diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 38d3c6a..b2724e7 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -148,6 +148,7 @@ extern int vfio_unregister_notifier(struct device *dev, struct kvm; extern void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm); +extern bool vfio_iommu_group_contained(struct iommu_group *group); /* * Sub-module helpers
Add a function that detects if an iommu_group has a valid container. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- drivers/vfio/vfio.c | 12 ++++++++++++ include/linux/vfio.h | 1 + 2 files changed, 13 insertions(+)