Re: [RFC PATCH 20/30] vfio/pci: Do TSM Unbind before zapping bars

[Xu Yilun <yilun.xu@linux.intel.com>]

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