Re: [Xen-devel] [PATCH v5 1/7] VT-d: Check VT-d Device-TLB flush error(IOMMU part).

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
> This patch checks all kinds of error and all the way up
> the call trees of VT-d Device-TLB flush(IOMMU part).
> 
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/amd/iommu_init.c      |   4 +-
>  xen/drivers/passthrough/amd/pci_amd_iommu.c   |   4 +-
>  xen/drivers/passthrough/arm/smmu.c            |  13 +--
>  xen/drivers/passthrough/iommu.c               |  37 +++++---
>  xen/drivers/passthrough/vtd/extern.h          |   4 +-
>  xen/drivers/passthrough/vtd/iommu.c           | 125 
> ++++++++++++++++----------
>  xen/drivers/passthrough/vtd/qinval.c          |   2 +-
>  xen/drivers/passthrough/vtd/quirks.c          |  26 +++---
>  xen/drivers/passthrough/vtd/x86/vtd.c         |  17 +++-
>  xen/drivers/passthrough/x86/iommu.c           |   6 +-
>  xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |   4 +-
>  xen/include/asm-x86/iommu.h                   |   2 +-
>  xen/include/xen/iommu.h                       |  20 ++---
>  13 files changed, 166 insertions(+), 98 deletions(-)

Please be sure to Cc all maintainers of code you modify.

> @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
>       return 0;
>  }
>  
> -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>  {
> +        return 0;
>  }

Bad indentation.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -146,14 +146,15 @@ static void __hwdom_init check_hwdom_reqs(struct domain 
> *d)
>      iommu_dom0_strict = 1;
>  }
>  
> -void __hwdom_init iommu_hwdom_init(struct domain *d)
> +int __hwdom_init iommu_hwdom_init(struct domain *d)
>  {
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
> +    int rc = 0;

Pointless initializer (more elsewhere).

> @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>                   ((page->u.inuse.type_info & PGT_type_mask)
>                    == PGT_writable_page) )
>                  mapping |= IOMMUF_writable;
> -            hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +            rc = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +            if ( rc )
> +                return rc;

So in this patch I can't find error checking getting introduced for
the caller of this function. And if you were to add it - what would
your intentions be? Panic? IOW I'm not sure bailing here is a good
idea. Logging an error message (but only once in this loop) would
be a possibility. (This pattern repeats further down.)

> @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d)
>          ops->share_p2m(d);
>  }
>  
> -void iommu_crash_shutdown(void)
> +int iommu_crash_shutdown(void)
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
> +
>      if ( iommu_enabled )
> -        ops->crash_shutdown();
> +        return ops->crash_shutdown();
> +
>      iommu_enabled = iommu_intremap = 0;
> +
> +    return 0;
>  }

Here again the question is - what is the error value going to be used
for? We're trying to shut down a crashed system when coming here.

> @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct domain *d, 
> unsigned long gfn,
>              continue;
>  
>          if ( page_count > 1 || gfn == -1 )
> -        {
> -            if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
> -                        0, flush_dev_iotlb) )
> -                iommu_flush_write_buffer(iommu);
> -        }
> +            rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
> +                        0, flush_dev_iotlb);
>          else
> -        {
> -            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
> +            rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
>                          (paddr_t)gfn << PAGE_SHIFT_4K, 0,
> -                        !dma_old_pte_present, flush_dev_iotlb) )
> -                iommu_flush_write_buffer(iommu);
> -        }
> +                        !dma_old_pte_present, flush_dev_iotlb);
> +
> +        if ( rc )
> +            iommu_flush_write_buffer(iommu);
>      }
> +
> +    return rc;
>  }

rc may be positive here (and afaict that's the indication to do the
flush, not one of failure). Quite likely this code didn't mean to flush
iommu_flush_iotlb_{d,p}si() returning a negative value...


> @@ -1742,7 +1757,9 @@ static int intel_iommu_map_page(
>      unmap_vtd_domain_page(page);
>  
>      if ( !this_cpu(iommu_dont_flush_iotlb) )
> -        __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
> +    {
> +        return __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
> +    }

Pointless introduction of braces.

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct domain *d)
>      this_cpu(iommu_dont_flush_iotlb) = 0;
>  
>      if ( !rc )
> -        iommu_iotlb_flush_all(d);
> +    {
> +        rc = iommu_iotlb_flush_all(d);
> +        if ( rc )
> +            return rc;
> +    }

But you leave all page tables set up?

>      else if ( rc != -ERESTART )
>          iommu_teardown(d);

I think you should let things reach this tear down in that case.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel