Re: [PATCH v9 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue

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

On April 05, 2016 5:48pm, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 01.04.16 at 16:47, <quan.xu@intel.com> wrote:
> > +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;
> 
> This should be accompanied by a comment explaining why returning here
> without indicating any error is okay.
> 

I'd say:
"The domain has been freed, and the device no longer belongs to this domain."



> > +        {
> > +            ASSERT(pdev->domain);
> > +            list_del(&pdev->domain_list);
> > +            pdev->domain = NULL;
> > +            pci_hide_existing_device(pdev);
> > +            break;
> > +        }
> > +    }
> > +
> > +    pcidevs_unlock();
> > +
> > +    if ( !is_hardware_domain(d) )
> > +        domain_crash(d);
> 
> else printk();
> 

As I mentioned in commit log "If impacted domain is hardware domain, just throw out a warning (done in queue_invalidate_wait).",
I am not sure whether we really need a printk() at this point or not.

> > @@ -251,11 +303,13 @@ int qinval_device_iotlb(struct iommu *iommu,  }
> >
> >  int qinval_device_iotlb_sync(struct iommu *iommu,
> > -    u32 max_invs_pend, u16 sid, u16 size, u64 addr)
> > +    u32 max_invs_pend, u16 did, u16 seg, u8 bus, u8 devfn, u16 size,
> > + u64 addr)
> >  {
> > +    u16 sid = PCI_BDF2(bus, devfn);
> 
> Pointless local variable (but being a matter of taste, the VT-d maintainers will
> have the final say).
> 

I will drop this local variable.

> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -116,6 +116,7 @@ const unsigned long *pci_get_ro_map(u16 seg);  int
> > pci_add_device(u16 seg, u8 bus, u8 devfn,
> >                     const struct pci_dev_info *, nodeid_t node);  int
> > pci_remove_device(u16 seg, u8 bus, u8 devfn);
> > +void pci_hide_existing_device(struct pci_dev *pdev);
> >  int pci_ro_device(int seg, int bus, int devfn);  int
> > pci_hide_device(int bus, int devfn);
> 
> Please move the addition here.
> 


Agreed.


Quan


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