Re: [Xen-devel] [PATCH v14 1/3] IOMMU/x86: use a struct pci_dev* instead of SBDF

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.16 at 11:11, <quan.xu@xxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel