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