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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> On February 11, 2016 at 1:01am, <JBeulich@xxxxxxxx> wrote:
> >>> 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).

> Please be sure to Cc all maintainers of code you modify.
> 
OK, also CCed Dario Faggioli.
Jan, could I re-send out the V5 and cc all maintainers of code I 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.
> 

I will modify it in next version. I tried to align with the coding style, as 
There was similar indentation nearby arm_smmu_iommu_hwdom_init() in that file.

> > --- 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).
> 
I will modify them in next version.

> > @@ -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.)
> 

Could I log an error message and break in this loop?


> > @@ -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.
> 
 I tried to clean up in error handling path chained up. It logs an error 
message,
 When it calls iommu_crash_shutdown() and returns a non-zero value [in patch 
2/7].


> > @@ -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...
> 
> 
IIUC, you refer to the follow:
 flush_iotlb_qi(
{
...
          if ( !cap_caching_mode(iommu->cap) )
            return 1;
...
}


As Kevin mentioned, originally 0/1 is used to indicate whether caller needs to 
flush cache.
But I try to return ZERO in [PATCH v5 1/7]. Because:
1) if It returns _1_, device passthrough doesn't work when I tried to clean up 
in error handling path chained up. 
2) as VT-d spec said, Caching Mode is 0: invalidations are not required for 
modifications to individual not present
  or invalid entries.

That is tricky for this point. Correct me if I am wrong.


> > @@ -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.
> 

I will modify it in next version.

> > --- 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.
> 

Agreed.


Jan, thanks for your comments.
BTW, just for a check, did you only review patch v5 1/7 ? 
Hope I didn't miss your other comments.

Quan

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