When device is TSM Bound, some of its MMIO regions are controlled by
secure firmware. E.g. TDX Connect would require these MMIO regions
mappeed in S-EPT and never unmapped until device Unbound. Zapping bars
irrespective of TSM Bound state may cause unexpected secure firmware
errors. It is always safe to do TSM Unbind first, transiting the device
to shared, then do whatever needed as before.
Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
---
drivers/vfio/pci/vfio_pci_config.c | 4 +++
drivers/vfio/pci/vfio_pci_core.c | 41 +++++++++++++++++++-----------
drivers/vfio/pci/vfio_pci_priv.h | 3 +++
3 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 7ac062bd5044..4ffe661c9e59 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -590,6 +590,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
if (!new_mem) {
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
vfio_pci_dma_buf_move(vdev, true);
} else {
@@ -712,6 +713,7 @@ static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
pci_power_t state)
{
if (state >= PCI_D3hot) {
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
vfio_pci_dma_buf_move(vdev, true);
} else {
@@ -907,6 +909,7 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
&cap);
if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
vfio_pci_dma_buf_move(vdev, true);
pci_try_reset_function(vdev->pdev);
@@ -992,6 +995,7 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
&cap);
if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
vfio_pci_dma_buf_move(vdev, true);
pci_try_reset_function(vdev->pdev);
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 92544e54c9c3..a8437fcecca1 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -286,6 +286,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
* The vdev power related flags are protected with 'memory_lock'
* semaphore.
*/
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
vfio_pci_dma_buf_move(vdev, true);
@@ -693,11 +694,7 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
eeh_dev_release(vdev->pdev);
#endif
- if (vdev->is_tsm_bound) {
- vfio_iommufd_tsm_unbind(&vdev->vdev);
- pci_release_regions(vdev->pdev);
- vdev->is_tsm_bound = false;
- }
+ __vfio_pci_tsm_unbind(vdev);
vfio_pci_core_disable(vdev);
@@ -1222,6 +1219,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
if (!vdev->reset_works)
return -EINVAL;
+ vfio_pci_tsm_unbind(vdev);
vfio_pci_zap_and_down_write_memory_lock(vdev);
/*
@@ -1491,12 +1489,32 @@ static int vfio_pci_ioctl_tsm_bind(struct vfio_pci_core_device *vdev,
return ret;
}
+void __vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
+{
+ struct pci_dev *pdev = vdev->pdev;
+
+ lockdep_assert_held(&vdev->vdev.dev_set->lock);
+
+ if (!vdev->is_tsm_bound)
+ return;
+
+ vfio_iommufd_tsm_unbind(&vdev->vdev);
+ pci_release_regions(pdev);
+ vdev->is_tsm_bound = false;
+}
+
+void vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
+{
+ mutex_lock(&vdev->vdev.dev_set->lock);
+ __vfio_pci_tsm_unbind(vdev);
+ mutex_unlock(&vdev->vdev.dev_set->lock);
+}
+
static int vfio_pci_ioctl_tsm_unbind(struct vfio_pci_core_device *vdev,
void __user *arg)
{
unsigned long minsz = offsetofend(struct vfio_pci_tsm_unbind, flags);
struct vfio_pci_tsm_unbind tsm_unbind;
- struct pci_dev *pdev = vdev->pdev;
if (copy_from_user(&tsm_unbind, arg, minsz))
return -EFAULT;
@@ -1504,15 +1522,7 @@ static int vfio_pci_ioctl_tsm_unbind(struct vfio_pci_core_device *vdev,
if (tsm_unbind.argsz < minsz || tsm_unbind.flags)
return -EINVAL;
- mutex_lock(&vdev->vdev.dev_set->lock);
-
- if (!vdev->is_tsm_bound)
- return 0;
-
- vfio_iommufd_tsm_unbind(&vdev->vdev);
- pci_release_regions(pdev);
- vdev->is_tsm_bound = false;
- mutex_unlock(&vdev->vdev.dev_set->lock);
+ vfio_pci_tsm_unbind(vdev);
return 0;
}
@@ -2526,6 +2536,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
break;
}
+ __vfio_pci_tsm_unbind(vdev);
/*
* Take the memory write lock for each device and zap BAR
* mappings to prevent the user accessing the device while in
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 6f3e8eafdc35..e5bf27f46a73 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -130,4 +130,7 @@ static inline void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev,
}
#endif
+void __vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev);
+void vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev);
+
#endif
--
2.25.1Xu Yilun <yilun.xu@linux.intel.com> writes:
> When device is TSM Bound, some of its MMIO regions are controlled by
> secure firmware. E.g. TDX Connect would require these MMIO regions
> mappeed in S-EPT and never unmapped until device Unbound. Zapping bars
> irrespective of TSM Bound state may cause unexpected secure firmware
> errors. It is always safe to do TSM Unbind first, transiting the device
> to shared, then do whatever needed as before.
>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
> drivers/vfio/pci/vfio_pci_config.c | 4 +++
> drivers/vfio/pci/vfio_pci_core.c | 41 +++++++++++++++++++-----------
> drivers/vfio/pci/vfio_pci_priv.h | 3 +++
> 3 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 7ac062bd5044..4ffe661c9e59 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -590,6 +590,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
> new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
>
> if (!new_mem) {
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
>
For a secure device mmio range instead of vfio_pci_zap_and_down_write_memory_lock()
-> unmap_mapping_range() we want the vfio_pci_dma_buf_move right? Also
is that expected to get called twice as below?
vfio_pci_tsm_unbind-> pci_tsm_unbind -> tdx_tsm_unbind ->
tsm_handler->disable_mmio() -> vfio_pci_core_tsm_disable_mmio -> vfio_pci_dma_buf_move(vdev, true);
-aneesh
On Mon, Jun 02, 2025 at 07:30:15PM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@linux.intel.com> writes:
>
> > When device is TSM Bound, some of its MMIO regions are controlled by
> > secure firmware. E.g. TDX Connect would require these MMIO regions
> > mappeed in S-EPT and never unmapped until device Unbound. Zapping bars
> > irrespective of TSM Bound state may cause unexpected secure firmware
> > errors. It is always safe to do TSM Unbind first, transiting the device
> > to shared, then do whatever needed as before.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > ---
> > drivers/vfio/pci/vfio_pci_config.c | 4 +++
> > drivers/vfio/pci/vfio_pci_core.c | 41 +++++++++++++++++++-----------
> > drivers/vfio/pci/vfio_pci_priv.h | 3 +++
> > 3 files changed, 33 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index 7ac062bd5044..4ffe661c9e59 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -590,6 +590,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
> > new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
> >
> > if (!new_mem) {
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> >
>
> For a secure device mmio range instead of vfio_pci_zap_and_down_write_memory_lock()
> -> unmap_mapping_range() we want the vfio_pci_dma_buf_move right?
Yes.
> Also is that expected to get called twice as below?
Yes for TDX Connect. First time zap the private MMIOs during unbind.
Second time block all mmio mapping.
Other platforms don't need these tsm handlers. They don't have this
awkwardness.
Thanks,
Yilun
>
> vfio_pci_tsm_unbind-> pci_tsm_unbind -> tdx_tsm_unbind ->
> tsm_handler->disable_mmio() -> vfio_pci_core_tsm_disable_mmio -> vfio_pci_dma_buf_move(vdev, true);
>
> -aneesh
Xu Yilun <yilun.xu@linux.intel.com> writes:
> When device is TSM Bound, some of its MMIO regions are controlled by
> secure firmware. E.g. TDX Connect would require these MMIO regions
> mappeed in S-EPT and never unmapped until device Unbound. Zapping bars
> irrespective of TSM Bound state may cause unexpected secure firmware
> errors. It is always safe to do TSM Unbind first, transiting the device
> to shared, then do whatever needed as before.
>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
> drivers/vfio/pci/vfio_pci_config.c | 4 +++
> drivers/vfio/pci/vfio_pci_core.c | 41 +++++++++++++++++++-----------
> drivers/vfio/pci/vfio_pci_priv.h | 3 +++
> 3 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 7ac062bd5044..4ffe661c9e59 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -590,6 +590,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
> new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
>
> if (!new_mem) {
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
>
Don't we need to re-bind the vdev with tsm_bind for the continued use of TDI?
> } else {
> @@ -712,6 +713,7 @@ static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
> pci_power_t state)
> {
> if (state >= PCI_D3hot) {
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
> } else {
> @@ -907,6 +909,7 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
> &cap);
>
> if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
> pci_try_reset_function(vdev->pdev);
> @@ -992,6 +995,7 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
> &cap);
>
> if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
> pci_try_reset_function(vdev->pdev);
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 92544e54c9c3..a8437fcecca1 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -286,6 +286,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
> * The vdev power related flags are protected with 'memory_lock'
> * semaphore.
> */
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vfio_pci_dma_buf_move(vdev, true);
>
> @@ -693,11 +694,7 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> eeh_dev_release(vdev->pdev);
> #endif
>
> - if (vdev->is_tsm_bound) {
> - vfio_iommufd_tsm_unbind(&vdev->vdev);
> - pci_release_regions(vdev->pdev);
> - vdev->is_tsm_bound = false;
> - }
> + __vfio_pci_tsm_unbind(vdev);
>
> vfio_pci_core_disable(vdev);
>
> @@ -1222,6 +1219,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> if (!vdev->reset_works)
> return -EINVAL;
>
> + vfio_pci_tsm_unbind(vdev);
> vfio_pci_zap_and_down_write_memory_lock(vdev);
>
> /*
> @@ -1491,12 +1489,32 @@ static int vfio_pci_ioctl_tsm_bind(struct vfio_pci_core_device *vdev,
> return ret;
> }
>
> +void __vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> +
> + lockdep_assert_held(&vdev->vdev.dev_set->lock);
> +
> + if (!vdev->is_tsm_bound)
> + return;
> +
> + vfio_iommufd_tsm_unbind(&vdev->vdev);
> + pci_release_regions(pdev);
> + vdev->is_tsm_bound = false;
>
Do we really need to check vdev->is_tsm_bound? The tsm_ops lock already
ensures that concurrent TSM operations can't happen, and repeated calls
to bind()/unbind() seem to be handled safely by pci_tsm_bind and pci_tsm_unbind.
> +}
> +
> +void vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
> +{
> + mutex_lock(&vdev->vdev.dev_set->lock);
> + __vfio_pci_tsm_unbind(vdev);
> + mutex_unlock(&vdev->vdev.dev_set->lock);
> +}
>
If is_tsm_bound is no longer needed, and pci_release_regions /
request_region_exclusive are now handled within pci_tsm_unbind / bind,
do we still need mutex_lock() to guard this path?
> +
> static int vfio_pci_ioctl_tsm_unbind(struct vfio_pci_core_device *vdev,
> void __user *arg)
> {
> unsigned long minsz = offsetofend(struct vfio_pci_tsm_unbind, flags);
> struct vfio_pci_tsm_unbind tsm_unbind;
> - struct pci_dev *pdev = vdev->pdev;
>
> if (copy_from_user(&tsm_unbind, arg, minsz))
> return -EFAULT;
> @@ -1504,15 +1522,7 @@ static int vfio_pci_ioctl_tsm_unbind(struct vfio_pci_core_device *vdev,
> if (tsm_unbind.argsz < minsz || tsm_unbind.flags)
> return -EINVAL;
>
> - mutex_lock(&vdev->vdev.dev_set->lock);
> -
> - if (!vdev->is_tsm_bound)
> - return 0;
> -
> - vfio_iommufd_tsm_unbind(&vdev->vdev);
> - pci_release_regions(pdev);
> - vdev->is_tsm_bound = false;
> - mutex_unlock(&vdev->vdev.dev_set->lock);
> + vfio_pci_tsm_unbind(vdev);
>
> return 0;
> }
> @@ -2526,6 +2536,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
> break;
> }
>
> + __vfio_pci_tsm_unbind(vdev);
> /*
> * Take the memory write lock for each device and zap BAR
> * mappings to prevent the user accessing the device while in
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> index 6f3e8eafdc35..e5bf27f46a73 100644
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -130,4 +130,7 @@ static inline void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev,
> }
> #endif
>
> +void __vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev);
> +void vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev);
> +
> #endif
> --
> 2.25.1
On Mon, Jun 02, 2025 at 10:50:11AM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@linux.intel.com> writes:
>
> > When device is TSM Bound, some of its MMIO regions are controlled by
> > secure firmware. E.g. TDX Connect would require these MMIO regions
> > mappeed in S-EPT and never unmapped until device Unbound. Zapping bars
> > irrespective of TSM Bound state may cause unexpected secure firmware
> > errors. It is always safe to do TSM Unbind first, transiting the device
> > to shared, then do whatever needed as before.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > ---
> > drivers/vfio/pci/vfio_pci_config.c | 4 +++
> > drivers/vfio/pci/vfio_pci_core.c | 41 +++++++++++++++++++-----------
> > drivers/vfio/pci/vfio_pci_priv.h | 3 +++
> > 3 files changed, 33 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index 7ac062bd5044..4ffe661c9e59 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -590,6 +590,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
> > new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
> >
> > if (!new_mem) {
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> >
>
> Don't we need to re-bind the vdev with tsm_bind for the continued use of TDI?
I choose not to re-bind because host basically cannot recover
everything. The guest does 'bind', 'attest', 'accept' to make a trusted
device, but for this series VFIO is only aware of 'bind' and can only
recover 'bind', which doesn't make much sense. So I think just make
guest fully aware of TDISP rules, guest should expect writing MSE breaks
private state, and should do 'bind', 'attest', 'accept' again for
recovery if it wants to.
>
> > } else {
> > @@ -712,6 +713,7 @@ static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
> > pci_power_t state)
> > {
> > if (state >= PCI_D3hot) {
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> > } else {
> > @@ -907,6 +909,7 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
> > &cap);
> >
> > if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> > pci_try_reset_function(vdev->pdev);
> > @@ -992,6 +995,7 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
> > &cap);
> >
> > if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> > pci_try_reset_function(vdev->pdev);
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index 92544e54c9c3..a8437fcecca1 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -286,6 +286,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
> > * The vdev power related flags are protected with 'memory_lock'
> > * semaphore.
> > */
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> > vfio_pci_dma_buf_move(vdev, true);
> >
> > @@ -693,11 +694,7 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> > eeh_dev_release(vdev->pdev);
> > #endif
> >
> > - if (vdev->is_tsm_bound) {
> > - vfio_iommufd_tsm_unbind(&vdev->vdev);
> > - pci_release_regions(vdev->pdev);
> > - vdev->is_tsm_bound = false;
> > - }
> > + __vfio_pci_tsm_unbind(vdev);
> >
> > vfio_pci_core_disable(vdev);
> >
> > @@ -1222,6 +1219,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> > if (!vdev->reset_works)
> > return -EINVAL;
> >
> > + vfio_pci_tsm_unbind(vdev);
> > vfio_pci_zap_and_down_write_memory_lock(vdev);
> >
> > /*
> > @@ -1491,12 +1489,32 @@ static int vfio_pci_ioctl_tsm_bind(struct vfio_pci_core_device *vdev,
> > return ret;
> > }
> >
> > +void __vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
> > +{
> > + struct pci_dev *pdev = vdev->pdev;
> > +
> > + lockdep_assert_held(&vdev->vdev.dev_set->lock);
> > +
> > + if (!vdev->is_tsm_bound)
> > + return;
> > +
> > + vfio_iommufd_tsm_unbind(&vdev->vdev);
> > + pci_release_regions(pdev);
> > + vdev->is_tsm_bound = false;
> >
>
> Do we really need to check vdev->is_tsm_bound? The tsm_ops lock already
> ensures that concurrent TSM operations can't happen, and repeated calls
> to bind()/unbind() seem to be handled safely by pci_tsm_bind and pci_tsm_unbind.
It is mainly for pci_release_regions(). I remember there is a concern
about whether pci_request/release_region() should be in VFIO driver,
maybe lets solve that concern first in that thread.
>
> > +}
> > +
> > +void vfio_pci_tsm_unbind(struct vfio_pci_core_device *vdev)
> > +{
> > + mutex_lock(&vdev->vdev.dev_set->lock);
> > + __vfio_pci_tsm_unbind(vdev);
> > + mutex_unlock(&vdev->vdev.dev_set->lock);
> > +}
> >
>
> If is_tsm_bound is no longer needed, and pci_release_regions /
> request_region_exclusive are now handled within pci_tsm_unbind / bind,
> do we still need mutex_lock() to guard this path?
We may still need the dev_set->lock. The vfio_pci/iommufd_device_tsm_bind()
not only does pci_tsm_bind(), but also secure IOMMU setup which affects
all devices in the dev_set.
Maybe I worried too much, I doesn't know there exists a real secure device
set.
Thanks,
Yilun
© 2016 - 2025 Red Hat, Inc.