Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain

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

> From: Paul Durrant [mailto:pdurrant@xxxxxxxxxx]
> Sent: Wednesday, November 20, 2019 8:09 PM
> 
> This patch introduces a new iommu_op to facilitate a per-implementation
> quarantine set up, and then further code for x86 implementations
> (amd and vtd) to set up a read/wrote scratch page to serve as the source/
> target for all DMA whilst a device is assigned to dom_io.
> 
> The reason for doing this is that some hardware may continue to re-try
> DMA, despite FLR, in the event of an error. Having a scratch page mapped
> will allow pending DMA to drain and thus quiesce such buggy hardware.

then there is no diagnostics at all since all faults are quiescent now...
why do we want to support such buggy hardware? Is it better to make
it an default-off option since buggy is supposed to niche case?

> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/amd/iommu_map.c       | 57 +++++++++++++++
>  xen/drivers/passthrough/amd/pci_amd_iommu.c   |  9 +--
>  xen/drivers/passthrough/iommu.c               | 25 ++++++-
>  xen/drivers/passthrough/vtd/iommu.c           | 71 +++++++++++++++----
>  xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  2 +
>  xen/include/xen/iommu.h                       |  1 +
>  6 files changed, 143 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> index cd5c7de7c5..8440ccf1c1 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -560,6 +560,63 @@ int
> amd_iommu_reserve_domain_unity_map(struct domain *domain,
>      return rt;
>  }
> 
> +int amd_iommu_quarantine_init(struct domain *d)
> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    unsigned int level;
> +    struct amd_iommu_pte *table;
> +
> +    if ( hd->arch.root_table )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return 0;
> +    }
> +
> +    spin_lock(&hd->arch.mapping_lock);
> +
> +    level = hd->arch.paging_mode;
> +
> +    hd->arch.root_table = alloc_amd_iommu_pgtable();
> +    if ( !hd->arch.root_table )
> +        goto out;
> +
> +    table = __map_domain_page(hd->arch.root_table);
> +    while ( level )
> +    {
> +        struct page_info *pg;
> +        unsigned int i;
> +
> +        /*
> +         * The pgtable allocator is fine for the leaf page, as well as
> +         * page table pages.
> +         */
> +        pg = alloc_amd_iommu_pgtable();
> +        if ( !pg )
> +            break;
> +
> +        for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
> +        {
> +            struct amd_iommu_pte *pde = &table[i];
> +
> +            set_iommu_pde_present(pde, mfn_x(page_to_mfn(pg)), level - 1,
> +                                  true, true);
> +        }
> +
> +        unmap_domain_page(table);
> +        table = __map_domain_page(pg);
> +        level--;
> +    }
> +    unmap_domain_page(table);
> +
> + out:
> +    spin_unlock(&hd->arch.mapping_lock);
> +
> +    amd_iommu_flush_all_pages(d);
> +
> +    /* Pages leaked in failure case */
> +    return level ? -ENOMEM : 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 75a0f1b4ab..c7858b4e8f 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -95,10 +95,6 @@ static void amd_iommu_setup_domain_device(
>      u8 bus = pdev->bus;
>      const struct domain_iommu *hd = dom_iommu(domain);
> 
> -    /* dom_io is used as a sentinel for quarantined devices */
> -    if ( domain == dom_io )
> -        return;
> -
>      BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
>              !iommu->dev_table.buffer );
> 
> @@ -290,10 +286,6 @@ static void
> amd_iommu_disable_domain_device(const struct domain *domain,
>      int req_id;
>      u8 bus = pdev->bus;
> 
> -    /* dom_io is used as a sentinel for quarantined devices */
> -    if ( domain == dom_io )
> -        return;
> -
>      BUG_ON ( iommu->dev_table.buffer == NULL );
>      req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
>      table = iommu->dev_table.buffer;
> @@ -632,6 +624,7 @@ static void amd_dump_p2m_table(struct domain
> *d)
>  static const struct iommu_ops __initconstrel _iommu_ops = {
>      .init = amd_iommu_domain_init,
>      .hwdom_init = amd_iommu_hwdom_init,
> +    .quarantine_init = amd_iommu_quarantine_init,
>      .add_device = amd_iommu_add_device,
>      .remove_device = amd_iommu_remove_device,
>      .assign_device  = amd_iommu_assign_device,
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 8cbe908fff..25283263d7 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -440,6 +440,28 @@ int iommu_iotlb_flush_all(struct domain *d,
> unsigned int flush_flags)
>      return rc;
>  }
> 
> +static int __init iommu_quarantine_init(void)
> +{
> +    const struct domain_iommu *hd = dom_iommu(dom_io);
> +    int rc;
> +
> +    dom_io->options |= XEN_DOMCTL_CDF_iommu;
> +
> +    rc = iommu_domain_init(dom_io, 0);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !hd->platform_ops->quarantine_init )
> +        return 0;
> +
> +    rc = hd->platform_ops->quarantine_init(dom_io);
> +
> +    if ( !rc )
> +        printk("Quarantine initialized\n");
> +
> +    return rc;
> +}
> +
>  int __init iommu_setup(void)
>  {
>      int rc = -ENODEV;
> @@ -473,8 +495,7 @@ int __init iommu_setup(void)
>      }
>      else
>      {
> -        dom_io->options |= XEN_DOMCTL_CDF_iommu;
> -        if ( iommu_domain_init(dom_io, 0) )
> +        if ( iommu_quarantine_init() )
>              panic("Could not set up quarantine\n");
> 
>          printk(" - Dom0 mode: %s\n",
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 25ad649c34..c20f2ca029 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1291,10 +1291,6 @@ int domain_context_mapping_one(
>      int agaw, rc, ret;
>      bool_t flush_dev_iotlb;
> 
> -    /* dom_io is used as a sentinel for quarantined devices */
> -    if ( domain == dom_io )
> -        return 0;
> -
>      ASSERT(pcidevs_locked());
>      spin_lock(&iommu->lock);
>      maddr = bus_to_context_maddr(iommu, bus);
> @@ -1541,10 +1537,6 @@ int domain_context_unmap_one(
>      int iommu_domid, rc, ret;
>      bool_t flush_dev_iotlb;
> 
> -    /* dom_io is used as a sentinel for quarantined devices */
> -    if ( domain == dom_io )
> -        return 0;
> -
>      ASSERT(pcidevs_locked());
>      spin_lock(&iommu->lock);
> 
> @@ -1677,10 +1669,6 @@ static int domain_context_unmap(struct
> domain *domain, u8 devfn,
>          goto out;
>      }
> 
> -    /* dom_io is used as a sentinel for quarantined devices */
> -    if ( domain == dom_io )
> -        goto out;
> -
>      /*
>       * if no other devices under the same iommu owned by this domain,
>       * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
> @@ -2683,9 +2671,68 @@ static void vtd_dump_p2m_table(struct domain
> *d)
>      vtd_dump_p2m_table_level(hd->arch.pgd_maddr, agaw_to_level(hd-
> >arch.agaw), 0, 0);
>  }
> 
> +static int intel_iommu_quarantine_init(struct domain *d)
> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    struct dma_pte *parent;
> +    unsigned int level = agaw_to_level(hd->arch.agaw);
> +    int rc;
> +
> +    if ( hd->arch.pgd_maddr )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return 0;
> +    }
> +
> +    spin_lock(&hd->arch.mapping_lock);
> +
> +    hd->arch.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
> +    if ( !hd->arch.pgd_maddr )
> +        goto out;
> +
> +    parent = (struct dma_pte *)map_vtd_domain_page(hd-
> >arch.pgd_maddr);
> +    while ( level )
> +    {
> +        uint64_t maddr;
> +        unsigned int offset;
> +
> +        /*
> +         * The pgtable allocator is fine for the leaf page, as well as
> +         * page table pages.
> +         */
> +        maddr = alloc_pgtable_maddr(1, hd->node);
> +        if ( !maddr )
> +            break;
> +
> +        for ( offset = 0; offset < PTE_NUM; offset++ )
> +        {
> +            struct dma_pte *pte = &parent[offset];
> +
> +            dma_set_pte_addr(*pte, maddr);
> +            dma_set_pte_readable(*pte);
> +            dma_set_pte_writable(*pte);
> +        }
> +        iommu_flush_cache_page(parent, 1);
> +
> +        unmap_vtd_domain_page(parent);
> +        parent = map_vtd_domain_page(maddr);
> +        level--;
> +    }
> +    unmap_vtd_domain_page(parent);
> +
> + out:
> +    spin_unlock(&hd->arch.mapping_lock);
> +
> +    rc = iommu_flush_iotlb_all(d);
> +
> +    /* Pages leaked in failure case */
> +    return level ? -ENOMEM : rc;
> +}
> +
>  const struct iommu_ops __initconstrel intel_iommu_ops = {
>      .init = intel_iommu_domain_init,
>      .hwdom_init = intel_iommu_hwdom_init,
> +    .quarantine_init = intel_iommu_quarantine_init,
>      .add_device = intel_iommu_add_device,
>      .enable_device = intel_iommu_enable_device,
>      .remove_device = intel_iommu_remove_device,
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> index 8ed9482791..39fb10f567 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -54,6 +54,8 @@ int amd_iommu_init_late(void);
>  int amd_iommu_update_ivrs_mapping_acpi(void);
>  int iov_adjust_irq_affinities(void);
> 
> +int amd_iommu_quarantine_init(struct domain *d);
> +
>  /* mapping functions */
>  int __must_check amd_iommu_map_page(struct domain *d, dfn_t dfn,
>                                      mfn_t mfn, unsigned int flags,
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 974bd3ffe8..6977ddbb97 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -211,6 +211,7 @@ typedef int iommu_grdm_t(xen_pfn_t start,
> xen_ulong_t nr, u32 id, void *ctxt);
>  struct iommu_ops {
>      int (*init)(struct domain *d);
>      void (*hwdom_init)(struct domain *d);
> +    int (*quarantine_init)(struct domain *d);
>      int (*add_device)(u8 devfn, device_t *dev);
>      int (*enable_device)(device_t *dev);
>      int (*remove_device)(u8 devfn, device_t *dev);
> --
> 2.20.1

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel