> From: da...@gibson.dropbear.id.au [mailto:da...@gibson.dropbear.id.au]
> Sent: Thursday, July 11, 2019 11:52 AM
> To: Liu, Yi L <yi.l....@intel.com>
> Subject: Re: [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice
>
> On Wed, Jul 10, 2019 at 11:08:15AM +0000, Liu, Yi L wrote:
> > > From: Peter Xu [mailto:zh...@redhat.com]
> > > Sent: Tuesday, July 9, 2019 10:12 AM
> > > To: Liu, Yi L <yi.l....@intel.com>
> > > Subject: Re: [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice
> > >
> > > On Fri, Jul 05, 2019 at 07:01:36PM +0800, Liu Yi L wrote:
> > > > +void pci_setup_pasid_ops(PCIDevice *dev, PCIPASIDOps *ops)
> > > > +{
> > > > + assert(ops && !dev->pasid_ops);
> > > > + dev->pasid_ops = ops;
> > > > +}
> > > > +
> > > > +bool pci_device_is_ops_set(PCIBus *bus, int32_t devfn)
> > >
> > > Name should be "pci_device_is_pasid_ops_set". Or maybe you can simply
> > > drop this function because as long as you check it in helper functions
> > > like [1] below always then it seems even unecessary.
> >
> > yes, the name should be "pci_device_is_pasid_ops_set". I noticed your
> > comments on the necessity in another, let's talk in that thread. :-)
> >
> > > > +{
> > > > + PCIDevice *dev;
> > > > +
> > > > + if (!bus) {
> > > > + return false;
> > > > + }
> > > > +
> > > > + dev = bus->devices[devfn];
> > > > + return !!(dev && dev->pasid_ops);
> > > > +}
> > > > +
> > > > +int pci_device_request_pasid_alloc(PCIBus *bus, int32_t devfn,
> > > > + uint32_t min_pasid, uint32_t
> > > > max_pasid)
> > >
> > > From VT-d spec I see that the virtual command "allocate pasid" does
> > > not have bdf information so it's global, but here we've got bus/devfn.
> > > I'm curious is that reserved for ARM or some other arch?
> >
> > You are right. VT-d spec doesn’t have bdf info. But we need to pass the
> > allocation request via vfio. So this function has bdf info. In vIOMMU side,
> > it should select a vfio-pci device and invoke this callback when it wants to
> > request PASID alloc/free.
>
> That doesn't seem conceptually right. IIUC, the pasids "belong" to a
> sort of SVM context. It seems to be the alloc should be on that
> object - and that object would already have some connection to any
> relevant vfio containers. At the vfio level this seems like it should
> be a container operation rather than a device operation.
Hi David,
Yeah, I agree it should finally be a container operation. Actually, in the
callback implementation, it is a container operation. May refer to the
implementation in below patch. :-)
[RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
Thanks,
Yi Liu
> > > > +{
> > > > + PCIDevice *dev;
> > > > +
> > > > + if (!bus) {
> > > > + return -1;
> > > > + }
> > > > +
> > > > + dev = bus->devices[devfn];
> > > > + if (dev && dev->pasid_ops && dev->pasid_ops->alloc_pasid) {
> > >
> > > [1]
> > >
> > > > + return dev->pasid_ops->alloc_pasid(bus, devfn, min_pasid,
> > > > max_pasid);
> >
> > Thanks,
> > Yi Liu
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
