Re: [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice

["david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2706 bytes --]

On Wed, Jul 10, 2019 at 11:08:15AM +0000, Liu, Yi L wrote:
> > From: Peter Xu [mailto:zhexu@redhat.com]
> > Sent: Tuesday, July 9, 2019 10:12 AM
> > To: Liu, Yi L <yi.l.liu@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.

> > > +{
> > > +    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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]