RE: [PATCH v2] pci/ats: do not allow broken devices to be assigned to guests

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: Friday, February 25, 2022 12:37 AM
> 
> Introduce a new field to mark devices as broken: having it set
> prevents the device from being assigned to guests. Use the field in
> order to mark ATS devices that have failed a flush as broken, thus
> preventing them to be assigned to any guest.
> 
> This allows the device IOMMU context entry to be cleaned up properly,
> as calling _pci_hide_device will just change the ownership of the
> device, but the IOMMU context entry of the device would be left as-is.
> It would also leak a Domain ID, as removing the device from it's
> previous owner will allow releasing the DID used by the device without
> having cleaned up the context entry.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> ---
> Cc: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
> ---
> Changes since v1:
>  - Allow assigning broken devices to dom_io or the hardware domain.
> ---
>  xen/drivers/passthrough/pci.c        | 11 +++++++----
>  xen/drivers/passthrough/vtd/qinval.c |  8 +++++++-
>  xen/include/xen/pci.h                |  3 +++
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 70b6684981..91b43a3f04 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -501,7 +501,7 @@ static void free_pdev(struct pci_seg *pseg, struct
> pci_dev *pdev)
>      xfree(pdev);
>  }
> 
> -static void _pci_hide_device(struct pci_dev *pdev)
> +static void __init _pci_hide_device(struct pci_dev *pdev)
>  {
>      if ( pdev->domain )
>          return;
> @@ -1487,6 +1487,11 @@ static int assign_device(struct domain *d, u16 seg,
> u8 bus, u8 devfn, u32 flag)
>      ASSERT(pdev && (pdev->domain == hardware_domain ||
>                      pdev->domain == dom_io));
> 
> +    /* Do not allow broken devices to be assigned to guests. */
> +    rc = -EBADF;
> +    if ( pdev->broken && d != hardware_domain && d != dom_io )
> +        goto done;
> +
>      rc = pdev_msix_assign(d, pdev);
>      if ( rc )
>          goto done;
> @@ -1585,9 +1590,7 @@ void iommu_dev_iotlb_flush_timeout(struct
> domain *d, struct pci_dev *pdev)
>          return;
>      }
> 
> -    list_del(&pdev->domain_list);
> -    pdev->domain = NULL;
> -    _pci_hide_device(pdev);
> +    pdev->broken = true;
> 
>      if ( !d->is_shutting_down && printk_ratelimit() )
>          printk(XENLOG_ERR "dom%d: ATS device %pp flush failed\n",
> diff --git a/xen/drivers/passthrough/vtd/qinval.c
> b/xen/drivers/passthrough/vtd/qinval.c
> index 9f291f47e5..510961a203 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -227,7 +227,7 @@ static int __must_check dev_invalidate_sync(struct
> vtd_iommu *iommu,
> 
>      ASSERT(iommu->qinval_maddr);
>      rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
> -    if ( rc == -ETIMEDOUT )
> +    if ( rc == -ETIMEDOUT && !pdev->broken )
>      {
>          struct domain *d = rcu_lock_domain_by_id(did_to_domain_id(iommu,
> did));
> 
> @@ -241,6 +241,12 @@ static int __must_check dev_invalidate_sync(struct
> vtd_iommu *iommu,
>          iommu_dev_iotlb_flush_timeout(d, pdev);
>          rcu_unlock_domain(d);
>      }
> +    else if ( rc == -ETIMEDOUT )
> +        /*
> +         * The device is already marked as broken, ignore the error in order 
> to
> +         * allow {de,}assign to succeed.
> +         */
> +        rc = 0;
> 
>      return rc;
>  }
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index b6d7e454f8..02b31f7259 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -108,6 +108,9 @@ struct pci_dev {
>      /* Device with errata, ignore the BARs. */
>      bool ignore_bars;
> 
> +    /* Device misbehaving, prevent assigning it to guests. */
> +    bool broken;
> +
>      enum pdev_type {
>          DEV_TYPE_PCI_UNKNOWN,
>          DEV_TYPE_PCIe_ENDPOINT,
> --
> 2.34.1