Re: [Xen-devel] [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF

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

On June 27, 2016 4:17 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 24.06.16 at 07:51, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/x86/ats.c
> > +++ b/xen/drivers/passthrough/x86/ats.c
> > @@ -22,26 +22,34 @@ 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 *pci_dev)
> 
> Is there anything preventing the second parameter to become a pointer to
> const too? Afaict that would in turn eliminate the need for some of the
> changes further up.
> 

If I make the second parameter to const, then compilation fails:

"""
  ats.c: In function 'enable_ats_device':
ats.c:71:23: error: assignment discards 'const' qualifier from pointer target 
type [-Werror]
         ats_dev->pdev = pdev;
                       ^
"""

Also I will hide pci_dev as device IOTLB flush error, with changing of pci_dev.
A const 'struct pci_dev *'  in  'struct pci_ats_dev' is not working..  I have 
not verified this, correct me if  I am wrong.


> >  {
> >      struct pci_ats_dev *pdev = NULL;
> >      u32 value;
> >      int pos;
> >
> > -    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> > +    pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev-
> >devfn,
> > +                                  PCI_EXT_CAP_ID_ATS);
> 
> Please add local variables seg, bus, and devfn, which will greatly reduce the
> number of changes you need to do to this function (and which likely will also
> produce better code).

Agreed. Good idea.

> 
> > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int
> > devfn)
> 
> For symmetry reasons this function would also get converted to taking const
> struct pci_dev *.
> 

What about ' struct pci_dev *', without const?

> > @@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int
> > bus, int devfn)
> 
> And this one then probably too.
> 

Ditto.

Quan


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