On Mon, Jun 02, 2025 at 10:50:11AM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
