Re: [Xen-devel] [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue

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

On March 17, 2016 7:14pm, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: Thursday, March 17, 2016 5:43 PM
> >
> > >>> On 17.03.16 at 09:17, <kevin.tian@xxxxxxxxx> wrote:
> > >>  From: Xu, Quan
> > >> Sent: Thursday, March 17, 2016 3:13 PM
> > >> --- a/xen/drivers/passthrough/vtd/qinval.c
> > >> +++ b/xen/drivers/passthrough/vtd/qinval.c
> > >> @@ -233,6 +233,57 @@ int qinval_device_iotlb(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]);
> > >> +
> > >> +    if ( d == NULL )
> > >> +        return;
> > >> +
> > >> +    pcidevs_lock();
> > >> +    for_each_pdev(d, pdev)
> > >
> > > we need a 'safe' version here since you're deleting nodes when
> > > walking list. for_each_pdev today is based on list_for_each_entry.
> > > Or if it's sure that only one pdev can match, we can break out of
> > > the loop to do removal.
> >
> > But breaking out of the loop is what is already being done ...
> >
> > >> +    {
> > >> +        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;
> >
> > ... here.
> >
> 
> however you see list_del happens before breaking out, right?
> 
For these version of macros:
  a). list_for_each_entry - iterate over list of given type
  b). list_for_each_entry_safe - iterate over list of given type safe against 
removal of list entry

__iiuc__, I think the key point is:
  -in general, we'd better remove entry under list_for_each_entry_safe. Yes, it 
is correct to use list_for_each_entry_safe for this point too.
  - If we delete the iterate from list_for_each_entry, it may point to 
undefined state, leading to xen panic.
   but the pdev->domain_list is not a point ( it is a "struct list_head 
domain_list" variable), so it is safe to
   delete it under list_for_each_entry in this case. 

   Jan, is it similar to yours?

Quan

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