On Tue, Aug 29, 2023 at 11:19:42PM +0000, Volodymyr Babchuk wrote:
> Add per-domain d->pci_lock that protects access to
> d->pdev_list. Purpose of this lock is to give guarantees to VPCI code
> that underlying pdev will not disappear under feet. This is a rw-lock,
> but this patch adds only write_lock()s. There will be read_lock()
> users in the next patches.
>
> This lock should be taken in write mode every time d->pdev_list is
> altered. This covers both accesses to d->pdev_list and accesses to
> pdev->domain_list fields.
Why do you mention pdev->domain_list here? I don't think the lock
covers accesses to pdev->domain_list, unless that domain_list field
happens to be part of the linked list in d->pdev_list. I find it kind
of odd to mention here.
> All write accesses also should be protected
> by pcidevs_lock() as well. Idea is that any user that wants read
> access to the list or to the devices stored in the list should use
> either this new d->pci_lock or old pcidevs_lock(). Usage of any of
> this two locks will ensure only that pdev of interest will not
> disappear from under feet and that the pdev still will be assigned to
> the same domain. Of course, any new users should use pcidevs_lock()
> when it is appropriate (e.g. when accessing any other state that is
> protected by the said lock). In case both the newly introduced
> per-domain rwlock and the pcidevs lock is taken, the later must be
> acquired first.
>
> Any write access to pdev->domain_list should be protected by both
> pcidevs_lock() and d->pci_lock in the write mode.
>
> Suggested-by: Roger Pau Monné <roger....@citrix.com>
> Suggested-by: Jan Beulich <jbeul...@suse.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>
> ---
>
> Changes in v9:
> - returned back "pdev->domain = target;" in AMD IOMMU code
> - used "source" instead of pdev->domain in IOMMU functions
> - added comment about lock ordering in the commit message
> - reduced locked regions
> - minor changes non-functional changes in various places
>
> Changes in v8:
> - New patch
>
> Changes in v8 vs RFC:
> - Removed all read_locks after discussion with Roger in #xendevel
> - pci_release_devices() now returns the first error code
> - extended commit message
> - added missing lock in pci_remove_device()
> - extended locked region in pci_add_device() to protect list_del() calls
> ---
> xen/common/domain.c | 1 +
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 9 ++-
> xen/drivers/passthrough/pci.c | 71 +++++++++++++++++----
> xen/drivers/passthrough/vtd/iommu.c | 9 ++-
> xen/include/xen/sched.h | 1 +
> 5 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 304aa04fa6..9b04a20160 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -651,6 +651,7 @@ struct domain *domain_create(domid_t domid,
>
> #ifdef CONFIG_HAS_PCI
> INIT_LIST_HEAD(&d->pdev_list);
> + rwlock_init(&d->pci_lock);
> #endif
>
> /* All error paths can depend on the above setup. */
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index bea70db4b7..d219bd9453 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -476,7 +476,14 @@ static int cf_check reassign_device(
>
> if ( devfn == pdev->devfn && pdev->domain != target )
> {
> - list_move(&pdev->domain_list, &target->pdev_list);
> + write_lock(&source->pci_lock);
> + list_del(&pdev->domain_list);
> + write_unlock(&source->pci_lock);
> +
> + write_lock(&target->pci_lock);
> + list_add(&pdev->domain_list, &target->pdev_list);
> + write_unlock(&target->pci_lock);
> +
> pdev->domain = target;
While I don't think this is strictly an issue right now, it would be
better to set pdev->domain before the device is added to domain_list.
A pattern like:
read_lock(d->pci_lock);
for_each_pdev(d, pdev)
foo(pdev->domain);
read_unlock(d->pci_lock);
Wouldn't work currently if the pdev is added to domain_list before the
pdev->domain field is updated to reflect the new owner.
> }
>
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 33452791a8..79ca928672 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -454,7 +454,9 @@ static void __init _pci_hide_device(struct pci_dev *pdev)
> if ( pdev->domain )
> return;
> pdev->domain = dom_xen;
> + write_lock(&dom_xen->pci_lock);
> list_add(&pdev->domain_list, &dom_xen->pdev_list);
> + write_unlock(&dom_xen->pci_lock);
> }
>
> int __init pci_hide_device(unsigned int seg, unsigned int bus,
> @@ -748,7 +750,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> if ( !pdev->domain )
> {
> pdev->domain = hardware_domain;
> + write_lock(&hardware_domain->pci_lock);
> list_add(&pdev->domain_list, &hardware_domain->pdev_list);
> + write_unlock(&hardware_domain->pci_lock);
>
> /*
> * For devices not discovered by Xen during boot, add vPCI handlers
> @@ -758,7 +762,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> if ( ret )
> {
> printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> + write_lock(&hardware_domain->pci_lock);
> list_del(&pdev->domain_list);
> + write_unlock(&hardware_domain->pci_lock);
> pdev->domain = NULL;
> goto out;
> }
> @@ -766,7 +772,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> if ( ret )
> {
> vpci_remove_device(pdev);
> + write_lock(&hardware_domain->pci_lock);
> list_del(&pdev->domain_list);
> + write_unlock(&hardware_domain->pci_lock);
> pdev->domain = NULL;
> goto out;
> }
> @@ -816,7 +824,11 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> pci_cleanup_msi(pdev);
> ret = iommu_remove_device(pdev);
> if ( pdev->domain )
> + {
> + write_lock(&pdev->domain->pci_lock);
> list_del(&pdev->domain_list);
> + write_unlock(&pdev->domain->pci_lock);
> + }
> printk(XENLOG_DEBUG "PCI remove device %pp\n", &pdev->sbdf);
> free_pdev(pseg, pdev);
> break;
> @@ -887,26 +899,61 @@ static int deassign_device(struct domain *d, uint16_t
> seg, uint8_t bus,
>
> int pci_release_devices(struct domain *d)
> {
> - struct pci_dev *pdev, *tmp;
> - u8 bus, devfn;
> - int ret;
> + int combined_ret;
> + LIST_HEAD(failed_pdevs);
>
> pcidevs_lock();
> - ret = arch_pci_clean_pirqs(d);
> - if ( ret )
> +
> + combined_ret = arch_pci_clean_pirqs(d);
> + if ( combined_ret )
> {
> pcidevs_unlock();
> - return ret;
> + return combined_ret;
> }
> - list_for_each_entry_safe ( pdev, tmp, &d->pdev_list, domain_list )
> +
> + write_lock(&d->pci_lock);
> +
> + while ( !list_empty(&d->pdev_list) )
> {
> - bus = pdev->bus;
> - devfn = pdev->devfn;
> - ret = deassign_device(d, pdev->seg, bus, devfn) ?: ret;
> + struct pci_dev *pdev = list_first_entry(&d->pdev_list,
> + struct pci_dev,
> + domain_list);
> + uint16_t seg = pdev->seg;
> + uint8_t bus = pdev->bus;
> + uint8_t devfn = pdev->devfn;
> + int ret;
> +
> + write_unlock(&d->pci_lock);
> + ret = deassign_device(d, seg, bus, devfn);
> + write_lock(&d->pci_lock);
> + if ( ret )
> + {
> + const struct pci_dev *tmp;
> +
> + /*
> + * We need to check if deassign_device() left our pdev in
> + * domain's list. As we dropped the lock, we can't be sure
> + * that list wasn't permutated in some random way, so we
> + * need to traverse the whole list.
> + */
> + for_each_pdev ( d, tmp )
> + {
> + if ( tmp == pdev )
> + {
> + list_move_tail(&pdev->domain_list, &failed_pdevs);
> + break;
> + }
> + }
> +
> + combined_ret = combined_ret ?: ret;
> + }
> }
> +
> + list_splice(&failed_pdevs, &d->pdev_list);
> + write_unlock(&d->pci_lock);
> pcidevs_unlock();
>
> - return ret;
> + return combined_ret;
> }
>
> #define PCI_CLASS_BRIDGE_HOST 0x0600
> @@ -1125,7 +1172,9 @@ static int __hwdom_init cf_check
> _setup_hwdom_pci_devices(
> if ( !pdev->domain )
> {
> pdev->domain = ctxt->d;
> + write_lock(&ctxt->d->pci_lock);
> list_add(&pdev->domain_list, &ctxt->d->pdev_list);
> + write_unlock(&ctxt->d->pci_lock);
> setup_one_hwdom_device(ctxt, pdev);
> }
> else if ( pdev->domain == dom_xen )
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 0e3062c820..3228900c97 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2806,7 +2806,14 @@ static int cf_check reassign_device_ownership(
>
> if ( devfn == pdev->devfn && pdev->domain != target )
> {
> - list_move(&pdev->domain_list, &target->pdev_list);
> + write_lock(&source->pci_lock);
> + list_del(&pdev->domain_list);
> + write_unlock(&source->pci_lock);
> +
> + write_lock(&target->pci_lock);
> + list_add(&pdev->domain_list, &target->pdev_list);
> + write_unlock(&target->pci_lock);
> +
> pdev->domain = target;
Same comment as in reassign_device() above.
Thanks, Roger.
