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

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

On May 20, 2016 5:59 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 20.05.16 at 09:15, <quan.xu@xxxxxxxxx> wrote:
> > 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).
> >
> >     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..
> 
> Oh, indeed. Yet - can't enable_ats_device() be passed a struct pci_dev *, and
> that be stored instead of SBDF inside struct pci_ats_dev?
> 

Make sense.  I appreciate your specific advice.

Quan

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