Re: [Xen-devel] [PATCH v3 2/2] VT-d: Fix vt-d flush timeout issue.

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

> On 15.12.2015 at 5:17pm, <JBeulich@xxxxxxxx> wrote:
> >>> On 15.12.15 at 09:15, <quan.xu@xxxxxxxxx> wrote:
> >> On 14.12.2015 at 5:28pm, <JBeulich@xxxxxxxx> wrote:
> >> >>> On 12.12.15 at 14:21, <quan.xu@xxxxxxxxx> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -1890,6 +1890,9 @@ static int intel_iommu_add_device(u8 devfn, 
> >> > struct
> >> pci_dev *pdev)
> >> >      if ( !pdev->domain )
> >> >          return -EINVAL;
> >> >
> >> > +    if ( is_pdev_unassignable(pdev) )
> >> > +        return -EACCES;
> >>
> >> Is this case possible at all (i.e. a newly added device being 
> >> unassignable)?
> >>
> >
> > IMO, it is possible, and I have checked and printed the invoke flow 
> > when Xen
> init.
> > iommu_setup() / iommu_hwdom_init() are called before 
> > pci_add_device()
> when Xen init.
> > If it flushes error in iommu_setup() / iommu_hwdom_init(), it will 
> > mark device as unassignable.
> 
> Please be more precise: I can't see how e.g. iommu_setup() would 
> manage to time out on invalidation on a particular device. I.e.
> please provide an exact scenario where the flag could get set before 
> the device gets reported through iommu_add_device().
> 
> Furthermore I think that boot time invalidations shouldn't lead to any 
> devices getting marked un-assignable. Instead such would probably 
> better result in the IOMMU to be turned of altogether.


...
construct_dom0()
|--iommu_hwdom_init()
  |--hd->platform_ops->hwdom_init(d) [intel_iommu_hwdom_init() for x86]
    |--iommu_flush_all()
    ...


If it is failed in iommu_flush_all(), it will mask all of dom0's devices as 
unassignable.
And all of that called before pci_add_device() ..   right?


> 
> >> > @@ -88,6 +89,16 @@ struct pci_dev {  #define 
> >> > for_each_pdev(domain,
> >> > pdev) \
> >> >      list_for_each_entry(pdev, &(domain->arch.pdev_list),
> >> > domain_list)
> >> >
> >> > +static inline void mark_pdev_unassignable(struct pci_dev *pdev) {
> >> > +    pdev->info.is_unassignable = 1; }
> >> > +
> >> > +static inline bool_t is_pdev_unassignable(const struct pci_dev
> >> > +*pdev) {
> >> > +    return pdev->info.is_unassignable; }
> >>
> >> Are you aware that we already have a mechanism to prevent 
> >> assignment (via pci_{ro,hide}_device())? I think at the very least 
> >> this check should
> > consider both
> >> variants. Whether fully using the existing mechanism for your 
> >> purpose is feasible I can't immediately tell (since the ownership 
> >> change may be problematic at the points where you want the flagging to 
> >> happen).
> >
> > pci_{ro,hide}_device() is dangerous, and it makes xen crash when I tried it.
> > for this case, IMO I think a flag is a better solution.
> 
> I can't really judge on this without know details of the crash. As 
> said, I'd prefer to have just a single mechanism, and would accept a 
> second one only with proper justification (i.e. more than "is 
> dangerous" or "makes xen crash" - after all that may also be a result of the 
> functions not being used as necessary).
> 


I need do some more investigation and try a second one only with proper 
justification.


Quan



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