Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

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

On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 22.04.16 at 12:54, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/vtd/qinval.c
> > +++ b/xen/drivers/passthrough/vtd/qinval.c
> > @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
> >      return 0;
> >  }
> >
> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> > +                                         u16 seg, u8 bus, u8 devfn) {
> > +    struct domain *d = NULL;
> > +    struct pci_dev *pdev;
> > +
> > +    if ( test_bit(did, iommu->domid_bitmap) )
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> > +
> > +    /*
> > +     * In case the domain has been freed or the IOMMU domid bitmap is
> > +     * not valid, the device no longer belongs to this domain.
> > +     */
> > +    if ( d == NULL )
> > +        return;
> > +
> > +    pcidevs_lock();
> > +
> > +    for_each_pdev(d, pdev)
> > +    {
> > +        if ( (pdev->seg == seg) &&
> > +             (pdev->bus == bus) &&
> > +             (pdev->devfn == devfn) )
> > +        {
> > +            ASSERT(pdev->domain);
> > +            list_del(&pdev->domain_list);
> > +            pdev->domain = NULL;
> > +            pci_hide_existing_device(pdev);
> > +            break;
> > +        }
> > +    }
> 
> A loop like this is of course not ideal (especially for Dom0, which may have
> many devices). And I wonder why you, ...
> 
> > @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
> >              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
> >              sbit = 1;
> >              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, 
> > did,
> > +                                          pdev->seg, pdev->bus, 
> > pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> >          case DMA_TLB_PSI_FLUSH:
> >              if ( !device_in_domain(iommu, pdev, did) ) @@ -154,8
> > +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
> >                  addr |= (((u64)1 << (size_order - 1)) - 1) << 
> > PAGE_SHIFT_4K;
> >              }
> >
> > -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> > -                                          sid, sbit, addr);
> > +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, 
> > did,
> > +                                          pdev->seg, pdev->bus, 
> > pdev->devfn,
> > +                                          sbit, addr);
> >              break;
> 
> ... holding pdev in your hands here, don't just pass it down (which at once
> would make the function signature less convoluted: you could even eliminate
> the currently 2nd parameter that way).
> 

Jan, 
    I am afraid we need to leave it as is.. this pdev , in 
dev_invalidate_iotlb(), is 'struct pci_ats_dev',
but we need a 'struct pci_dev' to hide device in dev_invalidate_iotlb_timeout().

'struct pci_ats_dev' and 'struct pci_dev' are quite different, however, SBDF is 
connection between them..

Quan

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