[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH 3/9] VT-d: undo device mappings upon error


  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Tian, Kevin" <kevin.tian@xxxxxxxxx>
  • Date: Thu, 24 Jun 2021 05:13:43 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=nnibFZVDAsaR0mBMP5cYkF7RspspIk3voJFRu8ArpaA=; b=TS/aznQ16hVF+S1gRFSjkqut6G7VR4OkaoNX81lDbxzok1y96maYa+uDYTBeCeH1eR9QNLd8yRb4we9qlKxvVflyJy4brheAsWc0S0dtkGWL28xdTWr0+6Bv5DJNqtzkDgChGZPgTovwNy3AzlV+do7mMKNmn8/1MswtPs6KXnZKZto1L62on7wUmDdRFrDI/dAXuDxNFH16d+D5+uMNGlZWnW+Rs5Phh/r+VwPCgt6oOKdRRmTYbxG1VXqizCUPfyv10O46WDTKnqL6i8hppr3ZYWGEWxlhCkOAfQZ5Sl4l11XimxYgb54IaK0/qsHi86QgEUPM5p5CZIVn5ydVFA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=knCwhuPgN7S6UEZhbm653ztH7VBit88WdBg04Btsnw7odeEARw6Y14Z5st93bMdDUfMKtpZQvUg6s+xa06ODf9ZcEp11cSShbrNsKDFQhLHSzXVXh4nvVA0oOtil8p13PlRY3hV61U5TAFEgZQIP9jxAd8oFy1HrDys79gipt4NU5WjzsjufoqJqWilMDaJd6HvCjOTKPIJLM4Z11M+UA1DufgMHB1xHCkLOyFyldYPKNDGPRYsj1zocwz5nCmCo6HQvVyctacsnk6JYxXzDTOnkGvQg2m+V79yKu/Wm+xMWKLJcpysVTOR2yFrPKD9KgFlQUHcKAr6hxg5SQ0Ciig==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=intel.com;
  • Cc: Paul Durrant <paul@xxxxxxx>, "Cooper, Andrew" <andrew.cooper3@xxxxxxxxxx>
  • Delivery-date: Thu, 24 Jun 2021 05:14:06 +0000
  • Dlp-product: dlpe-windows
  • Dlp-reaction: no-action
  • Dlp-version: 11.5.1.3
  • Ironport-sdr: 8mPYyQ74HM+yL5bewgh6f/7e/56jfgWsT9v99+d44cioUwhCXR6cXsgIl0OvBd/mOfNKbhQlyO lrCkclBvVhHg==
  • Ironport-sdr: KWHijnh6cYZqVx2vxuZFQ5q020d5iWIQzVBaHUQ8+Cpoz3te9UMh6KaDi68fdN6ETEPBZCc2Kl iHEEJyJGH0Cg==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHXXRG+XpEHT4VRdkm+LQYGIgowYqsitShQ
  • Thread-topic: [PATCH 3/9] VT-d: undo device mappings upon error

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, June 9, 2021 5:28 PM
> 
> When
>  - flushes (supposedly not possible anymore after XSA-373),
>  - secondary mappings for legacy PCI devices behind bridges,
>  - secondary mappings for chipset quirks, or
>  - find_upstream_bridge() invocations
> fail, the successfully established device mappings should not be left
> around.
> 
> Further, when (parts of) unmapping fail, simply returning an error is
> typically not enough. Crash the domain instead in such cases, arranging
> for domain cleanup to continue in a best effort manner despite such
> failures.
> 
> Finally make domain_context_unmap()'s error behavior consistent in the
> legacy PCI device case: Don't bail from the function in one special
> case, but always just exit the switch statement.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Paul Durrant <paul@xxxxxxx>

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

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1442,9 +1442,15 @@ int domain_context_mapping_one(
>      if ( !seg && !rc )
>          rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> 
> +    if ( rc )
> +        domain_context_unmap_one(domain, iommu, bus, devfn);
> +
>      return rc;
>  }
> 
> +static int domain_context_unmap(struct domain *d, uint8_t devfn,
> +                                struct pci_dev *pdev);
> +
>  static int domain_context_mapping(struct domain *domain, u8 devfn,
>                                    struct pci_dev *pdev)
>  {
> @@ -1505,16 +1511,21 @@ static int domain_context_mapping(struct
>          if ( ret )
>              break;
> 
> -        if ( find_upstream_bridge(seg, &bus, &devfn, &secbus) < 1 )
> -            break;
> +        if ( (ret = find_upstream_bridge(seg, &bus, &devfn, &secbus)) < 1 )
> +        {
> +            if ( !ret )
> +                break;
> +            ret = -ENXIO;
> +        }
> 
>          /*
>           * Mapping a bridge should, if anything, pass the struct pci_dev of
>           * that bridge. Since bridges don't normally get assigned to guests,
>           * their owner would be the wrong one. Pass NULL instead.
>           */
> -        ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
> -                                         NULL);
> +        if ( ret >= 0 )
> +            ret = domain_context_mapping_one(domain, drhd->iommu, bus,
> devfn,
> +                                             NULL);
> 
>          /*
>           * Devices behind PCIe-to-PCI/PCIx bridge may generate different
> @@ -1531,6 +1542,9 @@ static int domain_context_mapping(struct
>              ret = domain_context_mapping_one(domain, drhd->iommu, secbus,
> 0,
>                                               NULL);
> 
> +        if ( ret )
> +            domain_context_unmap(domain, devfn, pdev);
> +
>          break;
> 
>      default:
> @@ -1609,6 +1623,19 @@ int domain_context_unmap_one(
>      if ( !iommu->drhd->segment && !rc )
>          rc = me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
> 
> +    if ( rc && !is_hardware_domain(domain) && domain != dom_io )
> +    {
> +        if ( domain->is_dying )
> +        {
> +            printk(XENLOG_ERR "%pd: error %d
> unmapping %04x:%02x:%02x.%u\n",
> +                   domain, rc, iommu->drhd->segment, bus,
> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
> +            rc = 0; /* Make upper layers continue in a best effort manner. */
> +        }
> +        else
> +            domain_crash(domain);
> +    }
> +
>      return rc;
>  }
> 
> @@ -1661,17 +1688,29 @@ static int domain_context_unmap(struct d
> 
>          tmp_bus = bus;
>          tmp_devfn = devfn;
> -        if ( find_upstream_bridge(seg, &tmp_bus, &tmp_devfn, &secbus) < 1 )
> +        if ( (ret = find_upstream_bridge(seg, &tmp_bus, &tmp_devfn,
> +                                         &secbus)) < 1 )
> +        {
> +            if ( ret )
> +            {
> +                ret = -ENXIO;
> +                if ( !domain->is_dying &&
> +                     !is_hardware_domain(domain) && domain != dom_io )
> +                {
> +                    domain_crash(domain);
> +                    /* Make upper layers continue in a best effort manner. */
> +                    ret = 0;
> +                }
> +            }
>              break;
> +        }
> 
>          /* PCIe to PCI/PCIx bridge */
>          if ( pdev_type(seg, tmp_bus, tmp_devfn) ==
> DEV_TYPE_PCIe2PCI_BRIDGE )
>          {
>              ret = domain_context_unmap_one(domain, iommu, tmp_bus,
> tmp_devfn);
> -            if ( ret )
> -                return ret;
> -
> -            ret = domain_context_unmap_one(domain, iommu, secbus, 0);
> +            if ( !ret )
> +                ret = domain_context_unmap_one(domain, iommu, secbus, 0);
>          }
>          else /* Legacy PCI bridge */
>              ret = domain_context_unmap_one(domain, iommu, tmp_bus,
> tmp_devfn);


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.