>>> On 04.07.16 at 11:11, <quan...@intel.com> wrote: > --- a/xen/drivers/passthrough/ats.h > +++ b/xen/drivers/passthrough/ats.h > @@ -19,9 +19,7 @@ > > struct pci_ats_dev { > struct list_head list; > - u16 seg; > - u8 bus; > - u8 devfn; > + struct pci_dev *pdev; > u16 ats_queue_depth; /* ATS device invalidation queue depth */ > const void *iommu; /* No common IOMMU struct so use void pointer */ > };
Taking into consideration the single use the struct iommu * here has, I think a different approach is needed to achieve the goal: Eliminate the global "ats_devices" and link ATS devices on a per-IOMMU list. Since that'll leave (besides the list member and the struct pci_dev back pointer) only ats_queue_depth, I then don't see the existence of the structure above warranted anymore: It should be subsumed into struct pci_dev. > @@ -34,9 +32,9 @@ struct pci_ats_dev { > extern struct list_head ats_devices; > extern bool_t ats_enabled; > > -int enable_ats_device(int seg, int bus, int devfn, const void *iommu); > -void disable_ats_device(int seg, int bus, int devfn); > -struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn); > +int enable_ats_device(const void *iommu, struct pci_dev *pdev); The ordering of parameters is backwards here. > --- a/xen/drivers/passthrough/x86/ats.c > +++ b/xen/drivers/passthrough/x86/ats.c > @@ -22,10 +22,12 @@ LIST_HEAD(ats_devices); > bool_t __read_mostly ats_enabled = 0; > boolean_param("ats", ats_enabled); > > -int enable_ats_device(int seg, int bus, int devfn, const void *iommu) > +int enable_ats_device(const void *iommu, struct pci_dev *pdev) > { > - struct pci_ats_dev *pdev = NULL; > + struct pci_ats_dev *ats_dev = NULL; > u32 value; > + u16 seg = pdev->seg; > + u8 bus = pdev->bus, devfn = pdev->devfn; > int pos; > > pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS); > @@ -39,9 +41,14 @@ int enable_ats_device(int seg, int bus, int devfn, const > void *iommu) > PCI_FUNC(devfn), pos + ATS_REG_CTL); > if ( value & ATS_ENABLE ) > { > - list_for_each_entry ( pdev, &ats_devices, list ) > + list_for_each_entry ( ats_dev, &ats_devices, list ) > { > - if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == > devfn ) > + struct pci_dev *_pdev = ats_dev->pdev; > + > + ASSERT(_pdev); > + if ( _pdev->seg == seg && > + _pdev->bus == bus && > + _pdev->devfn == devfn ) Why do you do SBDF based compares, when you can simply compare the two pointers now? (And btw., local variables should never begin with an underscore.) Since you had indicated that today is your last day at Intel, I guess I'll see to pick up this series (or maybe Kevin could) to bring it to the point where it can go in. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel