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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> > @@ -262,6 +308,14 @@ static int __iommu_flush_iec(struct iommu *iommu,
>> > u8 granu, u8 im, u16 iidx)
>> >
>> >      queue_invalidate_iec(iommu, granu, im, iidx);
>> >      ret = invalidate_sync(iommu);
>> > +
>> > +    if ( ret == -ETIMEDOUT )
>> > +    {
>> > +        invalidate_timeout(iommu);
>> > +        dprintk(XENLOG_WARNING VTDPREFIX,
>> > +                "IEC flush timeout.\n");
>> > +        return ret;
>> > +    }
>> >      /*
>> 
>> Considering the recurring pattern, wouldn't it be better for
>> invalidate_sync() to invoke invalidate_timeout() (at once making sure no 
>> current
>> or future caller misses the need to do so)?
> 
> Now invalidate_sync() is a common function for iec/iotlb/context/device-tlb 
> invalidation.
> It is different to deal with the flush error.
> For device-tlb, it needs some parameters of device, such device 
> seg/bus/devfn.
> But iec/iotlb/context don't need these parameters.

Ah, I see, there's indeed one case where the handling is different.
Never mind then.

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

Jan


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