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

From: "Jan Beulich" <JBeulich@suse.com>
To: Quan Xu <quan.xu@intel.com>
Cc: "dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	Feng Wu <feng.wu@intel.com>, Kevin Tian <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue
Date: Fri, 20 May 2016 03:58:54 -0600	[thread overview]
Message-ID: <573EFBFE02000078000ED2CA@prv-mh.provo.novell.com> (raw)
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B8B3410@SHSMSX101.ccr.corp.intel.com>

>>> On 20.05.16 at 09:15, <quan.xu@intel.com> wrote:
> On May 17, 2016 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 22.04.16 at 12:54, <quan.xu@intel.com> 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?

Jan

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